feat: add and harden Generator FPM deployment target#1350
feat: add and harden Generator FPM deployment target#1350liyuanzhe1991 wants to merge 2 commits into
Conversation
Signed-off-by: YZLi <yuanli@nvidia.com>
Signed-off-by: YZLi <yuanli@nvidia.com>
WalkthroughAdds an ChangesFPM deployment target
Estimated code review effort: 5 (Critical) | ~120 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/cli_user_guide.md`:
- Line 18: Update the Deployment Target Selection section in cli_user_guide.md
so it exposes the deployment-target-selection anchor, preferably by converting
the existing bold section label into a Markdown heading while preserving its
text and content.
In `@src/aiconfigurator/generator/builders/fpm_builder.py`:
- Around line 386-615: Replace the inline script assembly in _render_run_script
with a Jinja template for the FPM run.sh artifact, passing all required prepared
values and generated command data from the builder. Ensure the template emits
#!/bin/bash and set -e while preserving the existing runtime behavior, then
update the associated golden snapshots and template coverage to match the
generated output.
- Around line 493-502: Validate that the JSON loaded in the terminal-result
reader is an object before accessing it with value.get(). Treat scalars and
arrays as terminal invalid results through invalid(path, ...) rather than
allowing an AttributeError or retrying; preserve the existing invalid
diagnostics for object results. Add a regression test covering a non-object JSON
result.
In `@src/aiconfigurator/generator/config/deployment_config.yaml`:
- Around line 132-135: Extend validation for the FPM passthrough fields
K8sConfig.fpm_shared_memory_size and K8sConfig.fpm_resource_labels: validate
shared memory values as Kubernetes resource quantities before assigning
emptyDir.sizeLimit, and validate label keys and values against Kubernetes label
syntax rather than only their Python types. Add negative tests covering
malformed quantities and invalid label keys/values.
In `@src/aiconfigurator/generator/main.py`:
- Around line 66-71: Add a shared backend/deployment-target compatibility
validation immediately after CLI argument parsing, rejecting any fpm target
whose backend is not vllm before dispatch reaches build_fpm_artifacts(). Reuse
the parsed backend and deployment-target values and provide a clear CLI error
for invalid combinations, while preserving all currently supported combinations.
In `@tests/unit/cli/test_cli_workflow.py`:
- Around line 263-281: Extend test_cli_fpm_rejects_thorough_sweep_before_spica
to cover both thorough_sweep and thorough_config inputs, preferably by
parameterizing the relevant fixture arguments and expected rejection message.
Preserve the existing SystemExit assertion and verify run_spica_thorough_default
and _execute_tasks are not called for either FPM rejection path.
In `@tests/unit/generator/test_fpm_artifacts.py`:
- Around line 430-437: Replace the direct subprocess.run usage in the test
runtime call sites with a shared subprocess runner that handles timeouts by
sending TERM first, allowing the script trap to clean up engine descendants,
then escalating to KILL if needed. Ensure every invocation of the generated Bash
scripts uses this runner rather than applying a local change only to the shown
completed call.
- Around line 572-621: Extend
test_fpm_run_script_rejects_mismatched_result_identity to cover status !=
"complete" and schema-v1 invalid coverage/result-count combinations. Use the
existing status and valid helpers to construct cases that reach malformed
coverage and result-count validation, and assert each case exits with code 1 and
reports its expected error.
- Around line 1255-1262: Add an end-to-end decode-mode test alongside
test_fpm_accepts_phase_specific_benchmark_mode that supplies a valid decode
result and executes the generated run.sh through result validation. Ensure the
test verifies successful validation for decode, not merely the emitted
--benchmark-mode argument, while preserving the existing prefill coverage.
- Line 25: Replace the unit marker on the tests in this file with the
repository’s established integration or snapshot classification, and move full
artifact-rendering and generated Bash runtime cases into the corresponding
integration/snapshot test locations. Keep only isolated generator-validation
tests here, following existing marker and CI registration conventions rather
than applying a marker-only change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 709a1953-1ad4-423b-b848-1cab9de6539c
📒 Files selected for processing (15)
README.mddocs/cli_user_guide.mddocs/dynamo_deployment_guide.mddocs/generator_overview.mdsrc/aiconfigurator/cli/main.pysrc/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/generator/api.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/builders/fpm_builder.pysrc/aiconfigurator/generator/builders/k8s_builder.pysrc/aiconfigurator/generator/config/deployment_config.yamlsrc/aiconfigurator/generator/main.pysrc/aiconfigurator/generator/rendering/engine.pytests/unit/cli/test_cli_workflow.pytests/unit/generator/test_fpm_artifacts.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (23)
src/aiconfigurator/generator/config/deployment_config.yaml
📄 CodeRabbit inference engine (.claude/rules/generator/config_schema.md)
src/aiconfigurator/generator/config/deployment_config.yaml: Define deployment configuration entries with a section-qualifiedkey,requiredstatus, default, optionalbackend_defaults, and optional backend restrictions.
Place new parameters in the correct section, determine whether they are required or optional, define static or computed defaults, and specify backend support.
For computed defaults, verify referenced variables are available during default evaluation and handle missing orNonevalues safely.
Quote string Jinja2 defaults with double quotes inside single quotes, such asdefault: '"my-string"'.
Guard default expressions against missing values, for example usemodel_path.split("/")[-1] if model_path else "".
Avoid circular default references between parameters because the schema loader may produceNonefor both; verify the dependency chain is acyclic.Define new parameters, defaults, and backend support in
deployment_config.yaml. Update parameter keys when renaming parameters.Verify parameter default expressions and applicable
backend_defaultswhen a value is missing orNone.For new user-facing parameters, add a schema entry with section, default, required status, and backend-specific defaults where behavior differs; verify default container-image expressions and version-tag resolution.
Add new input parameters and their defaults to the deployment configuration schema.
Files:
src/aiconfigurator/generator/config/deployment_config.yaml
src/aiconfigurator/generator/**
📄 CodeRabbit inference engine (.claude/rules/generator/config_schema.md)
src/aiconfigurator/generator/**: Update artifact templates when a parameter requires special handling in generated outputs.
Validate configuration changes by running the generator with the parameter set and omitted, running the generator validator against backend API schemas, and checking--generator-helpoutput.When modifying generator code, review the cross-module impact map for affected modules outside
src/aiconfigurator/generator/.
src/aiconfigurator/generator/**: For generator bugs, trace backwards from the generated output through template rendering, rule evaluation, parameter mapping, default application, input parsing, and schema loading.
Apply fixes at the stage where the defect originates rather than patching templates for rule or data-mapping bugs.
When fixing a generator issue, check all backends and all version templates for the same problem pattern.
After a generator fix, reproduce the original failure, run the generator validator, check other backends and versions, and add a regression test.
src/aiconfigurator/generator/**: TRT-LLM: Placebuild_configaccording to the selected version: nested before 1.2.0rc5 and top-level from 1.2.0rc5 onward.
TRT-LLM: Keep template variables as top-level keys becauseengine.pyflattens the context.
TRT-LLM: Emitcache_transceiver_configaccording to the applicable version and interface.
TRT-LLM: Setcache_transceiver_config.backendtoDEFAULT.
TRT-LLM: Put engine parameters in--override-engine-argsJSON rather than unsupported direct CLI flags.
Ensuremax_num_tokens % tokens_per_block == 0.
Ensurecache_transceiver_max_tokens_in_bufferis aligned to the block size.
Setdisable_overlap_scheduler=truefor prefill andfalsefor decode/aggregate modes.
For MoE models, setTP = moe_tp * moe_ep.
Map KV cache dtypefloat16orbfloat16toautofor TRT-LLM.
For SGLang, do not emit--moe-dense-tp-sizeunless its value is 1 or absent.
Use KV transfer backendnixlby default for SGL...
Files:
src/aiconfigurator/generator/config/deployment_config.yamlsrc/aiconfigurator/generator/main.pysrc/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/builders/k8s_builder.pysrc/aiconfigurator/generator/api.pysrc/aiconfigurator/generator/rendering/engine.pysrc/aiconfigurator/generator/builders/fpm_builder.py
⚙️ CodeRabbit configuration file
src/aiconfigurator/generator/**: - Enforce the generator development rules from.claude/rules/generator-development.md.
- Trace changes across the pipeline: input parsing, defaults, rule evaluation, parameter mapping, template rendering, and emitted artifacts.
- For
.j2templates and.rulefiles, verify# Guard:and# Why:comments are preserved and followed. Flag removals or bypasses unless the PR explicitly justifies them.- Check cross-backend consistency across TRT-LLM, vLLM, and SGLang when a backend-specific rule, template, parameter, or versioned artifact changes.
- For new user-facing generator parameters, require matching CLI, SDK/profiler bridge, generator validator, docs, and test coverage updates where applicable.
- For versioned template changes, verify the affected backend versions against
src/aiconfigurator/generator/config/backend_version_matrix.yaml.
Files:
src/aiconfigurator/generator/config/deployment_config.yamlsrc/aiconfigurator/generator/main.pysrc/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/builders/k8s_builder.pysrc/aiconfigurator/generator/api.pysrc/aiconfigurator/generator/rendering/engine.pysrc/aiconfigurator/generator/builders/fpm_builder.py
src/aiconfigurator/generator/config/**/*.yaml
📄 CodeRabbit inference engine (.claude/rules/generator/cross_module_impact.md)
When modifying template output, verify generated Kubernetes manifests and engine configuration files against their required schemas.
Files:
src/aiconfigurator/generator/config/deployment_config.yaml
src/aiconfigurator/generator/config/*.{j2,yaml}
📄 CodeRabbit inference engine (.claude/rules/generator/new_backend_version.md)
Create version-specific templates only when the backend CLI interface, engine configuration format, or startup command changes; do not create templates for no-op version bumps.
Files:
src/aiconfigurator/generator/config/deployment_config.yaml
src/aiconfigurator/generator/**/*.{py,jinja,j2,yaml,yml,json}
📄 CodeRabbit inference engine (.claude/rules/generator/testing.md)
For template, rule, or schema changes that affect generated output, add integration tests and update golden snapshots.
Files:
src/aiconfigurator/generator/config/deployment_config.yamlsrc/aiconfigurator/generator/main.pysrc/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/builders/k8s_builder.pysrc/aiconfigurator/generator/api.pysrc/aiconfigurator/generator/rendering/engine.pysrc/aiconfigurator/generator/builders/fpm_builder.py
src/aiconfigurator/generator/**/*
📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)
src/aiconfigurator/generator/**/*: When editing or reviewingsrc/aiconfigurator/generator/**, load and apply the generator rules, including.claude/rules/generator-development.md.
Do not drift into deployment-config topics unless the task actually targetssrc/aiconfigurator/generator/.
Files:
src/aiconfigurator/generator/config/deployment_config.yamlsrc/aiconfigurator/generator/main.pysrc/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/builders/k8s_builder.pysrc/aiconfigurator/generator/api.pysrc/aiconfigurator/generator/rendering/engine.pysrc/aiconfigurator/generator/builders/fpm_builder.py
**/*
⚙️ CodeRabbit configuration file
**/*: - Prefer applicable inline comments. When the correct fix is clear, small, and limited to the commented diff hunk, include it as a GitHub Suggested Change so the author can apply it with one click.
- Do not use a suggested change when the fix requires broader design choices, multiple files, generated artifacts, unavailable context, or validation that cannot be inferred from the diff.
- If a comment is not directly applicable, state the smallest concrete next step and why a one-click suggestion is not safe.
Files:
src/aiconfigurator/generator/config/deployment_config.yamlsrc/aiconfigurator/generator/main.pysrc/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/builders/k8s_builder.pyREADME.mdsrc/aiconfigurator/generator/api.pysrc/aiconfigurator/generator/rendering/engine.pydocs/dynamo_deployment_guide.mdsrc/aiconfigurator/cli/main.pydocs/cli_user_guide.mdtests/unit/cli/test_cli_workflow.pydocs/generator_overview.mdtests/unit/generator/test_fpm_artifacts.pysrc/aiconfigurator/generator/builders/fpm_builder.py
src/aiconfigurator/generator/**/*.py
📄 CodeRabbit inference engine (.claude/rules/generator/new_backend_version.md)
Do not hardcode backend version checks in the rendering engine; use version-template naming and fallback logic instead of conditions such as
if version >= ...in Python.
src/aiconfigurator/generator/**/*.py: For pure logic changes involving expressions or calculations, add unit-test coverage.
For new parameters or flags, add validator-test coverage.
Files:
src/aiconfigurator/generator/main.pysrc/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/builders/k8s_builder.pysrc/aiconfigurator/generator/api.pysrc/aiconfigurator/generator/rendering/engine.pysrc/aiconfigurator/generator/builders/fpm_builder.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the project’s
uv-managed environment and run linting withruff check .andruff format --check ..
Files:
src/aiconfigurator/generator/main.pysrc/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/builders/k8s_builder.pysrc/aiconfigurator/generator/api.pysrc/aiconfigurator/generator/rendering/engine.pysrc/aiconfigurator/cli/main.pytests/unit/cli/test_cli_workflow.pytests/unit/generator/test_fpm_artifacts.pysrc/aiconfigurator/generator/builders/fpm_builder.py
src/aiconfigurator/generator/aggregators.py
📄 CodeRabbit inference engine (.claude/rules/generator/cross_module_impact.md)
Update aggregators when renamed parameters are aggregated, and add backward-compatible aliases here when an old parameter name was user-facing.
Files:
src/aiconfigurator/generator/aggregators.py
src/aiconfigurator/generator/builders/k8s_builder.py
📄 CodeRabbit inference engine (.claude/rules/generator/new_backend_version.md)
Kubernetes deployment output is produced by the typed builder, not a template; modify
k8s_builder.pywhen Kubernetes output must change.
Files:
src/aiconfigurator/generator/builders/k8s_builder.py
src/aiconfigurator/generator/api.py
📄 CodeRabbit inference engine (.claude/rules/generator/cross_module_impact.md)
Add backward-compatible parameter aliases in
api.pywhen an old user-facing parameter name is renamed.When CLI overrides do not take effect, inspect
parse_cli_params()and verify dotted-path resolution and parsed input values.
Files:
src/aiconfigurator/generator/api.py
src/aiconfigurator/generator/rendering/engine.py
📄 CodeRabbit inference engine (.claude/rules/generator/debugging.md)
Trace template variables through
make_worker_context()to verify how values enter the rendering context.Inspect and update
make_worker_context()when template variables are injected or context shape changes.
Files:
src/aiconfigurator/generator/rendering/engine.py
docs/**
⚙️ CodeRabbit configuration file
docs/**: - Check that docs match changed CLI, SDK, generator, backend, and support-matrix behavior.
- Flag docs that describe unsupported runtimes, stale command names, or behavior not covered by tests or support-matrix evidence.
Files:
docs/dynamo_deployment_guide.mddocs/cli_user_guide.mddocs/generator_overview.md
src/aiconfigurator/cli/main.py
📄 CodeRabbit inference engine (.claude/rules/generator/cross_module_impact.md)
When adding a generator parameter, expose it through the CLI, using
--generator-setwhen applicable.
Files:
src/aiconfigurator/cli/main.py
src/aiconfigurator/cli/**
⚙️ CodeRabbit configuration file
src/aiconfigurator/cli/**: - Check that CLI argument changes preserve backward compatibility, validation behavior, defaults, and plain-output expectations.
- For new or changed user-facing options, verify docs updates and generator/SDK wiring.
- Watch for non-TTY assumptions in tests or output formatting.
Files:
src/aiconfigurator/cli/main.py
docs/cli_user_guide.md
📄 CodeRabbit inference engine (.claude/rules/generator/cross_module_impact.md)
Document newly added generator parameters in the CLI user guide.
Files:
docs/cli_user_guide.md
tests/**/*.{py,yaml,txt,sh}
📄 CodeRabbit inference engine (.claude/rules/generator/testing.md)
Use integration tests for the full input-to-artifacts pipeline, comparing output against golden snapshots without external dependencies.
Files:
tests/unit/cli/test_cli_workflow.pytests/unit/generator/test_fpm_artifacts.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run unit tests with
pytest -m unit; run the PR build subset withpytest -m "unit or build"when applicable.
Files:
tests/unit/cli/test_cli_workflow.pytests/unit/generator/test_fpm_artifacts.py
tests/**
⚙️ CodeRabbit configuration file
tests/**: - Check that tests cover the changed behavior rather than only the happy path.
- Watch for fixtures or golden outputs that mask backend drift, support-matrix ordering changes, or CLI output regressions.
Files:
tests/unit/cli/test_cli_workflow.pytests/unit/generator/test_fpm_artifacts.py
tests/unit/generator/**
📄 CodeRabbit inference engine (.claude/rules/generator/cross_module_impact.md)
Update generator test fixtures and assertions when parameters are renamed.
Files:
tests/unit/generator/test_fpm_artifacts.py
tests/unit/generator/**/*.py
📄 CodeRabbit inference engine (.claude/rules/generator/testing.md)
tests/unit/generator/**/*.py: Use fast unit tests for pure generator logic, testing individual functions in isolation and mocking schema loading and template rendering.
Test generator edge cases including MoE, disaggregated mode, aggregated mode, speculative decoding, prefix caching, minimal configuration, PVC configuration, each backend, and benchmark mode.
Files:
tests/unit/generator/test_fpm_artifacts.py
tests/unit/generator/test_*.py
📄 CodeRabbit inference engine (.claude/rules/generator/testing.md)
Place generator unit tests in tests/unit/generator/ and use descriptive test files such as test_aggregators.py, test_rule_engine.py, and backend-specific CLI argument tests.
Files:
tests/unit/generator/test_fpm_artifacts.py
🪛 ast-grep (0.44.1)
tests/unit/generator/test_fpm_artifacts.py
[info] 27-33: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"cudagraph_mode": "FULL",
"max_capture_size": 1024,
"compile_sizes": [1, 2, 4, 8],
}
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 418-418: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 546-546: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 599-599: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 669-669: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[error] 289-295: Command coming from incoming request
Context: subprocess.run(
["bash", "-n"],
input=script,
text=True,
capture_output=True,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 429-436: Command coming from incoming request
Context: subprocess.run(
["bash", str(script_path)],
text=True,
capture_output=True,
env=env,
timeout=10,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 441-448: Command coming from incoming request
Context: subprocess.run(
["bash", str(script_path)],
text=True,
capture_output=True,
env=env,
timeout=10,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 507-514: Command coming from incoming request
Context: subprocess.run(
["bash", str(script_path)],
text=True,
capture_output=True,
env=env,
timeout=12,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 557-564: Command coming from incoming request
Context: subprocess.run(
["bash", str(script_path)],
text=True,
capture_output=True,
env=env,
timeout=8,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 610-617: Command coming from incoming request
Context: subprocess.run(
["bash", str(script_path)],
text=True,
capture_output=True,
env=env,
timeout=8,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 690-697: Command coming from incoming request
Context: subprocess.run(
["bash", str(script_path)],
text=True,
capture_output=True,
env=env,
timeout=8,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 756-763: Command coming from incoming request
Context: subprocess.run(
["bash", str(script_path)],
text=True,
capture_output=True,
env=env,
timeout=8,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 996-1002: Command coming from incoming request
Context: subprocess.run(
["bash", str(script_path)],
text=True,
capture_output=True,
timeout=5,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 1040-1047: Command coming from incoming request
Context: subprocess.run(
["bash", str(script_path)],
text=True,
capture_output=True,
env=env,
timeout=5,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
[error] 1120-1127: Command coming from incoming request
Context: subprocess.run(
["bash", str(script_path)],
text=True,
capture_output=True,
env=env,
timeout=8,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 markdownlint-cli2 (0.23.0)
docs/cli_user_guide.md
[warning] 18-18: Link fragments should be valid
(MD051, link-fragments)
🔇 Additional comments (15)
README.md (1)
102-104: LGTM!docs/cli_user_guide.md (1)
689-689: LGTM!Also applies to: 698-770, 1018-1018
docs/dynamo_deployment_guide.md (1)
3-3: LGTM!Also applies to: 488-515
docs/generator_overview.md (1)
13-13: LGTM!Also applies to: 96-96, 176-214
tests/unit/cli/test_cli_workflow.py (1)
25-25: LGTM!Also applies to: 199-243, 830-830
src/aiconfigurator/cli/main.py (2)
140-144: LGTM!Also applies to: 2370-2371, 2439-2439
2312-2317: 🩺 Stability & AvailabilityFPM validation is already enforced in both paths —
src/aiconfigurator/cli/main.pyblocks non-agg/non-vLLM tasks before_execute_tasks, andsrc/aiconfigurator/generator/builders/fpm_builder.pyrejects non-vLLM or non-aggFPM inputs. A shared preflight helper would be cleanup only.> Likely an incorrect or invalid review comment.src/aiconfigurator/generator/main.py (1)
98-98: LGTM!src/aiconfigurator/generator/rendering/engine.py (1)
255-256: LGTM!Also applies to: 709-725
src/aiconfigurator/generator/config/deployment_config.yaml (1)
131-135: 🗄️ Data Integrity & IntegrationLeave these optional FPM-only fields unset. The schema already omits defaults for other optional inputs, and the defaults loader only applies a value when
defaultis present. Keep these absent so they remain no-ops when unused.> Likely an incorrect or invalid review comment.src/aiconfigurator/generator/aggregators.py (1)
286-291: LGTM!src/aiconfigurator/generator/api.py (1)
277-281: LGTM!Also applies to: 318-318, 675-676, 777-777
src/aiconfigurator/generator/artifacts.py (1)
19-19: LGTM!Also applies to: 43-44
src/aiconfigurator/generator/builders/k8s_builder.py (1)
229-229: LGTM!Also applies to: 536-536, 830-830
src/aiconfigurator/generator/builders/fpm_builder.py (1)
1-385: LGTM!Also applies to: 618-950
| - `--top-n N`: Number of top configurations to output — per experiment in `exp` mode, or per serving mode (agg/disagg) in `default` mode. Default: `5`. (`default`, `exp`, `generate`, `estimate`) | ||
| - `--systems-paths`: System search paths (comma-separated). Use `default` for the built-in systems path; the first match wins for an identical system/backend/version. (`default`, `exp`, `generate`, `estimate`) | ||
| - `--deployment-target`: Generated-artifact platform — `dynamo-j2` (default), `dynamo-python`, or `llm-d`. See [Deployment Target Selection](#default-mode). (`default`, `exp`, `generate`, `estimate`) | ||
| - `--deployment-target`: Generated-artifact platform — `dynamo-j2` (default), `dynamo-python`, `llm-d-helm`, `llm-d-kustomize`, or `fpm`. See [Deployment Target Selection](#deployment-target-selection). (`default`, `exp`, `generate`, `estimate`) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File listing ==\n'
git ls-files docs/cli_user_guide.md
printf '\n== Relevant lines around the link ==\n'
sed -n '1,40p' docs/cli_user_guide.md
printf '\n== Heading / anchor candidates in the doc ==\n'
rg -n '^#+ .*Deployment Target Selection|^#+ .*' docs/cli_user_guide.md | sed -n '1,120p'
printf '\n== markdownlint config / script references (if any) ==\n'
rg -n 'markdownlint|md013|MD034|MD001|Deployment Target Selection' .github docs package.json .markdownlint* . --glob '!**/node_modules/**' || trueRepository: ai-dynamo/aiconfigurator
Length of output: 5884
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '680,715p' docs/cli_user_guide.md | cat -n
printf '\n== Any explicit anchor / HTML ids nearby ==\n'
rg -n '<a id=|id="|id=' docs/cli_user_guide.mdRepository: ai-dynamo/aiconfigurator
Length of output: 3073
Fix the Deployment Target Selection link target. The section at docs/cli_user_guide.md is plain bold text, not a heading or anchor, so #deployment-target-selection has nowhere to jump. Make it a real heading or add an explicit anchor.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 18-18: Link fragments should be valid
(MD051, link-fragments)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/cli_user_guide.md` at line 18, Update the Deployment Target Selection
section in cli_user_guide.md so it exposes the deployment-target-selection
anchor, preferably by converting the existing bold section label into a Markdown
heading while preserving its text and content.
Sources: Path instructions, Linters/SAST tools
| def _render_run_script( | ||
| command: list[str], | ||
| env: list[tuple[str, str]], | ||
| benchmark_mode: str, | ||
| benchmark_output_path: str, | ||
| wait_timeout_seconds: int, | ||
| topology: dict[str, int], | ||
| ) -> str: | ||
| lines = [ | ||
| "#!/usr/bin/env bash", | ||
| "set -Eeuo pipefail", | ||
| "", | ||
| "ulimit -l unlimited || true", | ||
| "ulimit -n 1048576 || true", | ||
| ] | ||
| for name, value in env: | ||
| lines.append(f"export {name}={shlex.quote(value)}") | ||
|
|
||
| lines.extend( | ||
| [ | ||
| "", | ||
| f"node_count={topology['node_count']}", | ||
| f"data_parallel_size={topology['data_parallel_size']}", | ||
| f"local_data_parallel_size={topology['local_data_parallel_size']}", | ||
| "if (( node_count > 1 )); then", | ||
| ' node_rank="${LWS_WORKER_INDEX:?LWS_WORKER_INDEX is required for multinode FPM}"', | ||
| ' master_addr="${LWS_LEADER_ADDRESS:?LWS_LEADER_ADDRESS is required for multinode FPM}"', | ||
| "else", | ||
| ' node_rank="${LWS_WORKER_INDEX:-0}"', | ||
| ' master_addr="${LWS_LEADER_ADDRESS:-127.0.0.1}"', | ||
| "fi", | ||
| 'if ! [[ "$node_rank" =~ ^[0-9]+$ ]] || (( node_rank >= node_count )); then', | ||
| ' echo "Invalid FPM node rank: $node_rank (node_count=$node_count)" >&2', | ||
| " exit 2", | ||
| "fi", | ||
| 'export DYN_FPM_WORKER_ID="${DYN_FPM_WORKER_ID:-${FPM_RUN_ID:-fpm}-node${node_rank}}"', | ||
| f"benchmark_mode={shlex.quote(benchmark_mode)}", | ||
| f"benchmark_output_path={shlex.quote(benchmark_output_path)}", | ||
| f"wait_timeout_seconds={wait_timeout_seconds}", | ||
| f"engine_command=({' '.join(shlex.quote(token) for token in command)})", | ||
| 'for index in "${!engine_command[@]}"; do', | ||
| f' engine_command[$index]="${{engine_command[$index]//{_NODE_RANK_SENTINEL}/$node_rank}}"', | ||
| "done", | ||
| "", | ||
| "if (( node_count > 1 )); then", | ||
| " if (( data_parallel_size > 1 )); then", | ||
| ' engine_command+=(--data-parallel-size-local "$local_data_parallel_size")', | ||
| ' engine_command+=(--data-parallel-start-rank "$((node_rank * local_data_parallel_size))")', | ||
| ' engine_command+=(--data-parallel-address "$master_addr" --data-parallel-rpc-port 29510)', | ||
| " engine_command+=(--data-parallel-hybrid-lb)", | ||
| " else", | ||
| ' engine_command+=(--nnodes "$node_count" --node-rank "$node_rank")', | ||
| ' engine_command+=(--master-addr "$master_addr" --master-port 29500)', | ||
| " if (( node_rank > 0 )); then", | ||
| ' exec "${engine_command[@]}" --headless', | ||
| " fi", | ||
| " fi", | ||
| "fi", | ||
| "", | ||
| "benchmark_path_for_dp_rank() {", | ||
| " local dp_rank=$1", | ||
| ' local directory=""', | ||
| ' local filename="$benchmark_output_path"', | ||
| ' if [[ "$benchmark_output_path" == */* ]]; then', | ||
| ' directory="${benchmark_output_path%/*}/"', | ||
| ' filename="${benchmark_output_path##*/}"', | ||
| " fi", | ||
| " if (( dp_rank == 0 )); then", | ||
| ' printf "%s\\n" "$benchmark_output_path"', | ||
| ' elif [[ "$filename" == *.* ]]; then', | ||
| ' printf "%s%s_dp%s.%s\\n" "$directory" "${filename%.*}" "$dp_rank" "${filename##*.}"', | ||
| " else", | ||
| ' printf "%s%s_dp%s\\n" "$directory" "$filename" "$dp_rank"', | ||
| " fi", | ||
| "}", | ||
| "", | ||
| "expected_results=()", | ||
| "local_dp_start=$((node_rank * local_data_parallel_size))", | ||
| "local_dp_end=$((local_dp_start + local_data_parallel_size))", | ||
| "for ((dp_rank=local_dp_start; dp_rank<local_dp_end; dp_rank++)); do", | ||
| ' expected_results+=("$(benchmark_path_for_dp_rank "$dp_rank")")', | ||
| "done", | ||
| 'for path in "${expected_results[@]}"; do', | ||
| ' if [[ -e "$path" || -L "$path" ]]; then', | ||
| ' echo "Refusing to overwrite existing benchmark output: $path" >&2', | ||
| " exit 1", | ||
| " fi", | ||
| ' mkdir -p -- "$(dirname -- "$path")"', | ||
| "done", | ||
| "", | ||
| "check_result_files() {", | ||
| ' python3 - "$local_dp_start" "$benchmark_mode" "${expected_results[@]}" <<\'PY\'', | ||
| "import json", | ||
| "import pathlib", | ||
| "import sys", | ||
| "", | ||
| "start_rank = int(sys.argv[1])", | ||
| "expected_mode = sys.argv[2]", | ||
| "", | ||
| "def invalid(path, message):", | ||
| ' print(f"Invalid FPM benchmark result {path}: {message}", file=sys.stderr)', | ||
| " raise SystemExit(20)", | ||
| "", | ||
| "for offset, raw_path in enumerate(sys.argv[3:]):", | ||
| " path = pathlib.Path(raw_path)", | ||
| " if not path.is_file() or path.stat().st_size == 0:", | ||
| " raise SystemExit(10)", | ||
| " try:", | ||
| ' value = json.loads(path.read_text(encoding="utf-8"))', | ||
| " except (OSError, json.JSONDecodeError):", | ||
| " raise SystemExit(10)", | ||
| ' if (value.get("schema_version") != 1 or value.get("status") != "complete"', | ||
| ' or value.get("valid") is not True):', | ||
| " invalid(path,", | ||
| " f\"schema_version={value.get('schema_version')!r} \"", | ||
| " f\"status={value.get('status')!r} valid={value.get('valid')!r} \"", | ||
| " f\"skipped_points={value.get('skipped_points')!r}\")", | ||
| ' config = value.get("config")', | ||
| ' actual_mode = config.get("mode") if isinstance(config, dict) else None', | ||
| " if actual_mode != expected_mode:", | ||
| ' invalid(path, f"benchmark mode {actual_mode!r} "', | ||
| ' f"!= {expected_mode!r}")', | ||
| ' coverage = value.get("coverage")', | ||
| " if not isinstance(coverage, dict):", | ||
| ' invalid(path, "coverage must be an object")', | ||
| ' expected_points = coverage.get("expected_points")', | ||
| ' completed_points = coverage.get("completed_points")', | ||
| ' skipped_points = coverage.get("skipped_points")', | ||
| " if (type(expected_points) is not int or expected_points <= 0", | ||
| " or type(completed_points) is not int or completed_points != expected_points", | ||
| " or type(skipped_points) is not int or skipped_points != 0):", | ||
| ' invalid(path, f"invalid coverage {coverage!r}")', | ||
| ' results = value.get("results")', | ||
| " result_count = len(results) if isinstance(results, list) else None", | ||
| " if not isinstance(results, list) or len(results) != completed_points:", | ||
| ' invalid(path, f"results count does not match coverage: "', | ||
| ' f"{type(results).__name__}({result_count!r}) "', | ||
| ' f"!= {completed_points}")', | ||
| " expected_rank = start_rank + offset", | ||
| " observed_ranks = set()", | ||
| " for result in results:", | ||
| " if not isinstance(result, dict):", | ||
| ' invalid(path, "result entry must be an object")', | ||
| ' point = result.get("point")', | ||
| ' point_type = point.get("point_type") if isinstance(point, dict) else None', | ||
| " if point_type != expected_mode:", | ||
| ' invalid(path, f"point type {point_type!r} "', | ||
| ' f"!= {expected_mode!r}")', | ||
| ' fpms = result.get("fpms")', | ||
| " if not isinstance(fpms, list) or not fpms:", | ||
| ' invalid(path, "each result must contain at least one FPM sample")', | ||
| " for fpm in fpms:", | ||
| ' rank = fpm.get("dp_rank") if isinstance(fpm, dict) else None', | ||
| " if type(rank) is not int:", | ||
| ' invalid(path, f"invalid FPM dp_rank {rank!r}")', | ||
| " observed_ranks.add(rank)", | ||
| " if observed_ranks != {expected_rank}:", | ||
| ' invalid(path, f"FPM dp_ranks {sorted(observed_ranks)!r} "', | ||
| ' f"!= [{expected_rank}]")', | ||
| "PY", | ||
| "}", | ||
| "", | ||
| 'engine_pid=""', | ||
| "engine_shutdown_grace_seconds=30", | ||
| "terminate_engine() {", | ||
| " local pid=$1", | ||
| ' kill -TERM -- "-$pid" 2>/dev/null || kill -TERM "$pid" 2>/dev/null || true', | ||
| " local shutdown_deadline=$((SECONDS + engine_shutdown_grace_seconds))", | ||
| ' while kill -0 "$pid" 2>/dev/null || kill -0 -- "-$pid" 2>/dev/null; do', | ||
| " if (( SECONDS >= shutdown_deadline )); then", | ||
| ' echo "Engine did not stop within ${engine_shutdown_grace_seconds}s; sending SIGKILL" >&2', | ||
| ' kill -KILL -- "-$pid" 2>/dev/null || true', | ||
| ' kill -KILL "$pid" 2>/dev/null || true', | ||
| " break", | ||
| " fi", | ||
| " sleep 1", | ||
| " done", | ||
| ' wait "$pid" 2>/dev/null || true', | ||
| "}", | ||
| "cleanup() {", | ||
| " local status=$?", | ||
| " trap - EXIT INT TERM", | ||
| ' if [[ -n "${engine_pid:-}" ]]; then', | ||
| ' terminate_engine "$engine_pid"', | ||
| " fi", | ||
| ' exit "$status"', | ||
| "}", | ||
| "trap cleanup EXIT", | ||
| "trap 'exit 130' INT", | ||
| "trap 'exit 143' TERM", | ||
| "", | ||
| "python3 -c 'import os, sys; os.setsid(); os.execvp(sys.argv[1], sys.argv[1:])' \"${engine_command[@]}\" &", | ||
| "engine_pid=$!", | ||
| "deadline=$((SECONDS + wait_timeout_seconds))", | ||
| "", | ||
| "while true; do", | ||
| " set +e", | ||
| " check_result_files", | ||
| " result_status=$?", | ||
| " set -e", | ||
| " if (( result_status == 0 )); then", | ||
| " break", | ||
| " fi", | ||
| " if (( result_status == 20 )); then", | ||
| " exit 1", | ||
| " fi", | ||
| ' if ! kill -0 "$engine_pid" 2>/dev/null; then', | ||
| " set +e", | ||
| ' wait "$engine_pid"', | ||
| " engine_status=$?", | ||
| " set -e", | ||
| ' terminate_engine "$engine_pid"', | ||
| ' engine_pid=""', | ||
| ' echo "Engine exited before writing all FPM benchmark outputs" >&2', | ||
| ' if (( engine_status == 0 )); then exit 1; else exit "$engine_status"; fi', | ||
| " fi", | ||
| " if (( SECONDS >= deadline )); then", | ||
| ' echo "Timed out waiting for all FPM benchmark outputs" >&2', | ||
| " exit 124", | ||
| " fi", | ||
| " sleep 2", | ||
| "done", | ||
| "", | ||
| 'terminate_engine "$engine_pid"', | ||
| 'engine_pid=""', | ||
| "trap - EXIT INT TERM", | ||
| "", | ||
| ] | ||
| ) | ||
| return "\n".join(lines) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Render FPM run.sh from a Jinja template.
This flat-text artifact is assembled in Python, and it emits #!/usr/bin/env bash instead of the required #!/bin/bash. Move the script into a Jinja template, pass prepared values from this builder, and update golden coverage. A one-click change is unsafe because this spans templates and snapshots.
As per coding guidelines, “Use Jinja templates for flat-text artifacts such as cli_args, run.sh, and engine arguments,” and generated scripts must use #!/bin/bash and set -e. As per path instructions, multi-file generated-artifact fixes should not use one-click suggestions.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiconfigurator/generator/builders/fpm_builder.py` around lines 386 - 615,
Replace the inline script assembly in _render_run_script with a Jinja template
for the FPM run.sh artifact, passing all required prepared values and generated
command data from the builder. Ensure the template emits #!/bin/bash and set -e
while preserving the existing runtime behavior, then update the associated
golden snapshots and template coverage to match the generated output.
Sources: Coding guidelines, Path instructions
| " try:", | ||
| ' value = json.loads(path.read_text(encoding="utf-8"))', | ||
| " except (OSError, json.JSONDecodeError):", | ||
| " raise SystemExit(10)", | ||
| ' if (value.get("schema_version") != 1 or value.get("status") != "complete"', | ||
| ' or value.get("valid") is not True):', | ||
| " invalid(path,", | ||
| " f\"schema_version={value.get('schema_version')!r} \"", | ||
| " f\"status={value.get('status')!r} valid={value.get('valid')!r} \"", | ||
| " f\"skipped_points={value.get('skipped_points')!r}\")", |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject non-object JSON as a terminal invalid result.
A valid JSON scalar or array crashes on value.get(). Status 1 is treated as retryable, potentially delaying failure until the 7,800-second timeout while the engine remains alive.
Proposed fix
" except (OSError, json.JSONDecodeError):",
" raise SystemExit(10)",
+ " if not isinstance(value, dict):",
+ ' invalid(path, f"top-level JSON must be an object, got {type(value).__name__}")',
' if (value.get("schema_version") != 1 or value.get("status") != "complete"',Add a regression case using a non-object JSON result.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| " try:", | |
| ' value = json.loads(path.read_text(encoding="utf-8"))', | |
| " except (OSError, json.JSONDecodeError):", | |
| " raise SystemExit(10)", | |
| ' if (value.get("schema_version") != 1 or value.get("status") != "complete"', | |
| ' or value.get("valid") is not True):', | |
| " invalid(path,", | |
| " f\"schema_version={value.get('schema_version')!r} \"", | |
| " f\"status={value.get('status')!r} valid={value.get('valid')!r} \"", | |
| " f\"skipped_points={value.get('skipped_points')!r}\")", | |
| " try:", | |
| ' value = json.loads(path.read_text(encoding="utf-8"))', | |
| " except (OSError, json.JSONDecodeError):", | |
| " raise SystemExit(10)", | |
| " if not isinstance(value, dict):", | |
| ' invalid(path, f"top-level JSON must be an object, got {type(value).__name__}")', | |
| ' if (value.get("schema_version") != 1 or value.get("status") != "complete"', | |
| ' or value.get("valid") is not True):', | |
| " invalid(path,", | |
| " f\"schema_version={value.get('schema_version')!r} \"", | |
| " f\"status={value.get('status')!r} valid={value.get('valid')!r} \"", | |
| " f\"skipped_points={value.get('skipped_points')!r}\")", |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiconfigurator/generator/builders/fpm_builder.py` around lines 493 - 502,
Validate that the JSON loaded in the terminal-result reader is an object before
accessing it with value.get(). Treat scalars and arrays as terminal invalid
results through invalid(path, ...) rather than allowing an AttributeError or
retrying; preserve the existing invalid diagnostics for object results. Add a
regression test covering a non-object JSON result.
| - key: K8sConfig.fpm_shared_memory_size | ||
| required: false | ||
| - key: K8sConfig.fpm_resource_labels | ||
| required: false |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== deployment_config.yaml snippet ==="
sed -n '110,160p' src/aiconfigurator/generator/config/deployment_config.yaml
echo
echo "=== search for field usages ==="
rg -n "fpm_shared_memory_size|fpm_resource_labels|sizeLimit|resource_labels|shared_memory" src/aiconfigurator/generator -S
echo
echo "=== outline likely relevant files ==="
fd -a "deployment_config.yaml|.*validator.*|.*schema.*|.*k8s.*" src/aiconfigurator/generatorRepository: ai-dynamo/aiconfigurator
Length of output: 6210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== fpm_builder.py around validation ==="
sed -n '650,810p' src/aiconfigurator/generator/builders/fpm_builder.py
echo
echo "=== search tests for these fields ==="
rg -n "fpm_shared_memory_size|fpm_resource_labels|sizeLimit|reserved label" tests src/aiconfigurator -SRepository: ai-dynamo/aiconfigurator
Length of output: 7924
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== relevant test section ==="
sed -n '180,280p' tests/unit/generator/test_fpm_artifacts.py
echo
echo "=== search for invalid-input coverage ==="
rg -n "raises|invalid|quantity|label|fpm_shared_memory_size|fpm_resource_labels" tests/unit/generator/test_fpm_artifacts.py tests -SRepository: ai-dynamo/aiconfigurator
Length of output: 50380
Add Kubernetes-format validation for FPM passthroughs. fpm_shared_memory_size still needs a real quantity check before writing emptyDir.sizeLimit, and fpm_resource_labels should reject invalid Kubernetes label key/value syntax instead of only checking Python types. Add negative tests for malformed quantities and labels.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiconfigurator/generator/config/deployment_config.yaml` around lines 132
- 135, Extend validation for the FPM passthrough fields
K8sConfig.fpm_shared_memory_size and K8sConfig.fpm_resource_labels: validate
shared memory values as Kubernetes resource quantities before assigning
emptyDir.sizeLimit, and validate label keys and values against Kubernetes label
syntax rather than only their Python types. Add negative tests covering
malformed quantities and invalid label keys/values.
Sources: Coding guidelines, Path instructions
| p_art.add_argument( | ||
| "--deployment-target", | ||
| choices=["dynamo-j2", "dynamo-python", "llm-d-helm", "llm-d-kustomize", "fpm"], | ||
| default="dynamo-j2", | ||
| help="Artifact target. 'fpm' emits a reusable resource Pod and run.sh.", | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
git ls-files src/aiconfigurator/generator/main.py src/aiconfigurator/generator | sed -n '1,120p'
echo '--- main.py ---'
cat -n src/aiconfigurator/generator/main.py | sed -n '1,180p'
echo '--- search FPM/artifacts ---'
rg -n "build_fpm_artifacts|deployment-target|fpm|backend == \"vllm\"|deployment_target" src/aiconfigurator/generator -SRepository: ai-dynamo/aiconfigurator
Length of output: 26226
🏁 Script executed:
#!/bin/bash
set -e
sed -n '640,790p' src/aiconfigurator/generator/api.py
echo '---'
sed -n '700,735p' src/aiconfigurator/generator/rendering/engine.py
echo '---'
sed -n '1,140p' src/aiconfigurator/generator/builders/fpm_builder.pyRepository: ai-dynamo/aiconfigurator
Length of output: 13861
🏁 Script executed:
#!/bin/bash
set -e
echo '--- generate_backend_artifacts ---'
sed -n '240,360p' src/aiconfigurator/generator/api.py
echo '--- request cli ---'
sed -n '1,120p' src/aiconfigurator/generator/request/cli.py
echo '--- deployment target mentions ---'
rg -n "deployment_target.*fpm|backend != \"vllm\"|supports only the vllm backend|unsupported.*deployment_target|reject.*deployment_target" src/aiconfigurator/generator -SRepository: ai-dynamo/aiconfigurator
Length of output: 7483
Reject invalid FPM/backend combinations earlier.
The CLI still advertises fpm for every backend, but build_fpm_artifacts() only accepts vllm. render-artifacts --backend trtllm --deployment-target fpm makes it through dispatch and fails later in the FPM builder instead of being rejected up front. Add a shared backend/target compatibility check after parsing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/aiconfigurator/generator/main.py` around lines 66 - 71, Add a shared
backend/deployment-target compatibility validation immediately after CLI
argument parsing, rejecting any fpm target whose backend is not vllm before
dispatch reaches build_fpm_artifacts(). Reuse the parsed backend and
deployment-target values and provide a clear CLI error for invalid combinations,
while preserving all currently supported combinations.
Source: Path instructions
| @patch("aiconfigurator.cli.main.run_spica_thorough_default") | ||
| @patch("aiconfigurator.cli.main._execute_tasks") | ||
| def test_cli_fpm_rejects_thorough_sweep_before_spica( | ||
| self, | ||
| mock_execute, | ||
| mock_run_spica, | ||
| cli_args_factory, | ||
| ): | ||
| args = cli_args_factory( | ||
| mode="default", | ||
| thorough_sweep=True, | ||
| deployment_target="fpm", | ||
| ) | ||
|
|
||
| with pytest.raises(SystemExit, match="does not support Spica thorough sweeps"): | ||
| cli_main(args) | ||
|
|
||
| mock_run_spica.assert_not_called() | ||
| mock_execute.assert_not_called() |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Cover the FPM --thorough-config rejection too.
Production rejects both thorough_sweep and thorough_config, but this test covers only the former. Parametrize both inputs and retain the no-Spica/no-execution assertions. A one-click patch is unsafe because the fixture’s thorough_config path handling is not shown.
As per path instructions, “Check that tests cover the changed behavior rather than only the happy path.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/cli/test_cli_workflow.py` around lines 263 - 281, Extend
test_cli_fpm_rejects_thorough_sweep_before_spica to cover both thorough_sweep
and thorough_config inputs, preferably by parameterizing the relevant fixture
arguments and expected rejection message. Preserve the existing SystemExit
assertion and verify run_spica_thorough_default and _execute_tasks are not
called for either FPM rejection path.
Source: Path instructions
| from aiconfigurator.generator.main import main as generator_main | ||
| from aiconfigurator.generator.rendering.engine import render_backend_templates | ||
|
|
||
| pytestmark = pytest.mark.unit |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Split runtime artifact tests out of the unit suite.
This file renders full artifacts and executes generated Bash processes, including sleeps and shutdown timeouts. Keep isolated validation tests here; move pipeline/runtime cases to integration coverage and snapshots. A one-click marker change is unsafe without the repository’s integration-marker and CI registration context.
As per coding guidelines, “Use fast unit tests for pure generator logic, testing individual functions in isolation and mocking schema loading and template rendering.” As per path instructions, full input-to-artifacts tests should be integration tests comparing golden snapshots.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/generator/test_fpm_artifacts.py` at line 25, Replace the unit
marker on the tests in this file with the repository’s established integration
or snapshot classification, and move full artifact-rendering and generated Bash
runtime cases into the corresponding integration/snapshot test locations. Keep
only isolated generator-validation tests here, following existing marker and CI
registration conventions rather than applying a marker-only change.
Sources: Coding guidelines, Path instructions
| completed = subprocess.run( | ||
| ["bash", str(script_path)], | ||
| text=True, | ||
| capture_output=True, | ||
| env=env, | ||
| timeout=10, | ||
| check=False, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clean up fake engines when a subprocess timeout fires.
subprocess.run(timeout=...) kills only the Bash process; if generated cleanup regresses, the looping engine descendants survive. Use a shared runner that sends TERM first so the script trap runs, then escalates to KILL. This must cover all runtime call sites, so a local one-click change is unsafe.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/generator/test_fpm_artifacts.py` around lines 430 - 437, Replace
the direct subprocess.run usage in the test runtime call sites with a shared
subprocess runner that handles timeouts by sending TERM first, allowing the
script trap to clean up engine descendants, then escalating to KILL if needed.
Ensure every invocation of the generated Bash scripts uses this runner rather
than applying a local change only to the shown completed call.
| @pytest.mark.parametrize( | ||
| ("result", "expected_message"), | ||
| [ | ||
| (_benchmark_result(schema_version=2), "schema_version=2"), | ||
| (_benchmark_result(mode="decode"), "benchmark mode 'decode' != 'prefill'"), | ||
| (_benchmark_result(dp_rank=1), "FPM dp_ranks [1] != [0]"), | ||
| ], | ||
| ) | ||
| def test_fpm_run_script_rejects_mismatched_result_identity(tmp_path, result, expected_message): | ||
| output_path = tmp_path / "benchmark.json" | ||
| params = _params() | ||
| for entry in params["K8sConfig"]["extra_env"]: | ||
| if entry["name"] == "DYN_FPM_BENCHMARK_OUTPUT_PATH": | ||
| entry["value"] = str(output_path) | ||
|
|
||
| fake_package = tmp_path / "fake-package" / "dynamo" / "vllm" | ||
| fake_package.mkdir(parents=True) | ||
| (fake_package.parent / "__init__.py").write_text("") | ||
| (fake_package / "__init__.py").write_text("") | ||
| (fake_package / "__main__.py").write_text( | ||
| f"""\ | ||
| import pathlib | ||
| import signal | ||
| import sys | ||
| import time | ||
|
|
||
| path = pathlib.Path(sys.argv[sys.argv.index("--benchmark-output-path") + 1]) | ||
| path.parent.mkdir(parents=True, exist_ok=True) | ||
| path.write_text({json.dumps(result)!r}) | ||
| signal.signal(signal.SIGTERM, lambda *_: sys.exit(0)) | ||
| while True: | ||
| time.sleep(0.1) | ||
| """ | ||
| ) | ||
| script_path = tmp_path / "run.sh" | ||
| script_path.write_text(_render(params)["run.sh"]) | ||
| env = dict(os.environ) | ||
| env["PYTHONPATH"] = str(tmp_path / "fake-package") | ||
|
|
||
| completed = subprocess.run( | ||
| ["bash", str(script_path)], | ||
| text=True, | ||
| capture_output=True, | ||
| env=env, | ||
| timeout=8, | ||
| check=False, | ||
| ) | ||
|
|
||
| assert completed.returncode == 1 | ||
| assert expected_message in completed.stderr |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Exercise the remaining schema-v1 rejection branches.
The table omits status != "complete" despite the unused status helper parameter, and valid=False exits before testing malformed coverage or result-count handling. Add failed-status and inconsistent coverage/result-count cases.
As per path instructions, “Check that tests cover the changed behavior rather than only the happy path.”
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 599-599: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[error] 610-617: Command coming from incoming request
Context: subprocess.run(
["bash", str(script_path)],
text=True,
capture_output=True,
env=env,
timeout=8,
check=False,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/generator/test_fpm_artifacts.py` around lines 572 - 621, Extend
test_fpm_run_script_rejects_mismatched_result_identity to cover status !=
"complete" and schema-v1 invalid coverage/result-count combinations. Use the
existing status and valid helpers to construct cases that reach malformed
coverage and result-count validation, and assert each case exits with code 1 and
reports its expected error.
Source: Path instructions
| @pytest.mark.parametrize("benchmark_mode", ["prefill", "decode"]) | ||
| def test_fpm_accepts_phase_specific_benchmark_mode(benchmark_mode): | ||
| params = _params() | ||
| args = params["params"]["agg"]["extra_cli_args"] | ||
| index = args.index("--benchmark-mode") | ||
| args[index + 1] = benchmark_mode | ||
|
|
||
| assert f"--benchmark-mode {benchmark_mode}" in _render(params)["run.sh"] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Run the decode mode through result validation.
This only checks emitted argv; every successful run.sh test uses prefill. Add an end-to-end decode case with a valid decode result so a hardcoded prefill result gate cannot regress unnoticed.
As per path instructions, “Check that tests cover the changed behavior rather than only the happy path.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/unit/generator/test_fpm_artifacts.py` around lines 1255 - 1262, Add an
end-to-end decode-mode test alongside
test_fpm_accepts_phase_specific_benchmark_mode that supplies a valid decode
result and executes the generated run.sh through result validation. Ensure the
test verifies successful validation for decode, not merely the emitted
--benchmark-mode argument, while preserving the existing prefill coverage.
Source: Path instructions
1 participant
Summary:
prefill/decodebenchmark modes and alignsrun.shwith Dynamo's merged schema-v1 result contract.dp_rank, and bounded engine process-group cleanup.Test plan:
pytest tests/unit/generator/test_fpm_artifacts.py: 68 passed.ruff check,ruff format --check, andgit diff --check: passed.dp_rank=0,run.shexited 0, engine children were removed, and owned resources were deleted.Notes:
62633bb); that commit does not overlap the Generator files changed here.Relates to #1328
Summary by CodeRabbit
New Features
Bug Fixes
Documentation