diff --git a/tinyml-modelmaker/tests/test_config_dict.py b/tinyml-modelmaker/tests/test_config_dict.py index 2255d786..760a6a37 100644 --- a/tinyml-modelmaker/tests/test_config_dict.py +++ b/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 diff --git a/tinyml-modelmaker/tests/test_hardware_defaults.py b/tinyml-modelmaker/tests/test_hardware_defaults.py index 84c25f88..fde6b4b7 100644 --- a/tinyml-modelmaker/tests/test_hardware_defaults.py +++ b/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 == '' diff --git a/tinyml-modelmaker/tests/test_hardware_defaults_integration.py b/tinyml-modelmaker/tests/test_hardware_defaults_integration.py index 8323725c..0c21ae3b 100644 --- a/tinyml-modelmaker/tests/test_hardware_defaults_integration.py +++ b/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 diff --git a/tinyml-modelmaker/tinyml_modelmaker/ai_modules/timeseries/params.py b/tinyml-modelmaker/tinyml_modelmaker/ai_modules/timeseries/params.py index c468eb1e..9c26cfae 100644 --- a/tinyml-modelmaker/tinyml_modelmaker/ai_modules/timeseries/params.py +++ b/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 diff --git a/tinyml-modelmaker/tinyml_modelmaker/utils/config_dict.py b/tinyml-modelmaker/tinyml_modelmaker/utils/config_dict.py index d40e4cf2..a0d5a4d6 100644 --- a/tinyml-modelmaker/tinyml_modelmaker/utils/config_dict.py +++ b/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(): diff --git a/tinyml-modelmaker/tinyml_modelmaker/utils/hardware_defaults.py b/tinyml-modelmaker/tinyml_modelmaker/utils/hardware_defaults.py index 00488350..993aa43d 100644 --- a/tinyml-modelmaker/tinyml_modelmaker/utils/hardware_defaults.py +++ b/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." + ) + # + #