Skip to content

fix(cli): honour explicit CLI flags whose value equals the parser default - #1002

Merged
Xiaoming-AMD merged 6 commits into
AMD-AGI:mainfrom
SumonAMD:fix/cli/config-cli-override-precedence
Sep 1, 2026
Merged

Xiaoming-AMD merged 6 commits into
AMD-AGI:mainfrom
SumonAMD:fix/cli/config-cli-override-precedence

Conversation

@SumonAMD

Copy link
Copy Markdown
Contributor

Summary

Fixes #1001.

primus data diffusion-encoded documents and prints the guarantee that CLI flags override the
YAML config, but the merge kept no record of which flags were actually typed. It inferred that by
comparing each parsed value against a hardcoded default table, and argparse fills untyped options
with their defaults, so "value equals default" and "user did not type this flag" were
indistinguishable. Two failures followed from that one comparison:

  • A typed flag was silently discarded when its value equalled the parser default, and the YAML
    value was used instead. No error, no warning. Affects hf_split, shard_size, model_path,
    precision, device, batch_size, t5_max_length, image_size, max_size and
    vae_latent_mode.
  • A setting present in neither the YAML nor the typed flags was dropped from the namespace
    rather than falling back to its default, because the merge rebuilt the namespace from the
    filtered dict. Any config omitting the optional image: section therefore reached
    _prepare_encoded and raised AttributeError: 'Namespace' object has no attribute 'variable_size' — after validation had already declared the config valid.

The first is the more serious of the two: diffusion-encoded bakes these settings into an Energon
WebDataset that is written once and trained on many times, so a dropped --t5-max-length or
--image-size surfaces much later as unexplained training-quality loss, with nothing pointing back
at the preprocessing command. Full analysis and reproduction in #1001.

Fix

Both symptoms share one cause, so primus/cli/subcommands/data.py gets one fix in two parts:

  1. The encoded parser registers its options with argparse.SUPPRESS as the default, via a new
    opt-in defaults_from_config parameter on _add_common_args plus the encoder-specific options.
    The parsed namespace then contains only what the user typed, and presence means "explicit" — the
    ambiguity is removed rather than worked around.
  2. _load_config_with_cli_overrides merges three layers in priority order: defaults, then YAML,
    then explicit CLI. The defaults layer always contributes every configurable key, which makes the
    missing-attribute failure impossible by construction instead of patching each read site with
    another getattr.

The defaults table that already existed in this file becomes the actual source of values, rather
than only the right-hand side of a comparison.

diffusion-raw and diffusion-ingest are unaffected: defaults_from_config is opt-in and only the
encoded parser passes it, so they keep their argparse defaults.

Behaviour

With a config setting batch_size: 64, precision: fp32, t5_max_length: 256:

Command Before After
--config c.yaml --batch-size 8 64, flag ignored 8
--config c.yaml --precision bf16 fp32, flag ignored bf16
--config c.yaml --batch-size 16 16 16, unchanged
--config c.yaml 64 64, unchanged
config with no image: section AttributeError at data.py:489 defaults applied, image_size=1024

One note for downstream users: a script that types a flag at its default value while loading a
config that sets the same key differently will now get the CLI value where it previously got the
YAML value. That is the point of the fix, but it is a visible change for anyone who had unknowingly
come to depend on the old behaviour.

Tests

tests/unit_tests/cli/test_data_config.py

TestLoadConfigWithCliOverrides previously hand-built the namespace with every key pre-populated,
so the fixture inherited the same ambiguity as the bug: there was no way to express "flag omitted"
versus "flag typed with a value equal to the default", and the broken case was exactly the one that
could not be written down. Since the fix changes the calling convention, these tests now drive the
real parser through register_subcommand and parse_args.

Test Covers
test_explicit_cli_wins_when_value_equals_default types four flags at their default values, asserts each beats the YAML
test_omitted_yaml_section_falls_back_to_defaults config with no image: section, asserts the four image keys resolve to defaults
test_every_configurable_key_present no key from the defaults table is absent from the merged namespace
test_cli_overrides_yaml, test_yaml_used_when_flag_not_passed, test_no_config_applies_defaults the pre-existing contracts, retained
TestParserDefaults drift guard: parses the raw parser, which keeps real defaults, and compares against the defaults table so the two copies cannot silently diverge

The first three fail against the pre-fix data.py and pass with the fix. Verified by running the
new test file with b8d3cc5's data.py overlaid: 3 failed / 16 passed before, 19 passed after.

Verification

Base b8d3cc5, container rocm/primus:v26.5 (Python 3.12.3, torch 2.12.0+rocm7.15.0a), host
8 × MI350X.

  • 118 passed across tests/unit_tests/cli and
    tests/unit_tests/backends/megatron/diffusion/data/preprocessing.
  • pre-commit run on both changed files: trailing-whitespace, end-of-file-fixer,
    check-added-large-files, check-merge-conflict, isort, autoflake, black and thirdparty-lock all
    pass. check-yaml and shellcheck skip, no matching files.
  • End to end with the real CLI and the image:-less config from the issue: before, AttributeError
    at data.py:489; after, config handling completes and execution reaches encoder loading, failing
    only on absent local model weights, which is an unrelated later stage.

`primus data diffusion-encoded` inferred which CLI flags were explicitly
provided by comparing each parsed value against a default table, so a flag
typed with a value equal to its default was indistinguishable from an omitted
flag and silently lost to the YAML config. The same comparison dropped keys
supplied by neither the YAML nor the CLI from the merged namespace, so
_prepare_encoded raised AttributeError on args.variable_size whenever the
optional image: section was absent.

Register the encoded parser's options with argparse.SUPPRESS so the namespace
holds only what the user typed, and merge defaults -> YAML -> CLI in priority
order. The defaults layer always supplies every configurable key, which makes
the missing-attribute failure impossible by construction.

diffusion-raw and diffusion-ingest keep their argparse defaults and are
unaffected.

Fixes AMD-AGI#1001
@Xiaoming-AMD
Xiaoming-AMD merged commit da87fdf into AMD-AGI:main Sep 1, 2026
6 of 7 checks passed
Xiaoming-AMD pushed a commit that referenced this pull request Sep 1, 2026
…1065)

## Summary

- reuse the Primus YAML loader and deep-merge semantics for diffusion
preprocessing config resolution
- preserve the precedence contract from #1002: defaults -> YAML ->
explicitly supplied CLI flags
- move Megatron diffusion raw/encoded/ingest implementation under the
Megatron backend
- keep `primus data diffusion-raw`, `diffusion-encoded`, and
`diffusion-ingest` compatible through a thin CLI facade
- avoid introducing a generic data-backend registry before a second
backend needs this capability

## Relationship to #1002

#1002 has been merged into `main` as `da87fdf5` and fixes #1001.

This branch merged the latest `main` in `eeb4e00e` to resolve the
overlap without rebasing or rewriting any existing commit. The
historical `f44a5bde` commit carries the same #1002 patch and
contributes no unique diff relative to the current base.

The effective review scope of this PR is the following two refactor
commits:

1. `bf720f15 refactor(data): centralize preprocessing config resolution`
2. `13766887 refactor(cli): move data commands into megatron backend`

## Validation

- pre-commit: passed
- relevant CLI, Megatron diffusion config, core config, and YAML tests:
147 passed, 3 subtests passed
- diff against the latest `main` contains only the expected data and CLI
refactor files
- real `quickstart_pokemon.yaml` resolution was validated
- command registration does not eagerly import raw or encoded pipeline
implementations

---------

Co-authored-by: Sumon Singh <ssingh@amd.com>
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.

primus data diffusion-encoded: CLI flags are silently ignored, and config keys silently vanish, whenever a value equals its parser default

2 participants