feat: [generator] Add FPM deployment target with reusable Pod and run script (phase 1)#1328
feat: [generator] Add FPM deployment target with reusable Pod and run script (phase 1)#1328Ethan-ES wants to merge 4 commits into
Conversation
WalkthroughThis PR adds an ChangesFPM V1 Deployment Target
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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: 4
🤖 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 `@src/aiconfigurator/cli/main.py`:
- Around line 137-145: The shared CLI parser in main.py allows
--deployment-target fpm too broadly, so invalid FPM combinations are only
rejected later inside build_fpm_artifacts(). Add an upfront validation path in
the parser/CLI setup around common_parser.add_argument and the deployment-target
handling so unsupported default/exp cases are rejected before entering expensive
sweep/render work. Keep the existing FPM V1 constraint checks in
build_fpm_artifacts(), but also gate the fpm target earlier using the
deployment-target selection logic and the FPM-specific config symbols.
In `@src/aiconfigurator/generator/builders/fpm_builder.py`:
- Around line 185-203: The `_ensure_benchmark_output_path` logic in
`fpm_builder.py` mishandles an explicit empty `--benchmark-output-path`, because
`value = cli_value or env_value` treats an empty CLI string as missing and the
later `cli_value is None` guard lets `args` and `env` get out of sync. Update
`_ensure_benchmark_output_path` to validate emptiness before resolving/filling
defaults, and treat `cli_value` as present whenever the flag was provided even
if empty; then only append the default to `args` and `env` after confirming the
resolved path is non-empty, so the path used by `run.sh` always matches the
stored benchmark output path.
- Around line 246-296: The engine shutdown path is unbounded in both cleanup()
and the final happy-path termination, so a stuck process can block the reusable
Pod forever. Update the logic around engine_pid in cleanup() and the final
kill/wait sequence to use a bounded termination flow with the same
wait_timeout_seconds behavior as the benchmark-output loop, e.g. send SIGTERM,
wait only up to the timeout, then force cleanup with SIGKILL or exit with a
timeout status if the engine does not stop. Keep the timeout handling
centralized in a helper so both paths share the same bounded shutdown behavior.
- Around line 130-152: The _collect_concrete_env helper only validates
worker.envs and main_container.env, but FPM V1 also needs to reject
envFromSecret on DGDService because those secret-backed variables are not
propagated into the generated Pod or run.sh. Update the FPM builder path that
assembles environment handling to detect envFromSecret on the worker service and
fail fast with a clear error, alongside the existing valueFrom checks, so the
validation is enforced in the same code path as _collect_concrete_env.
🪄 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: 29667ac7-a857-43ff-aee1-537f4cf54300
📒 Files selected for processing (13)
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/main.pysrc/aiconfigurator/generator/rendering/engine.pytests/unit/cli/test_cli_workflow.pytests/unit/generator/test_fpm_artifacts.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: AIC Accuracy Regression Testing
- GitHub Check: Cargo Deny
- GitHub Check: Rust/Python engine-step parity
- GitHub Check: Build and Test (e2e)
- GitHub Check: Build and Test (unit)
🧰 Additional context used
📓 Path-based instructions (11)
**/*.py
📄 CodeRabbit inference engine (.claude/rules/generator/guard_rails.md)
**/*.py: Catch unsupported system/backend/version combinations early with fast validation to prevent wasting user time on late failures after profiling.
MoE model detection must useconfig.json, not model name patterns, to correctly identify MoE models with non-standard naming.
Usemodel_familynotmodel_namefor model compatibility checks;model_nameincludes size/quantization info that changes across variants.
safe_model_namemust be generated BEFORE saving results to avoid race condition with rawmodel_pathcontaining slashes.
Quantization type must be inferred from model config, not name patterns, to handle inconsistent naming conventions.
Files:
tests/unit/cli/test_cli_workflow.pysrc/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/cli/main.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/main.pysrc/aiconfigurator/generator/api.pysrc/aiconfigurator/generator/rendering/engine.pysrc/aiconfigurator/generator/builders/fpm_builder.pytests/unit/generator/test_fpm_artifacts.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:
tests/unit/cli/test_cli_workflow.pysrc/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/cli/main.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/main.pydocs/dynamo_deployment_guide.mdsrc/aiconfigurator/generator/api.pyREADME.mdsrc/aiconfigurator/generator/rendering/engine.pydocs/cli_user_guide.mddocs/generator_overview.mdsrc/aiconfigurator/generator/builders/fpm_builder.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
src/aiconfigurator/generator/**/*.py
📄 CodeRabbit inference engine (.claude/rules/generator/debugging.md)
When debugging generator output issues, verify template variable names match the rendering context in
src/aiconfigurator/generator/rendering/engine.py:make_worker_context()
Files:
src/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/main.pysrc/aiconfigurator/generator/api.pysrc/aiconfigurator/generator/rendering/engine.pysrc/aiconfigurator/generator/builders/fpm_builder.py
src/aiconfigurator/generator/**/?(naive|aggregators).py
📄 CodeRabbit inference engine (.claude/rules/generator/debugging.md)
Verify RFC-1123 compliance by checking
naive.pyoraggregators.pywhen debugging RFC-1123 violation bugs in generator output
Files:
src/aiconfigurator/generator/aggregators.py
src/aiconfigurator/generator/**
📄 CodeRabbit inference engine (AGENTS.md)
Before making any change under
src/aiconfigurator/generator/**, you must read.claude/rules/generator-development.mdfirst.
Files:
src/aiconfigurator/generator/aggregators.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/main.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/aggregators.pysrc/aiconfigurator/generator/artifacts.pysrc/aiconfigurator/generator/main.pysrc/aiconfigurator/generator/api.pysrc/aiconfigurator/generator/rendering/engine.pysrc/aiconfigurator/generator/builders/fpm_builder.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
**/*.md
📄 CodeRabbit inference engine (.claude/rules/generator/config_schema.md)
**/*.md: Add computation rules in the rules authoring system when a new parameter requires non-trivial value computation beyond simple defaults
Add template modifications in the template authoring system when a new parameter requires special handling in generated artifacts
Files:
docs/dynamo_deployment_guide.mdREADME.mddocs/cli_user_guide.mddocs/generator_overview.md
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/generator/api.py
📄 CodeRabbit inference engine (.claude/rules/generator/debugging.md)
Review
api.py:parse_cli_params()to verify dotted-path resolution and check the parsed input dict before processing when debugging overrides that are not taking effect
Files:
src/aiconfigurator/generator/api.py
tests/unit/generator/test_*.py
📄 CodeRabbit inference engine (.claude/rules/generator/testing.md)
Test individual functions in isolation using unit tests with mocked schema loading and template rendering
Files:
tests/unit/generator/test_fpm_artifacts.py
🪛 ast-grep (0.44.1)
tests/unit/generator/test_fpm_artifacts.py
[info] 25-31: 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)
[error] 191-197: 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] 285-292: 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] 297-304: 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)
🪛 markdownlint-cli2 (0.22.1)
docs/cli_user_guide.md
[warning] 18-18: Link fragments should be valid
(MD051, link-fragments)
🔇 Additional comments (16)
README.md (1)
102-104: LGTM!docs/cli_user_guide.md (2)
18-18: 📐 Maintainability & Code QualityVerify the in-page link target.
Please confirm the
#deployment-target-selectionfragment still resolves in the rendered markdown; markdownlint already flagged this line.Source: Linters/SAST tools
689-689: LGTM!Also applies to: 698-750, 998-998
docs/dynamo_deployment_guide.md (1)
3-3: LGTM!Also applies to: 488-513
docs/generator_overview.md (1)
13-13: LGTM!Also applies to: 96-96, 176-205
src/aiconfigurator/generator/main.py (1)
66-71: LGTM!Also applies to: 98-98
src/aiconfigurator/generator/api.py (2)
277-282: 📐 Maintainability & Code Quality | ⚡ Quick winDocstring lists
'fpm'but doesn't describe it.Every other option gets an explanatory clause ("...generates Kustomize overlays...");
'fpm'is added to the value list but has no matching description, unlikegenerate_naive_config's consistently terse style.📝 Suggested docstring addition
deployment_target: Deployment platform ('dynamo-j2', 'dynamo-python', 'llm-d-helm', 'llm-d-kustomize', or 'fpm'). 'dynamo-j2' uses typed Dynamo builders, 'dynamo-python' uses Dynamo's Python config modifiers, 'llm-d-helm' generates Helm values for llm-d-modelservice chart, and 'llm-d-kustomize' - generates Kustomize overlays for llm-d modelserver guides. + generates Kustomize overlays for llm-d modelserver guides. 'fpm' emits a reusable + keepalive resource Pod plus a standalone run.sh for FPM data collection.
318-318: LGTM!Also applies to: 777-777, 675-676
src/aiconfigurator/generator/artifacts.py (1)
19-19: LGTM!Also applies to: 44-44
src/aiconfigurator/generator/aggregators.py (1)
286-292: LGTM!src/aiconfigurator/generator/rendering/engine.py (1)
255-256: LGTM!Also applies to: 709-725
src/aiconfigurator/generator/builders/fpm_builder.py (2)
40-48: 🎯 Functional CorrectnessNo DynConfig key mismatch here
enable_router,router_mode,router_config, andplanner_configmatch theDynConfigschema and are already covered by the fail-fast guard.> Likely an incorrect or invalid review comment.
101-120: 📐 Maintainability & Code QualityNo action needed The
VllmWorkername is part of the current generator contract, so this check matches the rest of the pipeline.> Likely an incorrect or invalid review comment.tests/unit/cli/test_cli_workflow.py (1)
764-764: LGTM!tests/unit/generator/test_fpm_artifacts.py (2)
1-547: LGTM!
466-476: 🎯 Functional CorrectnessNo issue here.
K8sConfig.worker_extra_pod_specis a supported override path, andfpm_builderexplicitly rejectsmainContainer.envFrom, so this test exercises the intended case.> Likely an incorrect or invalid review comment.
| def _collect_concrete_env(worker: DGDService, main_container: MainContainer) -> list[tuple[str, str]]: | ||
| resolved: list[tuple[str, str]] = [] | ||
| entries = list(worker.envs or []) + list(main_container.env or []) | ||
| for entry in entries: | ||
| if not isinstance(entry, dict): | ||
| raise TypeError("FPM environment entries must be mappings") | ||
| if "valueFrom" in entry: | ||
| raise ValueError("FPM V1 does not support valueFrom environment entries") | ||
|
|
||
| name = entry.get("name") | ||
| if not isinstance(name, str) or not _ENV_NAME_RE.fullmatch(name): | ||
| raise ValueError(f"Invalid shell environment variable name: {name!r}") | ||
| if "value" not in entry or entry["value"] is None: | ||
| raise ValueError(f"Environment variable {name} must have a concrete value") | ||
|
|
||
| value = entry["value"] | ||
| if isinstance(value, bool): | ||
| resolved.append((name, "true" if value else "false")) | ||
| elif isinstance(value, (str, int, float)): | ||
| resolved.append((name, str(value))) | ||
| else: | ||
| raise TypeError(f"Environment variable {name} must have a scalar value") | ||
| return resolved |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "env_from_secret|envFromSecret" src/aiconfigurator/generator/builders/dgd_model.py src/aiconfigurator/generator/builders/k8s_builder.pyRepository: ai-dynamo/aiconfigurator
Length of output: 1071
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== generator-development rules ==\n'
sed -n '1,220p' .claude/rules/generator-development.md
printf '\n== dgd_model outline ==\n'
ast-grep outline src/aiconfigurator/generator/builders/dgd_model.py --view expanded
printf '\n== k8s_builder outline ==\n'
ast-grep outline src/aiconfigurator/generator/builders/k8s_builder.py --view expanded
printf '\n== fpm_builder around target lines ==\n'
sed -n '1,220p' src/aiconfigurator/generator/builders/fpm_builder.pyRepository: ai-dynamo/aiconfigurator
Length of output: 21100
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== fpm_builder lowering path ==\n'
rg -n "def _lower_worker_to_pod|env_from_secret|envFrom" src/aiconfigurator/generator/builders/fpm_builder.py src/aiconfigurator/generator/builders/k8s_builder.py src/aiconfigurator/generator/builders/dgd_model.py
printf '\n== fpm_builder relevant section ==\n'
sed -n '220,420p' src/aiconfigurator/generator/builders/fpm_builder.py
printf '\n== k8s_builder env handling section ==\n'
sed -n '90,150p' src/aiconfigurator/generator/builders/k8s_builder.py
printf '\n== dgd_model service serialization section ==\n'
sed -n '180,230p' src/aiconfigurator/generator/builders/dgd_model.pyRepository: ai-dynamo/aiconfigurator
Length of output: 12260
Reject envFromSecret for FPM V1 too. DGDService.envFromSecret is serialized on the worker service but never carried into the generated Pod or run.sh, so secret-backed env injection is silently lost and only fails later at runtime.
🛡️ Suggested guard
def _build_single_worker(context: dict[str, Any], backend: str, resolved_facts: Any) -> DGDService:
...
worker = workers[0][1]
+ if worker.env_from_secret:
+ raise ValueError("FPM V1 does not support DGDService.envFromSecret")
if worker.replicas != 1:
raise ValueError("FPM V1 requires worker replicas=1")🤖 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 130 - 152,
The _collect_concrete_env helper only validates worker.envs and
main_container.env, but FPM V1 also needs to reject envFromSecret on DGDService
because those secret-backed variables are not propagated into the generated Pod
or run.sh. Update the FPM builder path that assembles environment handling to
detect envFromSecret on the worker service and fail fast with a clear error,
alongside the existing valueFrom checks, so the validation is enforced in the
same code path as _collect_concrete_env.
AIC Accuracy Regression TestingWorkflow result: Old revision: Summary
Comparison summary table
|
Signed-off-by: etshen <etshen@nvidia.com>
Signed-off-by: etshen <etshen@nvidia.com>
e65a757 to
b679f40
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/aiconfigurator/generator/builders/fpm_builder.py`:
- Around line 731-768: The resource lowering in _lower_resources() updates
nvidia.com/gpu to gpu_limit but leaves custom resource values unchanged, so EFA
can still be over-requested per pod. Adjust the custom resource handling in
_lower_resources() to scale the EFA count in proportion to gpu_limit, keeping
the transport.yaml 1:1 GPU-to-EFA relationship intact, and preserve the existing
validation for resources.limits, resources.requests, and resources.custom.
🪄 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: 03534917-9b9b-4622-b920-76b900e283f3
📒 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
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (9)
- src/aiconfigurator/generator/aggregators.py
- src/aiconfigurator/generator/main.py
- src/aiconfigurator/generator/artifacts.py
- tests/unit/cli/test_cli_workflow.py
- src/aiconfigurator/generator/rendering/engine.py
- src/aiconfigurator/cli/main.py
- docs/dynamo_deployment_guide.md
- docs/generator_overview.md
- src/aiconfigurator/generator/api.py
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Build and Test (e2e)
- GitHub Check: Rust/Python engine-step parity
- GitHub Check: Cargo Deny
- GitHub Check: Build and Test (unit)
- GitHub Check: AIC Accuracy Regression Testing
🧰 Additional context used
📓 Path-based instructions (10)
src/aiconfigurator/generator/config/deployment_config.yaml
📄 CodeRabbit inference engine (.claude/rules/generator/config_schema.md)
src/aiconfigurator/generator/config/deployment_config.yaml: Add new parameters tosrc/aiconfigurator/generator/config/deployment_config.yamlwith proper YAML structure including key name, required status, default value, optional backend-specific defaults, and optional backend constraints
Ensure string defaults in YAML are double-quoted inside single quotes (e.g.,default: '"my-string"') to allow proper Jinja2 evaluation
Guard Jinja2 expressions in default values against None/empty values by using conditional logic (e.g.,'model_path.split("/")[-1] if model_path else ""') to prevent runtime errors
Avoid circular default references where parameter A defaults to an expression using parameter B and B defaults to an expression using A, as this produces silent None valuesCheck
src/aiconfigurator/generator/config/deployment_config.yamldefault expressions and evaluate them manually with test inputs when debugging missing or None parameter valuesInput schema and default values for backend template rendering must be defined in deployment_config.yaml
src/aiconfigurator/generator/config/deployment_config.yaml: Update deployment_config.yaml to add schema entries with section, default, and required status for new user-facing parameters, and add backend_defaults if behavior differs by backend
Verify that default image expressions in deployment_config.yaml resolve correctly for new backend versions and update container image references if the Dynamo release includes new images
Files:
src/aiconfigurator/generator/config/deployment_config.yaml
**/*.{yaml,yml,json}
📄 CodeRabbit inference engine (.claude/rules/generator/guard_rails.md)
**/*.{yaml,yml,json}:build_confignesting is version-dependent: Pre-1.2.0rc5 places it insidebuild_config. Post-1.2.0rc5 places it at top-level. Wrong placement is silently ignored.
Template variables must be top-level keys; nested paths likebuild_config.max_batch_sizeare undefined becauseengine.pyflattens the context.
cache_transceiver_configlifecycle: Absent pre-1.0.0rc4. In engine YAML 1.0.0rc4-1.3.0rc1. In CLI--override-engine-argspost-1.3.0rc5.
cache_transceiver_config.backendmust be set to'DEFAULT'or omission causes TRT-LLM to reject config in disagg mode.
Engine params must go into--override-engine-argsJSON; TRT-LLM's Dynamo argparser only accepts specific direct CLI flags.
max_num_tokens % tokens_per_block == 0must be true; TRT-LLM asserts block alignment and violation crashes engine startup.
cache_transceiver_max_tokens_in_buffermust align to block size; same block-alignment assertion applies asmax_num_tokens.
Prefill deployment: Setdisable_overlap_scheduler = trueto prevent hangs caused by overlap scheduler on prefill.
Decode/aggregation deployment: Setdisable_overlap_scheduler = falseto avoid performance degradation.
MoE models: EnsureTP = moe_tp * moe_epto prevent out-of-memory errors from model replication per GPU.
KV cache dtype: Convert literal"float16"or"bfloat16"to"auto"for TRT-LLM compatibility; literal strings are not accepted.
SGLang backend: Do NOT emit--moe-dense-tp-size; SGLang only accepts value 1 or None, other values crash startup.
SGLang backend: KV transfer backend default must be"nixl"for disaggregated mode (SGLang >=0.5.6.post2 requires explicit backend).
SGLang aggregation mode: Setenable_mixed_chunk = trueto avoid poor prefill/decode scheduling performance.
SGLang backend: Convert KV cache dtype"fp8"to"fp8_e4m3"; SGLang does not accept the literal"fp8"string.
SGLang backend: Convert KV cache dtype"bfloat16"to"auto"; SGLang interprets the li...
Files:
src/aiconfigurator/generator/config/deployment_config.yaml
src/aiconfigurator/generator/**
📄 CodeRabbit inference engine (AGENTS.md)
Before making any change under
src/aiconfigurator/generator/**, you must read.claude/rules/generator-development.mdfirst.
Files:
src/aiconfigurator/generator/config/deployment_config.yamlsrc/aiconfigurator/generator/builders/k8s_builder.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/builders/k8s_builder.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/builders/k8s_builder.pysrc/aiconfigurator/generator/builders/fpm_builder.pytests/unit/generator/test_fpm_artifacts.pydocs/cli_user_guide.md
src/aiconfigurator/generator/**/*.py
📄 CodeRabbit inference engine (.claude/rules/generator/debugging.md)
When debugging generator output issues, verify template variable names match the rendering context in
src/aiconfigurator/generator/rendering/engine.py:make_worker_context()
Files:
src/aiconfigurator/generator/builders/k8s_builder.pysrc/aiconfigurator/generator/builders/fpm_builder.py
**/*.py
📄 CodeRabbit inference engine (.claude/rules/generator/guard_rails.md)
**/*.py: Catch unsupported system/backend/version combinations early with fast validation to prevent wasting user time on late failures after profiling.
MoE model detection must useconfig.json, not model name patterns, to correctly identify MoE models with non-standard naming.
Usemodel_familynotmodel_namefor model compatibility checks;model_nameincludes size/quantization info that changes across variants.
safe_model_namemust be generated BEFORE saving results to avoid race condition with rawmodel_pathcontaining slashes.
Quantization type must be inferred from model config, not name patterns, to handle inconsistent naming conventions.
Files:
src/aiconfigurator/generator/builders/k8s_builder.pysrc/aiconfigurator/generator/builders/fpm_builder.pytests/unit/generator/test_fpm_artifacts.py
tests/unit/generator/test_*.py
📄 CodeRabbit inference engine (.claude/rules/generator/testing.md)
Test individual functions in isolation using unit tests with mocked schema loading and template rendering
Files:
tests/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/generator/test_fpm_artifacts.py
**/*.md
📄 CodeRabbit inference engine (.claude/rules/generator/config_schema.md)
**/*.md: Add computation rules in the rules authoring system when a new parameter requires non-trivial value computation beyond simple defaults
Add template modifications in the template authoring system when a new parameter requires special handling in generated artifacts
Files:
docs/cli_user_guide.md
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/cli_user_guide.md
🪛 ast-grep (0.44.1)
tests/unit/generator/test_fpm_artifacts.py
[info] 25-31: 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)
[error] 253-259: 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] 347-354: 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] 363-370: 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] 425-432: 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] 479-486: 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] 674-680: 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] 718-725: 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] 794-801: 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.22.1)
docs/cli_user_guide.md
[warning] 18-18: Link fragments should be valid
(MD051, link-fragments)
🔇 Additional comments (8)
docs/cli_user_guide.md (2)
18-18: LGTM!Also applies to: 689-689, 1018-1018
770-770: 📐 Maintainability & Code QualityNo change needed for the 0.24.0 flag note.
extra_cli_argsis an intentional raw passthrough here, and the doc already scopes those0.24.0-only flags as unvalidated against a0.24.0runtime image.> Likely an incorrect or invalid review comment.src/aiconfigurator/generator/builders/fpm_builder.py (3)
274-296:envFromSecretstill silently dropped for every FPM render.
_collect_concrete_envonly walksworker.envs+main_container.env; it never checksworker.env_from_secret. Sincek8s_builder.py'srender_workerunconditionally setsenv_from_secret="hf-token-secret"on every vLLM worker, this means every FPMrun.shtoday silently losesHF_TOKENinjection — not just an edge case.
329-347: Explicit empty--benchmark-output-pathstill diverges from the valuerun.shwaits on.
value = cli_value or env_valuetreats an explicit empty string as absent, and the laterif not value: raiseruns too late (afterargs/envare already mutated).
483-491: Engine termination is still unbounded in bothcleanup()and the happy-path shutdown.A stuck engine ignoring SIGTERM can hang the reusable Pod indefinitely for all future collection attempts, unlike the benchmark-wait loop which is correctly bounded by
wait_timeout_seconds.Also applies to: 527-529
src/aiconfigurator/generator/config/deployment_config.yaml (1)
131-135: LGTM!src/aiconfigurator/generator/builders/k8s_builder.py (1)
229-229: LGTM!Also applies to: 536-536, 830-830
tests/unit/generator/test_fpm_artifacts.py (1)
1-969: LGTM!
Signed-off-by: etshen <etshen@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/unit/generator/test_fpm_artifacts.py (1)
12-15: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove these contract tests out of the unit-test suite.
These ranges exercise the complete renderer and real Bash process trees rather than isolated functions. Move them and their helpers to the integration/contract suite, retaining mocked function-level coverage here.
A one-click change is unsafe because the destination and CI test-discovery configuration are unavailable.
As per coding guidelines,
tests/unit/generator/test_*.pymust “Test individual functions in isolation using unit tests with mocked schema loading and template rendering.” As per path instructions, broader multi-file changes should not use a suggested change.Also applies to: 300-342, 548-552, 628-628, 850-887, 1052-1053, 1072-1072
🤖 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 12 - 15, Move the contract-test ranges and their supporting helpers from tests/unit/generator/test_fpm_artifacts.py into the repository’s established integration/contract test suite, preserving mocked function-level coverage in the unit file. Update test-discovery or CI configuration only as needed to run the relocated tests, and verify the real renderer and Bash process-tree coverage remains included without treating it as isolated unit testing.Sources: Coding guidelines, Path instructions
🤖 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 `@tests/unit/generator/test_fpm_artifacts.py`:
- Around line 596-599: Update the run.sh rewrites in the test cases around
_render(params), including both occurrences near the 596 and 663 sections, to
assert that replacing engine_shutdown_grace_seconds=30 matched exactly once.
Preserve the rewritten value while making backend assignment drift fail
immediately with a clear assertion.
---
Nitpick comments:
In `@tests/unit/generator/test_fpm_artifacts.py`:
- Around line 12-15: Move the contract-test ranges and their supporting helpers
from tests/unit/generator/test_fpm_artifacts.py into the repository’s
established integration/contract test suite, preserving mocked function-level
coverage in the unit file. Update test-discovery or CI configuration only as
needed to run the relocated tests, and verify the real renderer and Bash
process-tree coverage remains included without treating it as isolated unit
testing.
🪄 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: 6be892f7-0949-4cf7-ae83-acce36d0d6bb
📒 Files selected for processing (4)
src/aiconfigurator/cli/main.pysrc/aiconfigurator/generator/builders/fpm_builder.pytests/unit/cli/test_cli_workflow.pytests/unit/generator/test_fpm_artifacts.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/aiconfigurator/generator/builders/fpm_builder.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Collect snapshot (old)
- GitHub Check: Collect snapshot (new)
- GitHub Check: Rust/Python engine-step parity
- GitHub Check: Build and Test (unit)
- GitHub Check: Build and Test (e2e)
- GitHub Check: Cargo Deny
🧰 Additional context used
📓 Path-based instructions (5)
**/*.py
📄 CodeRabbit inference engine (.claude/rules/generator/guard_rails.md)
**/*.py: Catch unsupported system/backend/version combinations early with fast validation to prevent wasting user time on late failures after profiling.
MoE model detection must useconfig.json, not model name patterns, to correctly identify MoE models with non-standard naming.
Usemodel_familynotmodel_namefor model compatibility checks;model_nameincludes size/quantization info that changes across variants.
safe_model_namemust be generated BEFORE saving results to avoid race condition with rawmodel_pathcontaining slashes.
Quantization type must be inferred from model config, not name patterns, to handle inconsistent naming conventions.
Files:
src/aiconfigurator/cli/main.pytests/unit/cli/test_cli_workflow.pytests/unit/generator/test_fpm_artifacts.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/cli/main.pytests/unit/cli/test_cli_workflow.pytests/unit/generator/test_fpm_artifacts.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
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/test_*.py
📄 CodeRabbit inference engine (.claude/rules/generator/testing.md)
Test individual functions in isolation using unit tests with mocked schema loading and template rendering
Files:
tests/unit/generator/test_fpm_artifacts.py
🪛 ast-grep (0.44.1)
tests/unit/generator/test_fpm_artifacts.py
[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] 676-683: 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)
🔇 Additional comments (8)
src/aiconfigurator/cli/main.py (4)
140-144: LGTM!
2307-2323: LGTM!
2370-2371: LGTM!
2439-2440: LGTM!tests/unit/cli/test_cli_workflow.py (4)
25-25: LGTM!
199-243: LGTM!
263-282: LGTM!
830-830: LGTM!
| args.extend(extra_cli_args) | ||
|
|
||
| env = _collect_concrete_env(worker, main_container) | ||
| _require_cli_option(args, "--benchmark-mode", expected="agg") |
There was a problem hiding this comment.
To avoid OOM during data collection, we switched to disaggregated "prefill" and "decode".
Signed-off-by: etshen <etshen@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/generator/test_fpm_artifacts.py (2)
121-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDefault
point_typesformode="agg"doesn't match real agg output shape.
point_types = ["prefill" if mode == "agg" else mode]yields a single"prefill"point for agg, but the only agg usage in this suite (line 539) explicitly passes["prefill", "decode"]. If a future test calls_benchmark_result(mode="agg")without an explicitpoint_types, it'll silently produce an incomplete/wrong payload.♻️ Proposed fix
if point_types is None: - point_types = ["prefill" if mode == "agg" else mode] + point_types = ["prefill", "decode"] if mode == "agg" else [mode]🤖 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 121 - 122, Update the defaulting logic in _benchmark_result so mode="agg" uses both "prefill" and "decode" point types, matching the explicitly configured aggregate output shape; retain the existing single-type defaults for other modes.
1322-1341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
_set_benchmark_modeinstead of re-deriving the index lookup.Both tests manually reimplement the same
args.index("--benchmark-mode")+ assignment already provided by the helper.♻️ Proposed fix
def test_fpm_accepts_supported_benchmark_modes(benchmark_mode): params = _params() - args = params["params"]["agg"]["extra_cli_args"] - index = args.index("--benchmark-mode") - args[index + 1] = benchmark_mode + _set_benchmark_mode(params, benchmark_mode) artifacts = _render(params)def test_fpm_rejects_unsupported_benchmark_mode(): params = _params() - args = params["params"]["agg"]["extra_cli_args"] - index = args.index("--benchmark-mode") - args[index + 1] = "disagg" + _set_benchmark_mode(params, "disagg") with pytest.raises(ValueError, match="agg, prefill, decode"):🤖 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 1322 - 1341, Update both benchmark-mode tests to use the existing _set_benchmark_mode helper for applying the parameter instead of manually locating --benchmark-mode and assigning the following argument. Pass each test’s benchmark mode to the helper while preserving the current artifact and ValueError assertions.
🤖 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.
Nitpick comments:
In `@tests/unit/generator/test_fpm_artifacts.py`:
- Around line 121-122: Update the defaulting logic in _benchmark_result so
mode="agg" uses both "prefill" and "decode" point types, matching the explicitly
configured aggregate output shape; retain the existing single-type defaults for
other modes.
- Around line 1322-1341: Update both benchmark-mode tests to use the existing
_set_benchmark_mode helper for applying the parameter instead of manually
locating --benchmark-mode and assigning the following argument. Pass each test’s
benchmark mode to the helper while preserving the current artifact and
ValueError assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c4da3201-c3c7-46be-b11a-0db26707459d
📒 Files selected for processing (5)
docs/cli_user_guide.mddocs/dynamo_deployment_guide.mddocs/generator_overview.mdsrc/aiconfigurator/generator/builders/fpm_builder.pytests/unit/generator/test_fpm_artifacts.py
🚧 Files skipped from review as they are similar to previous changes (3)
- docs/cli_user_guide.md
- docs/generator_overview.md
- src/aiconfigurator/generator/builders/fpm_builder.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (9)
**/*
📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)
**/*: Treat this file as the only always-injected rule file; do not add new always-on rules here without human approval. New rule files must includepaths:frontmatter.
When reviewing changes in a governed area, read that area's rule files first, even if path-based auto-loading does not occur during a read-only review. Rule violations are review findings even when the code works.
A task must remain within its module: collector tasks may change onlycollector/and its tests, generator tasks onlysrc/aiconfigurator/generator/and its tests, and SDK tasks must not modify either. Cross-module contract changes require explicit human approval.
Files:
docs/dynamo_deployment_guide.mdtests/unit/generator/test_fpm_artifacts.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:
docs/dynamo_deployment_guide.mdtests/unit/generator/test_fpm_artifacts.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.md
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/**/*.{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/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
**/*.{py,pyi}
📄 CodeRabbit inference engine (AGENTS.md)
Run
ruff check .andruff format --check .to validate Python linting and formatting.
Files:
tests/unit/generator/test_fpm_artifacts.py
tests/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Run unit tests with
pytest -m unit; usepytest -m "unit or build"for the build-test subset when required data is available.
Files:
tests/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/generator/test_fpm_artifacts.py
🪛 ast-grep (0.44.1)
tests/unit/generator/test_fpm_artifacts.py
[error] 182-189: 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)
[info] 162-162: use jsonify instead of json.dumps for JSON output
Context: json.dumps(result)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🔇 Additional comments (8)
tests/unit/generator/test_fpm_artifacts.py (6)
690-769: Shutdown-grace assertion fix confirmed applied.Both rewrites of
engine_shutdown_grace_seconds=30are now preceded byassert script.count(...) == 1, addressing the earlier review concern about a silently-no-op.replace()masking backend drift.Also applies to: 772-839
386-424: Zombie-aware process check and bounded-wait cleanup look solid.
_process_is_runningcorrectly treats zombie/dead states as "not running" via/proc/<pid>/status, and_assert_process_stoppedpolls with a deadline before force-killing and failing — good hygiene for these subprocess tests.
534-598: Good schema-v1 acceptance/rejection coverage.The parametrized accept-cases (prefill/decode/agg point types) and the reject-cases (schema version, mode mismatch, coverage, result count, point type, dp_rank, non-object payload) give solid edge-case coverage of the runtime validation contract, satisfying the "Test generator edge cases including... benchmark mode" expectation.
Also applies to: 671-680
600-668: DP wait/local-range logic and multinode arg assertions look correct.Rank-file naming (
base.stem/base.suffix), localdata-parallel-size-local/start-rankderivation, and glob-based output verification are all internally consistent with the fake engine's writes.Also applies to: 1125-1207
113-122: 🎯 Functional Correctness
list[str] | Noneis fine here. The repo targets Python 3.10+, and this file already enablesfrom __future__ import annotations, so the annotation won’t break imports.> Likely an incorrect or invalid review comment.
146-192: 🚀 Performance & ScalabilityNo marker change needed These subprocess-based
run.shtests are already underpytest.mark.unit, andunitis defined to include lightweight integration tests.docs/dynamo_deployment_guide.md (2)
489-501: Section 6 content checks out against the FPM test suite.Artifact list, Pod/LeaderWorkerSet split,
extra_cli_args/extra_envinjection points, and the--benchmark-modecontract all match the behavior exercised intests/unit/generator/test_fpm_artifacts.py.
513-513: 📐 Maintainability & Code QualityCross-reference is valid.
cli_user_guide.md#fpm-v1-resource-workload-and-run-scriptexists, so there’s nothing to fix here.> Likely an incorrect or invalid review comment.
3 participants
Overview:
This PR adds a new fpm deployment target to generator.
The target reuses the existing Generator rule, mapping, hardware-facts, and versioned-template pipeline, then emits a reusable Kubernetes resource Pod and a separate vLLM launch script for FPM data collection. The resource Pod can be created once and reused across multiple collection attempts. Each generated run.sh starts and stops its own Engine, so reusing the Pod avoids repeated scheduling and image setup, but does not keep the Engine or model resident in GPU memory. FPM V1 intentionally supports only vLLM aggregated deployments with one worker, one replica, and one node. Existing deployment targets are unchanged.
Details:
Usage
Add FPM-specific vLLM arguments as individual tokens under
Workers.agg.extra_cli_args, and add concrete environment variables underK8sConfig.extra_env:Generate the artifacts with:
The runtime image can also be overridden directly from the command line:
extra_cli_argsmust be alist[str]. The Generator preserves token boundaries and shell-quotes the final command.extra_envaccepts concrete{name, value}entries only.--benchmark-mode aggis required. The benchmark output path may be supplied through the CLI argument, the environment variable, or neither; the Generator fills in the missing side and defaults to/results/benchmark.json. If both are supplied, they must match.Generated artifacts
The FPM target emits exactly two artifacts:
k8s_deploy.yamlis a standard keepalive Pod. It contains the resolved runtime image, GPU count, volumes,/results,/dev/shm, node selectors, tolerations, and other infrastructure settings. It does not contain the vLLM command, Engine arguments, or FPM environment variables.run.shcontains concrete environment exports and the complete resolvedpython3 -m dynamo.vllm ...command. The base command still comes from the existing rules, mappings, hardware facts, and versioned templates; FPM-specificextra_cli_argsare appended afterward. The script starts the Engine, waits for valid benchmark JSON, stops the Engine, and refuses to overwrite an existing output file.A typical Agent workflow is:
The Pod may be reused, but every
run.shexecution starts a new Engine and reloads the model. Each collection attempt must therefore use a unique benchmark output path.The referenced scheduler module and FPM input files, such as case, capacity, and run-spec files, must already exist in the selected runtime image or be provided through mounted storage. The Generator passes their paths through but does not create those files.
valueFrom,envFrom, router/planner configurations, disaggregated mode, multiple workers, and multinode workers fail fast in FPM V1.Summary by CodeRabbit
fpmdeployment target that generates a reusable Kubernetes workload and a runnablerun.sh.--deployment-targetoptions across the CLI and artifact rendering (dynamo-j2,dynamo-python,llm-d-helm,llm-d-kustomize,fpm).fpmrun script output naming consistent with the selected target behavior.fpmconstraints/output behavior.fpmartifact generation and CLI validation.