From e4189735c6a12a397a8f4a492b6c221bbbe8b84c Mon Sep 17 00:00:00 2001 From: M Platypus Date: Tue, 4 Aug 2026 18:34:13 -0400 Subject: [PATCH 1/2] fix: vision/audio argv builders silently drop or duplicate transform flags Two independent bugs in the vision/audio ai_modules' train/test argv builders: 1. image_base.py's train argv rendered data_proc_transforms through an f-string (stringified list, e.g. "['BINARIZE']"), while test argv passed the real list object -- the two builders disagreed on the wire format for the same parameter. prepare_transforms() (tinyml-tinyverse's train_base.py) only combines data_proc_transforms into args.transforms when isinstance(args.data_proc_transforms, list) is true, with no else branch, so the stringified form silently skipped it during training while testing still applied it -- pure train/test skew for anyone customizing this parameter, with no error or warning. timeseries_base.py already passes the raw list on both sides correctly; image_base.py's train side now matches it. feat_ext_transform/augmentation_transform don't have this defect (parsed via _normalize_transform_list's literal_eval fallback, which tolerates both forms) and were left untouched. 2. audio_base.py's train and test argv builders each declared --data-proc-transforms/--feat-ext-transform twice: once correctly as a raw list, and again later as a stringified f-string. argparse's last-occurrence-wins semantics meant the later, stringified declaration always won, making the earlier, correct one dead code -- the same underlying defect as (1), just duplicated instead of split across two methods. Removed the dead duplicate declarations in both methods, keeping the single correct raw-list version each already had. Verified with unit tests against the argv-building methods directly (no full ModelRunner instance needed -- these only read self.params). All 4 tests fail on the unmodified code with the exact symptoms described above (stringified list where a real list was expected; --data-proc-transforms appearing twice in the argv) and pass after the fix. Co-Authored-By: Claude Sonnet 5 --- .../tests/test_argv_builder_transform_bugs.py | 86 +++++++++++++++++++ .../training/tinyml_tinyverse/audio_base.py | 5 -- .../training/tinyml_tinyverse/image_base.py | 12 ++- 3 files changed, 97 insertions(+), 6 deletions(-) create mode 100644 tinyml-modelmaker/tests/test_argv_builder_transform_bugs.py diff --git a/tinyml-modelmaker/tests/test_argv_builder_transform_bugs.py b/tinyml-modelmaker/tests/test_argv_builder_transform_bugs.py new file mode 100644 index 00000000..6a4f3397 --- /dev/null +++ b/tinyml-modelmaker/tests/test_argv_builder_transform_bugs.py @@ -0,0 +1,86 @@ +"""Regression tests for two argv-builder bugs in vision/audio ai_modules: + +1. image_base.py's train argv rendered data_proc_transforms as a stringified + list while test argv passed the raw list -- prepare_transforms() (in + tinyml-tinyverse) only combines it into args.transforms when + isinstance(args.data_proc_transforms, list) is true, so the stringified + form silently skipped it during training while testing still applied it. + +2. audio_base.py's train and test argv builders each declared + --data-proc-transforms/--feat-ext-transform twice (once raw, once + stringified); argparse's last-occurrence-wins semantics meant the earlier, + correct raw declaration was always shadowed by the later, stringified one. + +Both are pure unit tests against the argv-building methods in isolation +(constructed via unittest.mock.MagicMock for self, rather than a full +ModelRunner instance) -- these methods only read attributes off self.params +and don't need real training infrastructure. +""" +from unittest.mock import MagicMock + +from tinyml_modelmaker.ai_modules.vision.training.tinyml_tinyverse.image_base import ( + BaseImageModelTraining, +) +from tinyml_modelmaker.ai_modules.audio.training.tinyml_tinyverse.audio_base import ( + BaseAudioModelTraining, +) + + +def _argv_value_after(argv, flag): + return argv[argv.index(flag) + 1] + + +def _count_occurrences(argv, flag): + return argv.count(flag) + + +def test_image_train_argv_passes_data_proc_transforms_as_a_raw_list(): + fake_self = MagicMock() + fake_self.params.data_processing_feature_extraction.data_proc_transforms = ["BINARIZE"] + + argv = BaseImageModelTraining._build_common_train_argv(fake_self, device="cpu", distributed=0) + + value = _argv_value_after(argv, "--data-proc-transforms") + assert value == ["BINARIZE"] + assert isinstance(value, list) + + +def test_image_train_and_test_argv_agree_on_data_proc_transforms_form(): + fake_self = MagicMock() + fake_self.params.data_processing_feature_extraction.data_proc_transforms = ["BINARIZE", "RESIZE"] + + train_argv = BaseImageModelTraining._build_common_train_argv(fake_self, device="cpu", distributed=0) + test_argv = BaseImageModelTraining._build_common_test_argv( + fake_self, device="cpu", data_path="/tmp/data", model_path="/tmp/model.onnx", output_dir="/tmp/out" + ) + + assert _argv_value_after(train_argv, "--data-proc-transforms") == \ + _argv_value_after(test_argv, "--data-proc-transforms") + + +def test_audio_train_argv_declares_data_proc_transforms_exactly_once(): + fake_self = MagicMock() + fake_self.params.data_processing_feature_extraction.data_proc_transforms = ["NORMALIZE"] + fake_self.params.data_processing_feature_extraction.feat_ext_transform = ["MFCC"] + + argv = BaseAudioModelTraining._build_common_train_argv(fake_self, device="cpu", distributed=0) + + assert _count_occurrences(argv, "--data-proc-transforms") == 1 + assert _count_occurrences(argv, "--feat-ext-transform") == 1 + assert _argv_value_after(argv, "--data-proc-transforms") == ["NORMALIZE"] + assert _argv_value_after(argv, "--feat-ext-transform") == ["MFCC"] + + +def test_audio_test_argv_declares_data_proc_transforms_exactly_once(): + fake_self = MagicMock() + fake_self.params.data_processing_feature_extraction.data_proc_transforms = ["NORMALIZE"] + fake_self.params.data_processing_feature_extraction.feat_ext_transform = ["MFCC"] + + argv = BaseAudioModelTraining._build_common_test_argv( + fake_self, device="cpu", data_path="/tmp/data", model_path="/tmp/model.onnx", output_dir="/tmp/out" + ) + + assert _count_occurrences(argv, "--data-proc-transforms") == 1 + assert _count_occurrences(argv, "--feat-ext-transform") == 1 + assert _argv_value_after(argv, "--data-proc-transforms") == ["NORMALIZE"] + assert _argv_value_after(argv, "--feat-ext-transform") == ["MFCC"] diff --git a/tinyml-modelmaker/tinyml_modelmaker/ai_modules/audio/training/tinyml_tinyverse/audio_base.py b/tinyml-modelmaker/tinyml_modelmaker/ai_modules/audio/training/tinyml_tinyverse/audio_base.py index e8cd74f5..d62270e1 100644 --- a/tinyml-modelmaker/tinyml_modelmaker/ai_modules/audio/training/tinyml_tinyverse/audio_base.py +++ b/tinyml-modelmaker/tinyml_modelmaker/ai_modules/audio/training/tinyml_tinyverse/audio_base.py @@ -341,9 +341,6 @@ def _build_common_train_argv(self, device, distributed): '--normalize-audio', f'{self.params.data_processing_feature_extraction.normalize_audio}', '--mono', f'{self.params.data_processing_feature_extraction.mono}', - '--data-proc-transforms', f'{self.params.data_processing_feature_extraction.data_proc_transforms}', - '--feat-ext-transform', f'{self.params.data_processing_feature_extraction.feat_ext_transform}', - '--output-int', f'{self.params.training.output_int}', '--variables', f'{self.params.data_processing_feature_extraction.variables}', '--lis', f'{self.params.training.log_file_path}', @@ -392,8 +389,6 @@ def _build_common_test_argv(self, device, data_path, model_path, output_dir): '--normalize-audio', f'{self.params.data_processing_feature_extraction.normalize_audio}', '--mono', f'{self.params.data_processing_feature_extraction.mono}', - '--data-proc-transforms', f'{self.params.data_processing_feature_extraction.data_proc_transforms}', - '--feat-ext-transform', f'{self.params.data_processing_feature_extraction.feat_ext_transform}', '--nn-for-feature-extraction', f'{self.params.data_processing_feature_extraction.nn_for_feature_extraction}', '--output-int', f'{self.params.training.output_int}', diff --git a/tinyml-modelmaker/tinyml_modelmaker/ai_modules/vision/training/tinyml_tinyverse/image_base.py b/tinyml-modelmaker/tinyml_modelmaker/ai_modules/vision/training/tinyml_tinyverse/image_base.py index a6d3d462..bca82bfc 100644 --- a/tinyml-modelmaker/tinyml_modelmaker/ai_modules/vision/training/tinyml_tinyverse/image_base.py +++ b/tinyml-modelmaker/tinyml_modelmaker/ai_modules/vision/training/tinyml_tinyverse/image_base.py @@ -339,7 +339,17 @@ def _build_common_train_argv(self, device, distributed): '--generic-model', f'{self.params.common.generic_model}', '--sampling-rate', f'{self.params.data_processing_feature_extraction.sampling_rate}', # Transform - '--data-proc-transforms', f'{self.params.data_processing_feature_extraction.data_proc_transforms}', + # Pass the raw list (not stringified) -- matches the test argv builder + # below and timeseries_base.py's reference implementation. + # prepare_transforms() (train_base.py) only combines data_proc_transforms + # into args.transforms when isinstance(args.data_proc_transforms, list) is + # true; the stringified form previously used here made that check false + # every time, so args.transforms was silently never set for training, + # while the (correctly raw) test-time argv still applied it -- a real + # train/test skew for anyone customizing this parameter. feat_ext_transform + # doesn't have this defect: it's parsed via _normalize_transform_list's + # literal_eval fallback, which tolerates both string and list forms. + '--data-proc-transforms', self.params.data_processing_feature_extraction.data_proc_transforms, '--feat-ext-transform', f'{self.params.data_processing_feature_extraction.feat_ext_transform}', '--augmentation-transform', f'{self.params.data_processing_feature_extraction.augmentation_transform}', '--feat-ext-store-dir', f'{self.params.data_processing_feature_extraction.feat_ext_store_dir}', From 72196d6367c17ed1a1df5915eea438652d309f82 Mon Sep 17 00:00:00 2001 From: M Platypus Date: Tue, 4 Aug 2026 19:59:59 -0400 Subject: [PATCH 2/2] fix: this PR's own earlier fix crashed every image-classification run An independent Opus peer review caught that fixing --data-proc-transforms (making it a raw list, matching test-time argv) introduced a new crash: prepare_transforms() (tinyml-tinyverse/references/common/train_base.py) does args.transforms = args.data_proc_transforms + args.feat_ext_transform whenever args.data_proc_transforms is a list. image_base.py's train argv builder still stringified --feat-ext-transform (and --augmentation-transform), so once --data-proc-transforms became a real list, this became `list + str`, raising: TypeError: can only concatenate list (not "str") to list on every image-classification training run (the vision default for data_proc_transforms is [], and even an empty list still enters this branch, so this wasn't conditional on the user setting anything). An earlier comment in this file claimed feat_ext_transform "doesn't have this defect: it's parsed via _normalize_transform_list's literal_eval fallback, which tolerates both string and list forms" -- that's true of a different consumer (image_dataset.py's Dataset construction, later in the pipeline) but not of prepare_transforms(), which runs first and reads feat_ext_transform directly with no such tolerance. Fixed by passing --feat-ext-transform and --augmentation-transform as raw lists too, matching _build_common_test_argv (which already did this) and timeseries_base.py's reference implementation. Adds a test that drives the REAL prepare_transforms() against the argv builder's actual output, rather than only asserting argv shape (which is exactly why the previous test suite didn't catch this -- MagicMock-based shape checks never fed the argv through the function that actually crashes). Verified to fail on the pre-this-commit code with the exact TypeError above and pass post-fix. Co-Authored-By: Claude Sonnet 5 --- .../tests/test_argv_builder_transform_bugs.py | 44 ++++++++++++++++--- .../training/tinyml_tinyverse/image_base.py | 21 +++++---- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/tinyml-modelmaker/tests/test_argv_builder_transform_bugs.py b/tinyml-modelmaker/tests/test_argv_builder_transform_bugs.py index 6a4f3397..9f838a1a 100644 --- a/tinyml-modelmaker/tests/test_argv_builder_transform_bugs.py +++ b/tinyml-modelmaker/tests/test_argv_builder_transform_bugs.py @@ -1,4 +1,4 @@ -"""Regression tests for two argv-builder bugs in vision/audio ai_modules: +"""Regression tests for three argv-builder bugs in vision/audio ai_modules: 1. image_base.py's train argv rendered data_proc_transforms as a stringified list while test argv passed the raw list -- prepare_transforms() (in @@ -11,11 +11,23 @@ stringified); argparse's last-occurrence-wins semantics meant the earlier, correct raw declaration was always shadowed by the later, stringified one. -Both are pure unit tests against the argv-building methods in isolation -(constructed via unittest.mock.MagicMock for self, rather than a full -ModelRunner instance) -- these methods only read attributes off self.params -and don't need real training infrastructure. +3. Fixing (1) by making data_proc_transforms a raw list exposed a second bug + an independent peer review caught: prepare_transforms() does + `args.data_proc_transforms + args.feat_ext_transform` whenever + data_proc_transforms is a list. image_base.py's train argv builder still + stringified --feat-ext-transform (and --augmentation-transform), so this + became `list + str`, raising TypeError on every image-classification + training run. test_image_train_argv_feat_ext_transform_survives_ + prepare_transforms below exercises the REAL prepare_transforms() against + the argv builder's actual output -- the shape-only MagicMock tests below + couldn't catch this since they never fed argv through it. + +Most of these are pure unit tests against the argv-building methods in +isolation (constructed via unittest.mock.MagicMock for self, rather than a +full ModelRunner instance) -- these methods only read attributes off +self.params and don't need real training infrastructure. """ +from argparse import Namespace from unittest.mock import MagicMock from tinyml_modelmaker.ai_modules.vision.training.tinyml_tinyverse.image_base import ( @@ -24,6 +36,7 @@ from tinyml_modelmaker.ai_modules.audio.training.tinyml_tinyverse.audio_base import ( BaseAudioModelTraining, ) +from tinyml_tinyverse.references.common.train_base import prepare_transforms def _argv_value_after(argv, flag): @@ -45,6 +58,27 @@ def test_image_train_argv_passes_data_proc_transforms_as_a_raw_list(): assert isinstance(value, list) +def test_image_train_argv_feat_ext_transform_survives_prepare_transforms(): + """Drives the REAL prepare_transforms() (tinyml-tinyverse) against the + actual argv the train-argv builder produces -- data_proc_transforms and + feat_ext_transform must both come out as lists, or the `+` inside + prepare_transforms raises TypeError.""" + fake_self = MagicMock() + fake_self.params.data_processing_feature_extraction.data_proc_transforms = ["BINARIZE"] + fake_self.params.data_processing_feature_extraction.feat_ext_transform = ["MFCC"] + + argv = BaseImageModelTraining._build_common_train_argv(fake_self, device="cpu", distributed=0) + + args = Namespace( + data_proc_transforms=_argv_value_after(argv, "--data-proc-transforms"), + feat_ext_transform=_argv_value_after(argv, "--feat-ext-transform"), + ) + + prepare_transforms(args) + + assert args.transforms == ["BINARIZE", "MFCC"] + + def test_image_train_and_test_argv_agree_on_data_proc_transforms_form(): fake_self = MagicMock() fake_self.params.data_processing_feature_extraction.data_proc_transforms = ["BINARIZE", "RESIZE"] diff --git a/tinyml-modelmaker/tinyml_modelmaker/ai_modules/vision/training/tinyml_tinyverse/image_base.py b/tinyml-modelmaker/tinyml_modelmaker/ai_modules/vision/training/tinyml_tinyverse/image_base.py index bca82bfc..da110355 100644 --- a/tinyml-modelmaker/tinyml_modelmaker/ai_modules/vision/training/tinyml_tinyverse/image_base.py +++ b/tinyml-modelmaker/tinyml_modelmaker/ai_modules/vision/training/tinyml_tinyverse/image_base.py @@ -339,19 +339,18 @@ def _build_common_train_argv(self, device, distributed): '--generic-model', f'{self.params.common.generic_model}', '--sampling-rate', f'{self.params.data_processing_feature_extraction.sampling_rate}', # Transform - # Pass the raw list (not stringified) -- matches the test argv builder + # Pass raw lists (not stringified) -- matches the test argv builder # below and timeseries_base.py's reference implementation. - # prepare_transforms() (train_base.py) only combines data_proc_transforms - # into args.transforms when isinstance(args.data_proc_transforms, list) is - # true; the stringified form previously used here made that check false - # every time, so args.transforms was silently never set for training, - # while the (correctly raw) test-time argv still applied it -- a real - # train/test skew for anyone customizing this parameter. feat_ext_transform - # doesn't have this defect: it's parsed via _normalize_transform_list's - # literal_eval fallback, which tolerates both string and list forms. + # prepare_transforms() (train_base.py) does + # `args.data_proc_transforms + args.feat_ext_transform` whenever + # isinstance(args.data_proc_transforms, list) is true. Both operands + # must therefore be actual lists at that point, not just + # data_proc_transforms: a stringified feat_ext_transform (e.g. "[]") + # makes this `list + str`, raising TypeError on every training run + # once data_proc_transforms itself was made a raw list. '--data-proc-transforms', self.params.data_processing_feature_extraction.data_proc_transforms, - '--feat-ext-transform', f'{self.params.data_processing_feature_extraction.feat_ext_transform}', - '--augmentation-transform', f'{self.params.data_processing_feature_extraction.augmentation_transform}', + '--feat-ext-transform', self.params.data_processing_feature_extraction.feat_ext_transform, + '--augmentation-transform', self.params.data_processing_feature_extraction.augmentation_transform, '--feat-ext-store-dir', f'{self.params.data_processing_feature_extraction.feat_ext_store_dir}', '--dont-train-just-feat-ext', f'{self.params.data_processing_feature_extraction.dont_train_just_feat_ext}', '--store-feat-ext-data', f'{self.params.data_processing_feature_extraction.store_feat_ext_data}',