[feat] [3/n] Improve API: extend support to cli#1226
Conversation
|
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🔴 PR merge requirementsWaiting for:
This rule is failing.
|
There was a problem hiding this comment.
Code Review
This pull request refactors the FastVideo CLI to be "config-first," moving from a flat argument structure to a nested YAML/JSON configuration with dotted-path overrides. This change simplifies CLI usage by consolidating parameters into structured config files and introduces a new request_metadata module to track explicitly provided fields. The old CLI scripts have been replaced with new YAML config files and a generic run.sh wrapper. Documentation has been updated to reflect these changes. Review comments highlight a potential architectural concern regarding the global side effects of __setattr__ patching for request tracking and suggest improving error messages for users transitioning from the old flat config format.
|
|
||
| original_setattr(self, name, value) | ||
|
|
||
| type.__setattr__(config_type, "__setattr__", _tracking_setattr) |
There was a problem hiding this comment.
Patching __setattr__ on the class level has global side effects for all instances of these dataclasses within the process. While this is necessary for the current tracking implementation, it should be noted that any part of the codebase using these types will now incur the overhead of the tracking logic, even if they are not part of a tracked request. Consider if a more localized approach (e.g., a wrapper or a specific subclass) could achieve the same goal without global monkey-patching.
| if not isinstance(raw.get("generator"), Mapping): | ||
| raise ValueError("Inference config must use the nested schema with a top-level " | ||
| "'generator' mapping") |
There was a problem hiding this comment.
The check for the generator key is used to distinguish between the new nested schema and the legacy flat schema. It might be beneficial to provide a more descriptive error message or a migration hint if a flat config is detected, as users transitioning from older versions might be confused by the requirement for a top-level mapping.
|
/merge |
Convert test_vmoba_inference.py and test_bsa_inference.py from flat CLI args (--model-path, --prompt, etc.) to nested JSON config files matching the new fastvideo generate --config interface. Update stale CLI examples in architecture.md and bench_serving.py. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Motivation
The previous CLI surface mirrored every
SamplingParam/FastVideoArgsfieldas a flat argparse flag (~80+ flags on
generatealone). This made it hard tokeep CLI parity with the config schema, created brittle translation code, and
required per-model shell scripts full of duplicate flag lists. The nested config
approach lets users maintain one YAML per model and override individual values
on the command line without needing to know the full flag set.
Purpose
Replaces the flat argparse-flag inference CLI with a config-driven model.
fastvideo generateandfastvideo servenow accept only--config <path>plus optional dotted overrides (e.g.
--request.sampling.seed 42).Config-only CLI: Both subcommands take a single
--configpointing to anested YAML/JSON file with
generator:andrequest:(orserver:)top-level sections. All other CLI input uses dotted paths that map directly
into the config tree.
Request metadata tracking: New
request_metadata.pymodule tracks whichGenerationRequestfields were explicitly provided by the user vs filled byschema defaults. Uses a 3-way merge (raw config + baseline snapshot + current
state) with
__setattr__-based dirty-path recording so that post-loadmutations (e.g.
config.request.sampling.seed = 7) are correctly reflectedwhen translating to the legacy
SamplingParam.Typed config builders: New
inference_config.pyprovidesbuild_generate_run_config()andbuild_serve_config()with validation(prompt source checks, num_gpus > 0, dotted-override prefix enforcement).
Shell scripts → YAML configs: 12 per-model shell scripts replaced with
15 declarative YAML configs plus a single
run.shlauncher.FlexibleArgumentParser: Extended with
parse_known_argsoverride anddeferred config loading so dotted overrides pass through to the config
builders instead of being consumed by argparse.
Dashed-key normalization:
--request.output.output-pathis automaticallynormalized to
request.output.output_pathin the override parser.Changes
Test Plan
# Commands you ranTest Results
Test output
Checklist
pre-commit run --all-filesand fixed all issuesFor model/pipeline changes, also check: