From c62944136d668395b8b335cf11708eb50d5f5218 Mon Sep 17 00:00:00 2001 From: M Platypus Date: Fri, 14 Aug 2026 10:00:17 -0400 Subject: [PATCH 1/3] fix: YAML-path configs silently dropped user settings (F-1/F-1a) An adversarial review of apply_hardware_defaults's remaining call site (timeseries -- the only module still using it after radar/vision/audio were reverted) found the bug it protects against goes deeper than originally scoped: F-1a (root cause): init_params(*args) builds ConfigDict(default_params, *args). When a caller passes a YAML path as args[0] -- a normal, documented call shape -- ConfigDict's *args merge loop only merged dict/ConfigDict values and silently skipped strings, so the file was never read at all. Not just compile_model/native_amp: every field in that YAML was discarded, observable only where something later mutated the resulting default (which is exactly what apply_hardware_defaults does for compile_model/native_amp, making F-1 the visible symptom of a config-wide bug). F-1 (surface bug): explicitly_set was derived via `isinstance(args[0], dict)`, so a path-based config always looked like "the user set nothing," letting apply_hardware_defaults silently flip an explicit compile_model: 0 / native_amp: false back on for any CUDA host. Fix: - ConfigDict.resolve_config_value() factors out YAML-path loading and is now used for both the `input` position and each `*args` position, so a path passed anywhere in the constructor's arguments is actually read. - hardware_defaults.explicit_training_keys() replaces the isinstance(dict) check, resolving all positional args the same way ConfigDict does, so "what did the user set" is computed from the real content regardless of whether it arrived as a dict or a path. - Removed the stale "vision, audio -- Phase 2" hasattr-guard docstring (F-8): those modules no longer call this function at all as of today's earlier revert. An existing test previously asserted the bug as correct behavior (test_init_params_does_not_crash_on_non_dict_first_arg used a bogus path specifically because, per its own comment, "this never actually attempts to read the file" -- and asserted compile_model=1/native_amp=True as the expected outcome of that silent discard). Replaced with tests proving a real YAML path's explicit settings -- including a non-hardware-defaults canary field -- are now respected, auto-enable still fires when the user expresses no opinion, and a genuinely bad path now fails loudly instead of silently no-op'ing. Added ConfigDict-level tests for the same fix independent of hardware_defaults, since the bug lived in shared, multi-caller code. Verified the production reference CLI's two-stage init()/update() call pattern (run_tinyml_modelmaker.py) is unaffected -- it was already incidentally safe from F-1/F-1a due to an unrelated implementation detail, not a designed contract, which is part of why this was worth fixing at the root rather than leaving as a latent trap for any caller that uses the more natural single-call form. Co-Authored-By: Claude Sonnet 5 --- tinyml-modelmaker/tests/test_config_dict.py | 18 +++++ .../test_hardware_defaults_integration.py | 68 ++++++++++++++++--- .../ai_modules/timeseries/params.py | 8 +-- .../tinyml_modelmaker/utils/config_dict.py | 34 ++++++++-- .../utils/hardware_defaults.py | 31 ++++++++- 5 files changed, 133 insertions(+), 26 deletions(-) diff --git a/tinyml-modelmaker/tests/test_config_dict.py b/tinyml-modelmaker/tests/test_config_dict.py index 2255d786..eb0b180c 100644 --- a/tinyml-modelmaker/tests/test_config_dict.py +++ b/tinyml-modelmaker/tests/test_config_dict.py @@ -50,3 +50,21 @@ 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)) diff --git a/tinyml-modelmaker/tests/test_hardware_defaults_integration.py b/tinyml-modelmaker/tests/test_hardware_defaults_integration.py index 8323725c..4674ae23 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,23 +46,70 @@ 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) diff --git a/tinyml-modelmaker/tinyml_modelmaker/ai_modules/timeseries/params.py b/tinyml-modelmaker/tinyml_modelmaker/ai_modules/timeseries/params.py index c468eb1e..72591b2f 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) 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..caf67a0b 100644 --- a/tinyml-modelmaker/tinyml_modelmaker/utils/config_dict.py +++ b/tinyml-modelmaker/tinyml_modelmaker/utils/config_dict.py @@ -42,20 +42,23 @@ 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): + settings_file = value + value = self.resolve_config_value(value) + # if isinstance(value, (dict, ConfigDict)): self._deep_merge(input_dict, value) # @@ -95,6 +98,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..9899a6bf 100644 --- a/tinyml-modelmaker/tinyml_modelmaker/utils/hardware_defaults.py +++ b/tinyml-modelmaker/tinyml_modelmaker/utils/hardware_defaults.py @@ -1,13 +1,38 @@ import torch +from .config_dict import ConfigDict + + +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. """ if not torch.cuda.is_available(): return From b0dc35a83cf93b26a643d4020fd2575fc5980211 Mon Sep 17 00:00:00 2001 From: M Platypus Date: Mon, 31 Aug 2026 10:58:39 -0400 Subject: [PATCH 2/3] fix: log when apply_hardware_defaults actually changes a value (F-3) The policy mutated params silently -- nothing recorded that it fired or why, so a run's effective config could differ from the requested one with no trace explaining the difference. The original adversarial analysis ranked this above the specific-defaults questions: "a silent policy is the root problem; the specific defaults are secondary." Adds one INFO line per flag, only on an actual mutation (not on every call) -- CUDA-unavailable and already-explicit-set paths stay silent. Co-Authored-By: Claude Sonnet 5 --- .../tests/test_hardware_defaults.py | 30 +++++++++++++++++++ .../utils/hardware_defaults.py | 20 ++++++++++++- 2 files changed, 49 insertions(+), 1 deletion(-) 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/tinyml_modelmaker/utils/hardware_defaults.py b/tinyml-modelmaker/tinyml_modelmaker/utils/hardware_defaults.py index 9899a6bf..993aa43d 100644 --- a/tinyml-modelmaker/tinyml_modelmaker/utils/hardware_defaults.py +++ b/tinyml-modelmaker/tinyml_modelmaker/utils/hardware_defaults.py @@ -1,7 +1,11 @@ +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 @@ -32,13 +36,27 @@ def apply_hardware_defaults(params, explicitly_set: set) -> None: Skips fields present in explicitly_set — those are deliberate user choices and must not be overridden. hasattr guards keep this safe for - any params object that doesn't carry these fields. + 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." + ) + # + # From 0574251bcf08d6c4873d1333306e5eddcea5d5f0 Mon Sep 17 00:00:00 2001 From: M Platypus Date: Mon, 31 Aug 2026 13:45:16 -0400 Subject: [PATCH 3/3] fix: 2 CodeRabbit findings in the F-1/F-1a fix (settings_file, kwargs) CodeRabbit review of this session's commits (ba7e770..main) caught two real gaps in the earlier ConfigDict/explicit_training_keys fix, both independently reproduced before fixing: 1. config_dict.py: the *args loop unconditionally overwrote settings_file with the last string-typed arg processed, even when that arg's YAML didn't itself define include_files. When an earlier arg defines include_files but a later path-only arg doesn't, include_files now resolves relative to the wrong directory. Fixed by only adopting an arg's path as settings_file when it actually supplies include_files -- matching _deep_merge's own "last one wins" semantics for that specific key. 2. timeseries/params.py: explicit_training_keys(*args) never inspected kwargs, even though ConfigDict.__init__ treats **kwargs as an equally-legitimate way to set fields (input_dict[key] = value). compile_model/native_amp set via init_params(training=dict(...)) would look "unset" to apply_hardware_defaults and get silently overridden on CUDA -- the same class of bug the fix was written to close, just via a different call shape. Fixed by passing kwargs through as an extra positional arg. Both confirmed via direct before/after reproduction, not just inferred from reading the diff. No current caller in this repo uses either call shape, so neither was an active production bug -- but both are latent traps in shared, multi-caller code, matching why the original fix was worth doing at the root instead of leaving it caller-specific. Co-Authored-By: Claude Sonnet 5 --- tinyml-modelmaker/tests/test_config_dict.py | 25 +++++++++++++++++++ .../test_hardware_defaults_integration.py | 13 ++++++++++ .../ai_modules/timeseries/params.py | 2 +- .../tinyml_modelmaker/utils/config_dict.py | 9 ++++++- 4 files changed, 47 insertions(+), 2 deletions(-) diff --git a/tinyml-modelmaker/tests/test_config_dict.py b/tinyml-modelmaker/tests/test_config_dict.py index eb0b180c..760a6a37 100644 --- a/tinyml-modelmaker/tests/test_config_dict.py +++ b/tinyml-modelmaker/tests/test_config_dict.py @@ -68,3 +68,28 @@ def test_constructor_args_bad_extension_raises(self, tmp_path): 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_integration.py b/tinyml-modelmaker/tests/test_hardware_defaults_integration.py index 4674ae23..0c21ae3b 100644 --- a/tinyml-modelmaker/tests/test_hardware_defaults_integration.py +++ b/tinyml-modelmaker/tests/test_hardware_defaults_integration.py @@ -115,3 +115,16 @@ def test_init_params_does_not_crash_on_none_first_arg(): 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 72591b2f..9c26cfae 100644 --- a/tinyml-modelmaker/tinyml_modelmaker/ai_modules/timeseries/params.py +++ b/tinyml-modelmaker/tinyml_modelmaker/ai_modules/timeseries/params.py @@ -223,7 +223,7 @@ def init_params(*args, **kwargs): ), ) - user_training_keys = explicit_training_keys(*args) + 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 caf67a0b..a0d5a4d6 100644 --- a/tinyml-modelmaker/tinyml_modelmaker/utils/config_dict.py +++ b/tinyml-modelmaker/tinyml_modelmaker/utils/config_dict.py @@ -56,8 +56,15 @@ def __init__(self, input=None, *args, **kwargs): # isn't a dict/ConfigDict. for value in args: if isinstance(value, str): - settings_file = value + 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)