Repository navigation
[BugFix] Validate --stage-overrides JSON shape with a clear, flag-named error - #4209
SuperMarioYL wants to merge 2 commits into
Conversation
--stage-overrides is documented as a JSON object mapping each stage_id to a
dict of per-stage overrides, but only its JSON syntax was validated. A
syntactically-valid but mis-shaped payload (a top-level list/scalar, or a
stage_id -> scalar mapping) crashed deep in config loading with a bare
AttributeError that named neither the flag nor the offending value. A
non-numeric stage id (e.g. {"abc": {...}}) was silently dropped, since the
downstream parser only matches stage_<digits>_* keys.
Validate the payload shape at the consume site in load_stage_configs_from_model
and raise a ValueError that names the --stage-overrides flag and the offending
value. Behavior is unchanged for well-formed input.
Signed-off-by: supermario_leo <leo.stack@outlook.com>
Reword 'mis-shaped' to 'malformed' in the _validate_stage_overrides docstring; the typos hook rejects 'mis' as a misspelling, failing the pre-commit CI check. No functional change. Signed-off-by: supermario_leo <leo.stack@outlook.com>
|
Gentle ping on this one 🙂 — it's a small, self-contained bugfix that validates the |
Omni ReviewBot: no human activity for 53 days@SuperMarioYL this pull request has had no human commit, comment or review since 2026-07-17. Per repository policy it may be closed if it stays inactive. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
|
Closing this one — it's superseded by #4281, which landed |
Purpose
--stage-overridesis documented as a JSON object mapping eachstage_idto a dict of per-stage overrides, e.g.'{"0": {"gpu_memory_utilization": 0.8}, "2": {"enforce_eager": true}}'(see the flag help inentrypoints/cli/serve.py). Today only the JSON syntax is validated (json.loadsinasync_omni_engine); the decoded object's shape is never checked before it is consumed inload_stage_configs_from_model:This blindly assumes a dict-of-dicts, with two bad outcomes for a syntactically-valid payload:
stage_id -> scalarmapping, raises a bareAttributeError: '<type>' object has no attribute 'items'deep in config loading — naming neither the--stage-overridesflag nor the offending value:--stage-overrides '[1,2]'→AttributeError: 'list' object has no attribute 'items'--stage-overrides '{"0": 5}'→AttributeError: 'int' object has no attribute 'items'--stage-overrides '{"abc": {"k": 1}}'produces a dead override keystage_abc_kthat the downstream parser_STAGE_OVERRIDE_PATTERN = re.compile(r"^stage_(\d+)_(.+)$")(config/stage_config.py) can never match, so the user's override vanishes with no error.This PR validates the payload shape at the single consume site (
load_stage_configs_from_model, reached by both the CLI and engine paths) and raises aValueErrorthat names the--stage-overridesflag and the offending value. Behavior is unchanged for well-formed input.Resolves the input-validation gap noted while triaging stage-override handling.
Test Plan
Added
TestValidateStageOverridesintests/entrypoints/test_utils.py(CPU-only, no model load — the validator runs before any config/model access):Covers:
None/empty no-op; well-formed string-keyed and int-keyed payloads pass; top-level list/scalar/string rejected; per-stage scalar/list/string value rejected; non-integer stage ids ("abc","stage0","0.5","-1",True) rejected; and an end-to-end assertion thatload_stage_configs_from_model(model, stage_overrides=[1, 2])now fails fast with a clearValueErrorinstead of the previous crypticAttributeError.Test Result
All new cases pass; the end-to-end case is red on
main(raisesAttributeError) and green here (raisesValueErrornaming--stage-overrides).ruff checkandruff formatare clean on both changed files.