Skip to content

ADR-224: Implement intent stages (PhraseVariator, VocabComputer, DVC entry-points)#217

Merged
jodavis merged 9 commits into
feature/ADR-191-oop-ml-pipelinefrom
dev/claude/ADR-223-implement-modifierstage-abstract-base
Jun 11, 2026
Merged

ADR-224: Implement intent stages (PhraseVariator, VocabComputer, DVC entry-points)#217
jodavis merged 9 commits into
feature/ADR-191-oop-ml-pipelinefrom
dev/claude/ADR-223-implement-modifierstage-abstract-base

Conversation

@jodavis-claude

Copy link
Copy Markdown
Collaborator

ADR-224 — Intent Stages: PhraseVariator, VocabComputer, DVC Entry-Points

Summary

Implements Task 3 of the OOP pipeline refactor (ml/_spec_OopPipeline.md): the ml/pipeline/intent/ package, ml/pipeline/stages/conventions.py, and the first two DVC entry-points.

Files Added

ml/pipeline/intent/phrase_variator.py

  • PhraseVariator class with injectable rng: random.Random
  • generate(base_phrases, variations_per_phrase) returns list[TextSample]
  • Ports _create_variation() and sanity_check() from the legacy 01_generate_phrases.py script; all random.* calls replaced with self.rng.*
  • TextSample constructed with seed=0, content_hash=sha256(content), content=surface_form, label=command

ml/pipeline/intent/vocab_computer.py

  • VocabComputer, VocabResult, PhonemeProvider, PhonemeNotFoundError
  • compute(manifest, output_dir) extracts words from TextSample.label (splitting on _), not content
  • Writes phoneme_list.txt and words_to_phonemes.json; no phoneme_trie.json
  • VocabResult.ctc_blank_idx = len(phoneme_list)

ml/pipeline/stages/conventions.py

  • All seven path-convention functions: manifest_path, split_manifest_path, sample_file_path, model_path, evaluation_predictions_path, evaluation_metrics_path, test_samples_path

ml/pipeline/stages/intent_01_generate_phrases.py

  • CLI: --input-phrases, --output-dir, --variations-per-phrase, --subsample-rate
  • Reads CSV, constructs PhraseVariator(random.Random(42)), applies subsample filter, writes Manifest[TextSample]

ml/pipeline/stages/intent_02_compute_vocab.py

  • CLI: --input-manifest-dir, --output-dir, --phoneme-dict
  • Includes _CmuPhonemeProvider for CMU Pronouncing Dictionary format
  • Reads manifest, constructs VocabComputer, calls compute()

ml/test/pipeline/intent/test_phrase_variator.py

  • Determinism tests (same seed → same output; different seeds → different output)
  • Output structure tests (labels match commands, seed=0, content_hash integrity, variant count)
  • Sanity-check tests (empty, too-short, no-letters, unrecognizable base phrase, too-long all rejected)
  • Snapshot determinism check against original VariationGenerator

ml/test/pipeline/intent/test_vocab_computer.py

  • Phoneme list coverage; words_to_phonemes mapping; ctc_blank_idx == len(phoneme_list)
  • File output correctness (phoneme_list.txt, words_to_phonemes.json)
  • Uses label, not content
  • TestConventions: all seven convention functions verified

Build & Test

  • .NET build: clean, 0 warnings, 0 errors
  • .NET unit tests: 367 passed (312 AdaptiveRemote.App.Tests + 55 AdaptiveRemote.Speech.Tests)
  • E2E failures are pre-existing infrastructure issues unrelated to this change:
    • Headless E2E: MessagePack.Annotations DLL not copied after git clean (fixed in this PR by adding direct package reference to AdaptiveRemote.App.csproj)
    • WPF E2E: AdaptiveRemote.runtimeconfig.json missing after git clean (pre-existing, not introduced here)

Design Notes

  • TextSample.id is content-addressable (= content_hash) per the existing sample.py implementation — the spec's mention of uuid4() is superseded by the actual dataclass
  • VocabComputer splits label words on _ (e.g. TV_ON["TV", "ON"])
  • No digit-to-word substitution; no phoneme_trie.json
  • Entry-points add ml/ to sys.path for direct invocation compatibility

@jodavis
jodavis changed the base branch from main to feature/ADR-191-oop-ml-pipeline June 11, 2026 01:30
@github-actions

github-actions Bot commented Jun 11, 2026

Copy link
Copy Markdown

Test Results

401 tests  ±0   401 ✅ ±0   3m 2s ⏱️ +39s
  5 suites ±0     0 💤 ±0 
  5 files   ±0     0 ❌ ±0 

Results for commit 90db3ad. ± Comparison against base commit 1dffa03.

♻️ This comment has been updated with latest results.

@jodavis-claude
jodavis-claude force-pushed the dev/claude/ADR-223-implement-modifierstage-abstract-base branch from 286b6b9 to 6f04420 Compare June 11, 2026 01:54

@jodavis jodavis left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This review is suggesting a pretty large design change, so we'll start with this. The PhraseVariator needs to use VariationGenerator in order to keep variations consistent, otherwise we will end up with large differences when parameters change.

Comment thread ml/pipeline/intent/phrase_variator.py Outdated
Comment thread ml/pipeline/intent/phrase_variator.py Outdated
Comment thread ml/pipeline/stages/intent_01_generate_phrases.py Outdated
Comment thread ml/pipeline/intent/phrase_variator.py Outdated
…riables

Moves variations_per_phrase, subsample_rate, and the five probability floats
out of source code and into a central params.yaml so they can be tracked and
overridden as DVC parameters.
…producible randomisation

Replace injectable rng: random.Random with vgen_factory: Callable[[int], VariationGenerator]
and five probability float constructor params. Remove module-level _*_CHANCE constants.

In generate(), use random.Random(42) to draw a phrase_seed per base phrase, then
random.Random(phrase_seed) to draw a variant_seed per variant, and pass each seed to
vgen_factory to get a VariationGenerator for that variant's _create_variation() call.

In _create_variation(), replace all self._rng.* calls with vgen.should_vary(),
vgen.choose(), and vgen.generate_int() using the stable variable name strings
specified in ADR-224.

Update tests to use the new constructor signature and a factory lambda.
…iables

Remove --variations-per-phrase and --subsample-rate from argparse. Load all five
probability floats, variations_per_phrase, and subsample_rate from ml/params.yaml
(resolved relative to __file__). Construct PhraseVariator with the factory lambda
and probability params from the YAML.
Comment thread ml/pipeline/stages/intent_01_generate_phrases.py
Comment thread ml/pipeline/stages/intent_01_generate_phrases.py Outdated
Comment thread ml/pipeline/stages/intent_01_generate_phrases.py Outdated
Comment thread ml/test/pipeline/intent/test_vocab_computer.py Outdated
… object, PhraseVariator params object, move TestConventions

- conventions.params_path(project_root) centralises the params.yaml path
- PipelineParams/GeneratePhraseParams dataclasses deserialise params.yaml into a typed object
- PhraseVariator.__init__ now takes GeneratePhraseParams instead of five individual floats
- TestConventions moved from test_vocab_computer.py to its own test_conventions.py file

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tall from it in CI

pyyaml was used in ml/pipeline/stages/params.py and ml/test/pipeline/stages/test_params.py
but was not listed as a dependency. The CI workflow only ran `pip install pytest`, so pyyaml
was never installed. Added ml/requirements.txt with pytest and pyyaml>=6.0, and updated the
python-tests job to install from that file instead.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@jodavis
jodavis enabled auto-merge (rebase) June 11, 2026 22:40
@jodavis
jodavis disabled auto-merge June 11, 2026 22:40
@jodavis
jodavis enabled auto-merge (squash) June 11, 2026 22:40
@jodavis
jodavis merged commit 7c1af06 into feature/ADR-191-oop-ml-pipeline Jun 11, 2026
3 checks passed
@jodavis
jodavis deleted the dev/claude/ADR-223-implement-modifierstage-abstract-base branch June 11, 2026 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants