feat(cli): support Python workflow args#799
Conversation
Forward arguments after the CLI separator to params-aware local Python config modules while preserving legacy zero-argument workflows. Reject script arguments for serialized and remote configs, and document the public params protocol. Fixes #507 Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
|
/nvskills-ci |
|
Fern preview: https://nvidia-preview-pr-799.docs.buildwithfern.com/nemo/datadesigner
|
Greptile SummaryThis PR introduces
|
| Filename | Overview |
|---|---|
| packages/data-designer-config/src/data_designer/config/script_params.py | New frozen dataclass DataDesignerScriptParams with an immutable argv tuple; minimal and correct. |
| packages/data-designer/src/data_designer/cli/utils/config_loader.py | Core loader change: inspects the Python workflow signature (0-arg, positional, or keyword-only), rejects argv for non-Python sources, and dispatches correctly; all branches are covered by tests. |
| packages/data-designer/src/data_designer/cli/controllers/generation_controller.py | _load_config always wraps script_args into DataDesignerScriptParams (empty when None) and passes it through; the empty-argv path preserves backward compatibility with YAML/JSON configs. |
| packages/data-designer/src/data_designer/cli/main.py | Fixes _is_version_request to ignore args after the -- separator; correctly rebinds the local variable without mutating sys.argv. |
| packages/data-designer/tests/cli/utils/test_config_loader.py | Comprehensive new tests covering: param forwarding, keyword-only parameters, legacy signature rejection, unsupported signatures (*args/**kwargs/multi-param), serialized config rejection, remote URL rejection, and sensitive-value leak prevention in error messages. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User
participant CLI as CLI (create/preview/validate/check-models)
participant Controller as GenerationController
participant Loader as load_config_builder
participant Module as Python Workflow Module
User->>CLI: data-designer create workflow.py -- --seed-path seeds.parquet
CLI->>Controller: run_create(config_source, script_args)
Controller->>Controller: "DataDesignerScriptParams(argv=(...))"
Controller->>Loader: load_config_builder(config_source, script_params)
Loader->>Loader: Check: is_remote or non-.py? reject if argv non-empty
Loader->>Loader: inspect.signature(load_config_builder)
alt 0 parameters
Loader->>Module: func()
else 1 positional parameter
Loader->>Module: func(script_params)
else 1 keyword-only parameter
Loader->>Module: "func(param_name=script_params)"
else unsupported signature
Loader-->>Controller: ConfigLoadError
end
Module-->>Loader: DataDesignerConfigBuilder
Loader-->>Controller: DataDesignerConfigBuilder
Controller->>Controller: Run generation workflow
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant User
participant CLI as CLI (create/preview/validate/check-models)
participant Controller as GenerationController
participant Loader as load_config_builder
participant Module as Python Workflow Module
User->>CLI: data-designer create workflow.py -- --seed-path seeds.parquet
CLI->>Controller: run_create(config_source, script_args)
Controller->>Controller: "DataDesignerScriptParams(argv=(...))"
Controller->>Loader: load_config_builder(config_source, script_params)
Loader->>Loader: Check: is_remote or non-.py? reject if argv non-empty
Loader->>Loader: inspect.signature(load_config_builder)
alt 0 parameters
Loader->>Module: func()
else 1 positional parameter
Loader->>Module: func(script_params)
else 1 keyword-only parameter
Loader->>Module: "func(param_name=script_params)"
else unsupported signature
Loader-->>Controller: ConfigLoadError
end
Module-->>Loader: DataDesignerConfigBuilder
Loader-->>Controller: DataDesignerConfigBuilder
Controller->>Controller: Run generation workflow
Reviews (9): Last reviewed commit: "Merge branch 'main' into codex/issue-507" | Re-trigger Greptile
Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
|
/nvskills-ci |
1 similar comment
|
/nvskills-ci |
Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
nabinchha
left a comment
There was a problem hiding this comment.
Nice work on this one, @eric-tramel — the layering and test coverage here are really clean.
Summary
This PR lets local Python config modules accept workflow-specific CLI arguments passed after --, surfaced through a new lazily-exported DataDesignerScriptParams. The implementation matches the stated intent: args are threaded through create/preview/validate → controller → loader, the loader validates the workflow signature, and non-Python/remote sources reject forwarded args without echoing their (potentially sensitive) values. The signature-handling and rejection logic is well thought out and thoroughly tested.
Findings
Warnings — Worth addressing
packages/data-designer/src/data_designer/cli/controllers/generation_controller.py:119 — check-models can't parameterize a workflow
- What:
create,preview, andvalidateall gainedscript_args, butrun_check_modelsstill callsself._load_config(config_source)with no args. A workflow whoseload_config_builder(params)requires argv (e.g. an argparserequired=True --seed-path) will fail undercheck-models, because it always receives an emptyDataDesignerScriptParams(). - Why:
check-modelsis the natural pre-flight companion tovalidate, so a user who parameterizes their workflow will reasonably expectcheck-models workflow.py -- --seed-path seeds.parquetto work too. Today it either errors (required arg missing) or silently runs against defaults, which is surprising given the other three commands support it. - Suggestion: Either extend
check-models(command +run_check_models) with the samescript_argsplumbing, or, if excluding it is intentional, add a one-line note in the docs/PR description so the asymmetry is a documented decision rather than a gap.
Suggestions — Take it or leave it
packages/data-designer/src/data_designer/cli/utils/config_loader.py:191-198 — Keyword-only param name is hard-coded to params
- What: A single positional-or-keyword parameter is accepted under any name, but a keyword-only parameter is only accepted when it is literally named
params(def load_config_builder(*, config)is rejected,def load_config_builder(config)is fine). - Why: The asymmetry is a little surprising — the name only matters in the keyword-only branch. A user who copies the documented positional pattern is fine, but anyone experimenting with keyword-only signatures could hit a confusing rejection.
- Suggestion: Since we already know it's the sole parameter, we can bind by name generically for the keyword-only case (
func(**{parameter.name: params})) and drop the== "params"constraint — or, if the constraint is deliberate, mention the expected name insignature_error.
packages/data-designer/src/data_designer/cli/main.py:52 — Dense one-liner in _is_version_request
- What:
return "--version" in args[: args.index("--")] if "--" in args else "--version" in argspacks the separator-aware logic into a single nested ternary. - Why: It's correct (and nicely tested), but takes a second read to parse the slice-before-
--intent. - Suggestion: Optional readability tweak, e.g.
def _is_version_request(args: list[str]) -> bool:
if "--" in args:
args = args[: args.index("--")]
return "--version" in argspackages/data-designer/src/data_designer/cli/commands/create.py:74 — Variadic script_args also captures bare positionals without --
- What:
script_argsis a variadictyper.Argument, sodata-designer create a.py b.pysilently bindsb.pyas a forwarded arg, even though the help text says "after--". (Unknown options before--are correctly rejected — nice — but stray positionals aren't.) - Why: Minor foot-gun: a typo'd second path produces a downstream "does not accept script arguments" error rather than an "unexpected argument" one. Not a regression in the common path, just slightly less obvious.
- Suggestion: Leave as-is if you consider it acceptable; otherwise a short note in the arg help ("captured after
--") or a guard could make the intent clearer.
What Looks Good
- Signature validation is genuinely careful — zero-arg, positional, keyword-only, and the
*args/**kwargs/multi-arg rejection paths are all handled and parametrized in tests. - Security-conscious rejection — refusing forwarded args for YAML/JSON/remote sources without printing their values, plus the explicit
"sensitive-value" not in str(exc_info.value)assertion, is a thoughtful touch. - Clean layering —
DataDesignerScriptParamslands inconfigas a frozen/slots dataclass and is lazily exported, respecting the interface → engine → config direction, and the Fern docs were updated in the same PR.
Verdict
Needs changes — only the check-models asymmetry is worth resolving (fix or document) before merge; everything else is optional polish. Linter (ruff check + ruff format --check) passes on all changed files.
This review was generated by an AI assistant.
Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com> # Conflicts: # packages/data-designer/src/data_designer/cli/controllers/generation_controller.py # packages/data-designer/src/data_designer/cli/utils/config_loader.py # packages/data-designer/tests/cli/controllers/test_generation_controller.py # packages/data-designer/tests/cli/test_main.py
Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
- Forward workflow arguments through check-models. - Bind sole keyword-only loader parameters by their declared name. - Clarify separator-aware version detection. Signed-off-by: Eric W. Tramel <eric.tramel@gmail.com>
|
@nabinchha Thanks for the thorough review. I addressed the feedback in
Validation is green: 813 CLI tests passed with 1 skip, Ruff passed, and Fern validation reported 0 errors. I also merged the latest |
nabinchha
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround, @eric-tramel — the new fix(cli): complete workflow argument handling commit resolves all three actionable findings cleanly, with matching test and doc updates.
Addressed
check-modelsnow forwards workflow args —check_models_commandgainedscript_args,run_check_modelsthreads it through_load_config, docs list the newcheck-models workflow.py -- --seed-path ...example, and both the command and controller tests are updated. This closes the asymmetry that was the reason for requesting changes.- Keyword-only param no longer name-locked — dropped the
parameter.name == "params"constraint and binds generically viafunc(**{parameters[0].name: params}); the test now usesdef load_config_builder(*, config)and"*, other"was correctly removed from the unsupported-signature cases. _is_version_requestreadability — rewritten to the early-return form; clear and still separator-aware.
Remaining (optional, non-blocking)
- The variadic
script_argsstill captures bare positionals without--(e.g.create a.py b.py). Fine to leave as-is — noted only for awareness.
Verdict
Ship it (with nits) — all requested changes are in, and ruff check + ruff format --check pass on the touched files. Approving.
This review was generated by an AI assistant.
📋 Summary
Support workflow-specific arguments for local Python config modules used by
data-designer create,preview,validate, andcheck-models. Arguments after--are exposed through a publicDataDesignerScriptParamsobject while existing zero-argument workflows remain compatible.The loader rejects forwarded arguments for serialized and remote configs, validates Python workflow signatures, and keeps unknown Data Designer options before
--strict.🔗 Related Issue
Fixes #507
🔄 Changes
DataDesignerScriptParams(argv=...)config API.💻 Usage
🧪 Testing
The full repository suite previously passed with 3,805 tests and 1 skip. After merging the latest
mainand addressing review feedback, the full CLI suite passed with 813 tests and 1 skip; Ruff passed on all changed Python files; Fern validation completed with zero errors.✅ Checklist
Description updated with AI