fix: YAML-path config discard + no observability in apply_hardware_defaults

tinyml-modelmaker/tests/test_config_dict.py

@@ -50,3 +50,46 @@ def test_constructor_args_deep_merge_nested_dict(self):
         user = dict(training=dict(b=99))
         cfg = ConfigDict(default, user)
         assert dict(cfg.training) == {"a": 1, "b": 99}
+
+    def test_constructor_args_path_string_is_loaded_and_merged(self, tmp_path):
+        """A .yaml path passed as a positional arg *after* `input` (e.g.
+        ConfigDict(default_params, user_yaml_path), the shape init_params
+        uses) must actually be read and merged -- not silently discarded
+        because it's a string rather than a dict/ConfigDict."""
+        default = dict(training=dict(a=1, b=2))
+        user_file = tmp_path / "user.yaml"
+        user_file.write_text(yaml.dump(dict(training=dict(b=99))))
+        cfg = ConfigDict(default, str(user_file))
+        assert dict(cfg.training) == {"a": 1, "b": 99}
+
+    def test_constructor_args_bad_extension_raises(self, tmp_path):
+        default = dict(training=dict(a=1))
+        bad_file = tmp_path / "user.txt"
+        bad_file.write_text("a: 1")
+        with pytest.raises(ValueError, match="unrecognized file type"):
+            ConfigDict(default, str(bad_file))
+
+    def test_include_files_resolves_against_the_arg_that_defines_it(self, tmp_path):
+        """Regression test (CodeRabbit finding): when multiple positional
+        .yaml-path args are passed and only an EARLIER one defines
+        include_files, settings_file must not be clobbered by a LATER
+        path-only arg that has no include_files of its own -- otherwise
+        the relative include path resolves against the wrong directory.
+        """
+        dir_a = tmp_path / "dir_a"
+        dir_a.mkdir()
+        (dir_a / "shared.yaml").write_text(yaml.dump({"from_shared": 42}))
+        config_a = dir_a / "config_a.yaml"
+        config_a.write_text(yaml.dump({"include_files": ["shared.yaml"]}))
+
+        dir_b = tmp_path / "dir_b"
+        dir_b.mkdir()
+        config_b = dir_b / "config_b.yaml"
+        config_b.write_text(yaml.dump({"unrelated": True}))
+
+        cfg = ConfigDict({}, str(config_a), str(config_b))
+        assert cfg.from_shared == 42, (
+            "include_files from config_a should resolve relative to dir_a "
+            "even though config_b (no include_files) was passed after it"
+        )
+        assert cfg.unrelated is True

tinyml-modelmaker/tests/test_hardware_defaults.py

@@ -52,3 +52,33 @@ def test_safe_when_params_lacks_flags():
     with patch('torch.cuda.is_available', return_value=True):
         apply_hardware_defaults(params, set())  # must not raise
     assert params.training.batch_size == 32
+
+
+def test_logs_when_it_actually_changes_a_value(caplog):
+    """F-3: the policy must announce itself when it fires, so an effective
+    config that differs from the requested one isn't silently unexplained."""
+    from tinyml_modelmaker.utils.hardware_defaults import apply_hardware_defaults
+    params = _make_params()
+    with patch('torch.cuda.is_available', return_value=True):
+        with caplog.at_level('INFO'):
+            apply_hardware_defaults(params, set())
+    assert 'compile_model' in caplog.text
+    assert 'native_amp' in caplog.text
+
+
+def test_does_not_log_when_nothing_changes(caplog):
+    """No log noise when CUDA is unavailable, or the user already set both
+    flags -- only log on an actual mutation."""
+    from tinyml_modelmaker.utils.hardware_defaults import apply_hardware_defaults
+
+    params = _make_params()
+    with patch('torch.cuda.is_available', return_value=False):
+        with caplog.at_level('INFO'):
+            apply_hardware_defaults(params, set())
+    assert caplog.text == ''
+
+    params2 = _make_params()
+    with patch('torch.cuda.is_available', return_value=True):
+        with caplog.at_level('INFO'):
+            apply_hardware_defaults(params2, {'compile_model', 'native_amp'})
+    assert caplog.text == ''

tinyml-modelmaker/tests/test_hardware_defaults_integration.py

@@ -1,5 +1,6 @@
 from unittest.mock import patch
 import pytest
+import yaml
 
 
 def test_init_params_auto_enables_on_cuda():
@@ -45,25 +46,85 @@ def test_init_params_no_change_without_cuda():
     assert params.training.native_amp is False
 
 
-def test_init_params_does_not_crash_on_non_dict_first_arg():
-    """ConfigDict documents a YAML path string (or None) as valid first
-    positional input, not just a dict. init_params's own explicitly-set-key
-    detection must not assume args[0] is dict-like and crash instead."""
+def test_init_params_respects_explicit_settings_from_yaml_path(tmp_path):
+    """Regression test for a bug found by an independent analysis of this
+    function (mmcli's ANALYSIS-cuda-auto-defaults.md, finding F-1) and
+    reproduced here: init_params's explicitly-set-key detection used to
+    check `isinstance(args[0], dict)`, so a YAML file passed by *path*
+    (not a dict) always looked like "the user set nothing" -- silently
+    overriding an explicit compile_model: 0 / native_amp: false on any
+    CUDA machine. Also proves the deeper bug (F-1a): passing a path as a
+    positional arg to init_params used to silently discard the whole file
+    -- ConfigDict's *args merge loop only merged dict/ConfigDict values,
+    so a string arg (a YAML path) was dropped entirely, not just
+    misclassified as non-explicit. model_name here is the canary: if the
+    file were still being silently discarded, it would come back None
+    (the hardcoded default) instead of the value actually written below."""
     from tinyml_modelmaker.ai_modules.timeseries.params import init_params
+    config_file = tmp_path / "config.yaml"
+    config_file.write_text(yaml.dump({
+        "training": {
+            "model_name": "my_explicit_model",
+            "compile_model": 0,
+            "native_amp": False,
+        },
+    }))
     with patch('torch.cuda.is_available', return_value=True):
-        # A bogus path is fine here -- ConfigDict's *args merge loop only
-        # merges dict/ConfigDict values and silently ignores strings, so
-        # this never actually attempts to read the file. The point of this
-        # test is solely that passing a string doesn't crash init_params's
-        # own explicitly-set-key detection before ConfigDict is even built.
-        params = init_params('/nonexistent/path/to/config.yaml')
+        params = init_params(str(config_file))
+    assert params.training.model_name == "my_explicit_model", (
+        "the YAML's own content was lost -- init_params(path) must load "
+        "the file, not just avoid crashing on it"
+    )
+    assert params.training.compile_model == 0, (
+        "explicit compile_model: 0 from a YAML-path config was silently overridden"
+    )
+    assert params.training.native_amp is False, (
+        "explicit native_amp: false from a YAML-path config was silently overridden"
+    )
+
+
+def test_init_params_still_auto_enables_for_yaml_path_with_no_opinion(tmp_path):
+    """The fix for the above must not make YAML-path configs opt out of
+    the auto-enable policy entirely -- a YAML that doesn't mention
+    compile_model/native_amp at all should still get them auto-enabled on
+    CUDA, exactly like an equivalent dict config already does."""
+    from tinyml_modelmaker.ai_modules.timeseries.params import init_params
+    config_file = tmp_path / "config.yaml"
+    config_file.write_text(yaml.dump({"training": {"model_name": "some_model"}}))
+    with patch('torch.cuda.is_available', return_value=True):
+        params = init_params(str(config_file))
     assert params.training.compile_model == 1
     assert params.training.native_amp is True
 
 
+def test_init_params_raises_on_nonexistent_yaml_path():
+    """A genuinely bad path must fail loudly, not silently no-op -- the
+    same way ConfigDict(bad_path) already behaves when the path is passed
+    as the constructor's `input` instead of as a later positional arg."""
+    from tinyml_modelmaker.ai_modules.timeseries.params import init_params
+    with patch('torch.cuda.is_available', return_value=True):
+        with pytest.raises(FileNotFoundError):
+            init_params('/nonexistent/path/to/config.yaml')
+
+
 def test_init_params_does_not_crash_on_none_first_arg():
+    """None is a documented valid first positional argument (an empty
+    config) and must still be handled without error."""
     from tinyml_modelmaker.ai_modules.timeseries.params import init_params
     with patch('torch.cuda.is_available', return_value=True):
         params = init_params(None)
     assert params.training.compile_model == 1
     assert params.training.native_amp is True
+
+
+def test_init_params_respects_explicit_settings_passed_via_kwargs():
+    """Regression test (CodeRabbit finding): ConfigDict's constructor also
+    accepts explicit settings via **kwargs (`input_dict[key] = value` in
+    ConfigDict.__init__), not just positional args -- explicit_training_keys
+    must inspect kwargs too, or a caller using this documented call shape
+    would have its explicit compile_model/native_amp silently overridden."""
+    from tinyml_modelmaker.ai_modules.timeseries.params import init_params
+    with patch('torch.cuda.is_available', return_value=True):
+        params = init_params(training=dict(model_name="kwarg_model", compile_model=0, native_amp=False))
+    assert params.training.compile_model == 0
+    assert params.training.native_amp is False

tinyml-modelmaker/tinyml_modelmaker/ai_modules/timeseries/params.py

@@ -35,7 +35,7 @@
 
 from ... import utils
 from . import constants
-from ...utils.hardware_defaults import apply_hardware_defaults
+from ...utils.hardware_defaults import apply_hardware_defaults, explicit_training_keys
 
 
 def init_params(*args, **kwargs):
@@ -223,11 +223,7 @@ def init_params(*args, **kwargs):
         ),
     )
 
-    # args[0] is usually a user config dict, but ConfigDict itself also
-    # accepts a YAML path string or None as a first positional argument --
-    # only inspect it as a mapping when it actually is one.
-    user_training_keys = set(args[0].get('training', {}).keys()) \
-        if args and isinstance(args[0], dict) else set()
+    user_training_keys = explicit_training_keys(*args, kwargs)
     params = utils.ConfigDict(default_params, *args, **kwargs)
     apply_hardware_defaults(params, user_training_keys)
     return params

tinyml-modelmaker/tinyml_modelmaker/utils/config_dict.py

@@ -42,20 +42,30 @@ def __init__(self, input=None, *args, **kwargs):
         input_dict = dict()
         settings_file = None
         if isinstance(input, str):
-            ext = os.path.splitext(input)[1]
-            if ext != '.yaml':
-                raise ValueError(f'unrecognized file type for: {input}')
-            with open(input) as fp:
-                input_dict = yaml.safe_load(fp)
-            #
+            input_dict = self.resolve_config_value(input)
             settings_file = input
         elif isinstance(input, dict):
             input_dict = input
         elif input is not None:
             raise TypeError('got invalid input')
         #
-        # override the entries with args
+        # override the entries with args -- a positional arg may also be a
+        # .yaml path string (not just a dict), matching what `input` itself
+        # accepts. Resolve it the same way so a path-based override is
+        # actually merged in, instead of silently discarded because it
+        # isn't a dict/ConfigDict.
         for value in args:
+            if isinstance(value, str):
+                path = value
+                value = self.resolve_config_value(value)
+                # Only adopt this arg's path as settings_file if it's the
+                # one actually supplying include_files -- otherwise a later
+                # path-only arg with no include_files of its own would
+                # clobber the correct base path from an earlier source.
+                if isinstance(value, dict) and 'include_files' in value:
+                    settings_file = path
+                #
+            #
             if isinstance(value, (dict, ConfigDict)):
                 self._deep_merge(input_dict, value)
             #
@@ -95,6 +105,23 @@ def __setstate__(self, state):
     def _initialize(self):
         pass
 
+    @staticmethod
+    def resolve_config_value(value):
+        """Resolve a .yaml path string into its loaded dict; pass
+        dicts/ConfigDicts through unchanged. Shared by both the `input`
+        position and the `*args` positions of the constructor so a
+        path-based config is loaded consistently no matter which position
+        it's passed in."""
+        if isinstance(value, str):
+            ext = os.path.splitext(value)[1]
+            if ext != '.yaml':
+                raise ValueError(f'unrecognized file type for: {value}')
+            with open(value) as fp:
+                return yaml.safe_load(fp)
+            #
+        #
+        return value
+
     @staticmethod
     def _deep_merge(target, source):
         for key, value in source.items():

tinyml-modelmaker/tinyml_modelmaker/utils/hardware_defaults.py

@@ -1,19 +1,62 @@
+import logging
+
 import torch
 
+from .config_dict import ConfigDict
+
+logger = logging.getLogger(__name__)
+
+
+def explicit_training_keys(*args) -> set:
+    """Union of 'training'-section keys explicitly present across all
+    positional config args passed to init_params -- dicts, ConfigDicts, or
+    .yaml path strings.
+
+    Mirrors ConfigDict's own arg resolution (see
+    ConfigDict.resolve_config_value) so a YAML-path config is inspected the
+    same way a dict config is. Checking only `isinstance(args[0], dict)`
+    made every setting in a path-based config look unset, so
+    apply_hardware_defaults would silently override explicit choices (e.g.
+    compile_model: 0) written to a YAML file passed by path.
+    """
+    keys = set()
+    for value in args:
+        if isinstance(value, str):
+            value = ConfigDict.resolve_config_value(value)
+        #
+        if isinstance(value, (dict, ConfigDict)):
+            keys |= set((value.get('training') or {}).keys())
+        #
+    #
+    return keys
+
 
 def apply_hardware_defaults(params, explicitly_set: set) -> None:
     """Auto-enable compile_model and native_amp when CUDA is available.
 
     Skips fields present in explicitly_set — those are deliberate user
-    choices from the YAML config and must not be overridden.
-    hasattr guards keep this safe for params that don't carry these fields
-    yet (vision, audio — Phase 2).
+    choices and must not be overridden. hasattr guards keep this safe for
+    any params object that doesn't carry these fields. Logs an INFO line
+    whenever it actually changes a value, so a run's effective config is
+    never silently different from what was requested with no trace of why.
     """
     if not torch.cuda.is_available():
         return
     if 'compile_model' not in explicitly_set and hasattr(params.training, 'compile_model'):
         if getattr(params.training, 'compile_model', 0) == 0:
             params.training.compile_model = 1
+            logger.info(
+                "CUDA detected: auto-enabling training.compile_model (0 -> 1); "
+                "not specified in config. Set it explicitly to override."
+            )
+        #
+    #
     if 'native_amp' not in explicitly_set and hasattr(params.training, 'native_amp'):
         if not getattr(params.training, 'native_amp', False):
             params.training.native_amp = True
+            logger.info(
+                "CUDA detected: auto-enabling training.native_amp (False -> True); "
+                "not specified in config. Set it explicitly to override."
+            )
+        #
+    #