feature: FPM(ForwardPassMetrics) collector#1365
Conversation
WalkthroughChangesFPM Forward Campaign
Estimated code review effort: 5 (Critical) | ~120 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: 19
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (4)
docs/cli_user_guide.md-18-18 (1)
18-18: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the invalid section-link fragment.
Markdownlint reports MD051 for
#deployment-target-selection. Update it to the exact slug generated by the target heading. A one-click change is unsafe because the heading text is outside this selected range.🤖 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 reference in the CLI guide to use the exact Markdown anchor generated by the corresponding target-selection heading, resolving the MD051 invalid-fragment warning. Locate that heading before choosing the replacement slug; change only the link fragment and preserve the surrounding option documentation.Source: Linters/SAST tools
src/aiconfigurator/generator/builders/fpm_builder.py-394-400 (1)
394-400: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the required
/bin/bashshebang.As per coding guidelines, generated run scripts must use
#!/bin/bash.Proposed fix
- "#!/usr/bin/env bash", + "#!/bin/bash",🤖 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 394 - 400, Update the generated script header in the lines list of the FPM builder to use the required /bin/bash shebang instead of the env-based bash shebang. Preserve the remaining shell options and ulimit commands unchanged.Source: Coding guidelines
tests/unit/generator/test_fpm_artifacts.py-430-437 (1)
430-437: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winClean up timed-out fake-engine subprocesses here.
subprocess.run(..., timeout=...)only kills the bash wrapper, so the background fake engine can survive a timeout and leak into later tests. Use a sharedPopen(start_new_session=True)helper and kill the process group infinally; the same pattern applies to the other timeout-based invocations in this file.🤖 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 timeout-based subprocess.run invocations in the tests with a shared Popen helper using start_new_session=True, including the invocation around completed. Ensure the helper kills the entire process group in a finally block when execution times out, and apply the same cleanup pattern to every other timeout-based invocation in this file.collector/fpm_forward/config.py-312-331 (1)
312-331: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject deployment flags when FPM is not selected.
This guard omits every option from
add_fpm_generator_arguments(). Because registration uses a raw argv token scan, unrelated values equal tofpm_forwardcan expose these flags and let normal collection silently ignore them.Proposed fix
"fpm_database_root", "fpm_smoke_points", + "generator_config", + "generator_set", + "generated_config_version", + "generator_dynamo_version", + "namespace", + "model_cache", + "transport", + "image_pull_secret",🤖 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 `@collector/fpm_forward/config.py` around lines 312 - 331, Update the deployment-flag guard’s name list to include every option registered by add_fpm_generator_arguments(), including any omitted FPM generator arguments. Ensure the raw argv scan rejects those flags when FPM is not selected, even when unrelated argument values equal fpm_forward.
🤖 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 `@collector/fpm_forward/capabilities.py`:
- Around line 233-262: Remove the runtime support-gating failures in the
mode-resolution flow around _require_mode, context_modes, generation_modes, and
unsupported_kv so missing AIC database evidence does not abort collection.
Preserve the checkpoint-resolved FMHA and KV-cache modes without silently
replacing FMHA with bfloat16, and allow unavailable memory estimates to proceed
to explicit runtime verification. Ensure version incompatibilities and runtime
collection failures are observed and classified as non-fatal errors.
- Around line 198-209: Remove the model_family and bootstrap_template fallback
branches from the capability resolution flow around support_level, source_name,
attention_kind, and template_id. Require an exact declared collector model case;
when none resolves, fail loudly through the existing
unsupported/unresolvable-declaration path rather than synthesizing family or
generic template identities.
- Line 180: Update the is_moe assignment near _attention_template so
exact_source "mla_module" is not automatically classified as MoE; retain only
the intended MoE detection and the dsa_module exception. Ensure dense_mla
checkpoints can reach the _attention_template() branch without requiring MoE
database or topology support.
In `@collector/fpm_forward/capacity.py`:
- Around line 304-321: Update the exception handling around the build calls in
the FPM capacity-model flow to catch only the estimator’s explicitly documented
unsupported-model exception. Preserve the existing bootstrap fallback and
non-bootstrap ValueError for that exception, while allowing malformed
breakdowns, API regressions, and other collector failures to propagate
unchanged; use the estimator’s actual exception symbol and contract rather than
a broad Exception catch.
- Around line 766-777: Update the topology admission logic around the
CapacityDecision construction to validate that estimate["total_kv_size_tokens"]
is greater than zero before appending the topology to admitted. Reject
non-positive values with the existing rejection/decision handling, and preserve
the current admission path for positive KV capacity.
In `@collector/fpm_forward/database.py`:
- Around line 273-310: Serialize the full Parquet read/merge/write transaction
around the existing merge logic, including metadata generation, both writes, and
os.replace calls, using an inter-process lock. Replace the fixed .tmp filenames
derived from parquet_path and metadata_path with unique temporary files so
concurrent collectors cannot collide.
In `@collector/fpm_forward/entry.py`:
- Around line 105-111: Update the --model-cache parsing in the argument-handling
flow to reject values containing exactly two non-empty colon-separated parts
(NAME:MOUNT). Accept only NAME or NAME:MOUNT:SUBPATH, validating the format
before populating k8s fields so unsupported inputs fail early and runner.py
receives mount and model subpath together.
In `@collector/fpm_forward/population.py`:
- Around line 223-261: Replace the hardcoded token limits, prefix anchors, and
sequence-dependent decode batch predicates in the population-building flow with
workloads derived from the declared DSV4 cases exposed by case_generator.
Preserve the prefill/decode point generation while sourcing all shape
correlations through the authoritative model YAML/base-op case definitions; if
the current case-generator API cannot express these correlations, extend the
mechanism and obtain approval rather than adding collector-side predicates.
In `@collector/fpm_forward/runner.py`:
- Around line 815-819: Before creating the artifact directory or writing
collection-plan.json in the runner flow, canonicalize the active generator
overrides using the shared planner/runner-owned canonicalization helper, compute
their SHA-256, and compare it with plan.generator_config_sha256. Reject the run
on mismatch so artifacts cannot be published under an incorrect plan hash, while
preserving the existing root and artifact-writing behavior for matching
configurations.
- Around line 976-990: Update the cleanup exception handling in the runner’s
finally block so a cell whose resource.cleanup() fails cannot remain marked as
passed. Propagate the cleanup failure into the cell’s final status or include
cleanup_error in the final success gate, ensuring --resume does not skip it and
publish the database as successful.
- Around line 162-191: Update Runner.apply to check applied.returncode
immediately after the _kubectl("apply", ...) call and raise a RuntimeError on
failure, before querying or accepting any existing resource; preserve the
existing diagnostic details and successful apply flow.
- Around line 351-360: Clear the existing pod_root contents before invoking
_kubectl("cp", ...) in the per-pod collection flow, so retries start from an
empty results directory and _file_manifest(pod_root) only sees files from the
current copy. Preserve the existing directory creation and copy behavior.
In `@collector/fpm_forward/runtime/run_with_etcd.sh`:
- Around line 1-2: Add the repository-required SPDX license and copyright header
at the top of run_with_etcd.sh, before the shebang, using the project’s standard
header format.
In `@collector/fpm_forward/runtime/vllm_scheduler.py`:
- Around line 235-248: Preserve the remaining planned grid when a point fails:
update _bench_skip_point() and the graph-aware _bench_save_current_point() path
so they do not clear _fpm_pending_execution_meta or _bench_grid after a canary
miss. Also adjust _capacity() to avoid pruning cases during grid construction,
allowing every planned point to execute or report its failure before selection
in the eligible/capacity_sufficient flow.
In `@collector/fpm_forward/sampling.py`:
- Around line 336-341: Update the budget count calculation around the
budget_counts construction so every budget, including "full", is clamped to the
admitted population size len(ordered), not len(unique). Preserve the existing
minimum anchor behavior while ensuring one_eighth, one_quarter, one_half, and
full remain monotonic and never exceed the admitted points.
In `@collector/fpm_forward/types.py`:
- Around line 20-26: Update __post_init__ to reject decode points with
prefix_length == 0, while preserving the existing validation for prefill points
and other fields. Ensure the constructor enforces the runtime requirement that
decode points have a positive prefix_length before sampling can occur.
In `@src/aiconfigurator/cli/main.py`:
- Around line 140-144: Update build_default_tasks and the related
deployment-target validation so default mode with --deployment-target fpm
remains reachable when total_gpus >= 2, without allowing unsupported task
combinations. Preserve existing behavior for other targets, and add a cli_main
regression test covering a successful multi-GPU default invocation with the fpm
target.
In `@src/aiconfigurator/generator/builders/fpm_builder.py`:
- Around line 170-183: Preserve the multinode compute-domain manifest chain from
build_dgd() through build_fpm_artifacts(), either by emitting the ComputeDomain
as a second YAML document or explicitly implementing the alternate reservation
path instead of relying on stripped resourceClaims in _lower_worker_pod_spec()
and _lower_resources(). Also update the generated script shebang in
build_fpm_artifacts() to use /bin/bash.
In `@tests/unit/collector/test_fpm_runtime_adapter.py`:
- Around line 537-551: The canary-failure path around
scheduler._bench_skip_point must not silently drain queued benchmark points;
make the runtime raise a classified terminal error for remaining queued cases
while recording only the failed canary. Update
test_integrated_canary_failure_aborts_remaining_grid and the related test at the
referenced section to assert the error type and preserve the queued cases’
traceability according to the execute-or-raise contract.
---
Minor comments:
In `@collector/fpm_forward/config.py`:
- Around line 312-331: Update the deployment-flag guard’s name list to include
every option registered by add_fpm_generator_arguments(), including any omitted
FPM generator arguments. Ensure the raw argv scan rejects those flags when FPM
is not selected, even when unrelated argument values equal fpm_forward.
In `@docs/cli_user_guide.md`:
- Line 18: Update the deployment-target reference in the CLI guide to use the
exact Markdown anchor generated by the corresponding target-selection heading,
resolving the MD051 invalid-fragment warning. Locate that heading before
choosing the replacement slug; change only the link fragment and preserve the
surrounding option documentation.
In `@src/aiconfigurator/generator/builders/fpm_builder.py`:
- Around line 394-400: Update the generated script header in the lines list of
the FPM builder to use the required /bin/bash shebang instead of the env-based
bash shebang. Preserve the remaining shell options and ulimit commands
unchanged.
In `@tests/unit/generator/test_fpm_artifacts.py`:
- Around line 430-437: Replace the timeout-based subprocess.run invocations in
the tests with a shared Popen helper using start_new_session=True, including the
invocation around completed. Ensure the helper kills the entire process group in
a finally block when execution times out, and apply the same cleanup pattern to
every other timeout-based invocation in this file.
🪄 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: 2aa805ca-3e98-44de-9835-e23b2339dfcc
📒 Files selected for processing (39)
README.mdcollector/README.mdcollector/collect.pycollector/fpm_forward/__init__.pycollector/fpm_forward/capabilities.pycollector/fpm_forward/capacity.pycollector/fpm_forward/config.pycollector/fpm_forward/database.pycollector/fpm_forward/entry.pycollector/fpm_forward/planner.pycollector/fpm_forward/population.pycollector/fpm_forward/runner.pycollector/fpm_forward/runtime/__init__.pycollector/fpm_forward/runtime/install_overlay.pycollector/fpm_forward/runtime/preflight.pycollector/fpm_forward/runtime/run_with_etcd.shcollector/fpm_forward/runtime/vllm_scheduler.pycollector/fpm_forward/sampling.pycollector/fpm_forward/topology.pycollector/fpm_forward/types.pydocs/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/collector/test_fpm_forward.pytests/unit/collector/test_fpm_forward_cli_isolation.pytests/unit/collector/test_fpm_runner.pytests/unit/collector/test_fpm_runtime_adapter.pytests/unit/collector/test_vllm_dsv4_attn.pytests/unit/generator/test_fpm_artifacts.py
| architecture = _architecture(config, model_architecture) | ||
| model_family = common.ARCHITECTURE_TO_MODEL_FAMILY.get(architecture) if architecture else None | ||
| exact_source = resolve_attention_source(selected_ops, required=False) | ||
| is_moe = _is_moe(config) or exact_source in {"dsa_module", "mla_module"} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not classify every MLA checkpoint as MoE.
_attention_template() supports dense_mla, but this assignment makes that branch unreachable for exact MLA model cases and incorrectly requires MoE database/topology support.
Proposed fix
- is_moe = _is_moe(config) or exact_source in {"dsa_module", "mla_module"}
+ is_moe = _is_moe(config) or exact_source == "dsa_module"📝 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.
| is_moe = _is_moe(config) or exact_source in {"dsa_module", "mla_module"} | |
| is_moe = _is_moe(config) or exact_source == "dsa_module" |
🤖 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 `@collector/fpm_forward/capabilities.py` at line 180, Update the is_moe
assignment near _attention_template so exact_source "mla_module" is not
automatically classified as MoE; retain only the intended MoE detection and the
dsa_module exception. Ensure dense_mla checkpoints can reach the
_attention_template() branch without requiring MoE database or topology support.
| elif model_family: | ||
| support_level = "family_template" | ||
| source_name = template_source | ||
| attention_kind = template_kind | ||
| template_id = f"aic_family:{model_family.lower()}:{template_kind}" | ||
| support_reason = "no exact collector model case; derived from the registered AIC model family" | ||
| else: | ||
| support_level = "bootstrap_template" | ||
| source_name = template_source | ||
| attention_kind = template_kind | ||
| template_id = f"generic:{template_kind}" | ||
| support_reason = "architecture is not registered by AIC; using a conservative config-derived template" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not synthesize collector templates for undeclared models.
The family/bootstrap branches replace missing model-case declarations with inferred workload identities, allowing formally published data for shapes that were never declared by the model case layer. Require an exact declaration or propose an approved generator mechanism change; a one-click fix is unsafe because planning, tests, and documentation must change together.
As per path instructions, model case YAML owns structural shapes and activation, and unresolvable declarations must fail loudly rather than use fallback identities.
🤖 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 `@collector/fpm_forward/capabilities.py` around lines 198 - 209, Remove the
model_family and bootstrap_template fallback branches from the capability
resolution flow around support_level, source_name, attention_kind, and
template_id. Require an exact declared collector model case; when none resolves,
fail loudly through the existing unsupported/unresolvable-declaration path
rather than synthesizing family or generic template identities.
Source: Path instructions
| _require_mode(supported, "gemm", gemm) | ||
| if is_moe: | ||
| _require_mode(supported, "moe", moe) | ||
|
|
||
| context_modes = _supported_modes(supported, context_op) | ||
| if not context_modes: | ||
| raise ValueError(f"AIC database has no context-attention dtype evidence for template op {context_op!r}") | ||
| if inferred_fmha in context_modes: | ||
| fmha = inferred_fmha | ||
| fmha_resolution = "checkpoint_native" | ||
| elif "bfloat16" in context_modes: | ||
| fmha = "bfloat16" | ||
| fmha_resolution = f"aic_data_fallback_from_{inferred_fmha}" | ||
| else: | ||
| raise ValueError( | ||
| f"AIC database cannot resolve FMHA dtype {inferred_fmha!r} for {context_op}; " | ||
| f"supported modes: {sorted(context_modes)}" | ||
| ) | ||
|
|
||
| generation_modes = _supported_modes(supported, generation_op) | ||
| if not generation_modes: | ||
| raise ValueError(f"AIC database has no generation-attention dtype evidence for template op {generation_op!r}") | ||
| requested_kv = tuple(dict.fromkeys(_normalize_kv_dtype(value) for value in requested_kv_cache_dtypes)) | ||
| resolved_kv = tuple(dict.fromkeys(native_kv if value == "auto" else value for value in requested_kv)) | ||
| unsupported_kv = [value for value in resolved_kv if value not in generation_modes] | ||
| if unsupported_kv: | ||
| raise ValueError( | ||
| f"AIC database does not support KV-cache dtype(s) {unsupported_kv} for {generation_op}; " | ||
| f"supported modes: {sorted(generation_modes)}" | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not use the existing performance database as a runtime support gate.
Missing database evidence may represent the new mode this campaign needs to collect. These checks abort before runtime evidence is recorded, while the FMHA branch silently changes the inferred mode to bfloat16. Preserve the checkpoint-resolved modes and route unavailable memory estimates through explicit runtime verification instead.
Based on learnings, version incompatibilities should surface as non-fatal runtime collection errors rather than aggressive preventive gating. As per path instructions, collector failures must be observed and classified rather than predicted.
🤖 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 `@collector/fpm_forward/capabilities.py` around lines 233 - 262, Remove the
runtime support-gating failures in the mode-resolution flow around
_require_mode, context_modes, generation_modes, and unsupported_kv so missing
AIC database evidence does not abort collection. Preserve the
checkpoint-resolved FMHA and KV-cache modes without silently replacing FMHA with
bfloat16, and allow unavailable memory estimates to proceed to explicit runtime
verification. Ensure version incompatibilities and runtime collection failures
are observed and classified as non-fatal errors.
Sources: Path instructions, Learnings
| try: | ||
| baseline: dict[str, Any] = build(1).breakdown | ||
| high: dict[str, Any] = build(max(1, max_num_tokens_hint)).breakdown | ||
| except Exception as error: | ||
| if capability.support_level != "bootstrap_template": | ||
| raise ValueError( | ||
| f"AIC cannot build a reusable memory model for topology={topology.to_dict()}: {error}" | ||
| ) from error | ||
| return FPMCapacityModel( | ||
| total_gpu_capacity_bytes=capacity_bytes, | ||
| fixed_non_activation_bytes=None, | ||
| activation_floor_bytes=None, | ||
| activation_bytes_per_token=None, | ||
| tokens_from_kv_bytes=None, | ||
| kv_bytes_for_sequence=None, | ||
| source="bootstrap_runtime_verified", | ||
| error=str(error), | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Do not convert arbitrary estimator failures into bootstrap fallback.
Catching Exception also masks malformed breakdowns, API regressions, and collector bugs as “runtime verification required.” Narrow this to the estimator’s explicit unsupported-model failure and propagate everything else. A one-click change is unsafe without the estimator’s exception contract.
As per path instructions, proven collector-code failures must be surfaced and fixed rather than hidden through fallback behavior.
🤖 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 `@collector/fpm_forward/capacity.py` around lines 304 - 321, Update the
exception handling around the build calls in the FPM capacity-model flow to
catch only the estimator’s explicitly documented unsupported-model exception.
Preserve the existing bootstrap fallback and non-bootstrap ValueError for that
exception, while allowing malformed breakdowns, API regressions, and other
collector failures to propagate unchanged; use the estimator’s actual exception
symbol and contract rather than a broad Exception catch.
Source: Path instructions
| kv_tokens = int(estimate["total_kv_size_tokens"]) | ||
| source = str(estimate["source"]) | ||
| decisions.append( | ||
| CapacityDecision( | ||
| topology, | ||
| True, | ||
| source, | ||
| "rank-local model state leaves a positive baseline KV budget", | ||
| kv_tokens, | ||
| ) | ||
| ) | ||
| admitted.append(topology) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject topologies with no positive KV capacity.
The method promises positive baseline KV capacity but admits total_kv_size_tokens <= 0.
Proposed fix
kv_tokens = int(estimate["total_kv_size_tokens"])
source = str(estimate["source"])
+ if kv_tokens <= 0:
+ decisions.append(
+ CapacityDecision(
+ topology,
+ False,
+ source,
+ "rank-local model state leaves no positive baseline KV budget",
+ kv_tokens,
+ )
+ )
+ continue
decisions.append(📝 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.
| kv_tokens = int(estimate["total_kv_size_tokens"]) | |
| source = str(estimate["source"]) | |
| decisions.append( | |
| CapacityDecision( | |
| topology, | |
| True, | |
| source, | |
| "rank-local model state leaves a positive baseline KV budget", | |
| kv_tokens, | |
| ) | |
| ) | |
| admitted.append(topology) | |
| kv_tokens = int(estimate["total_kv_size_tokens"]) | |
| source = str(estimate["source"]) | |
| if kv_tokens <= 0: | |
| decisions.append( | |
| CapacityDecision( | |
| topology, | |
| False, | |
| source, | |
| "rank-local model state leaves no positive baseline KV budget", | |
| kv_tokens, | |
| ) | |
| ) | |
| continue | |
| decisions.append( | |
| CapacityDecision( | |
| topology, | |
| True, | |
| source, | |
| "rank-local model state leaves a positive baseline KV budget", | |
| kv_tokens, | |
| ) | |
| ) | |
| admitted.append(topology) |
🤖 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 `@collector/fpm_forward/capacity.py` around lines 766 - 777, Update the
topology admission logic around the CapacityDecision construction to validate
that estimate["total_kv_size_tokens"] is greater than zero before appending the
topology to admitted. Reject non-positive values with the existing
rejection/decision handling, and preserve the current admission path for
positive KV capacity.
| budget_counts = { | ||
| name: max(len(anchors), math.ceil(len(unique) / divisor)) for name, divisor in _BUDGET_DIVISORS.items() | ||
| } | ||
| budget_counts["full"] = len(ordered) | ||
| for name in ("one_eighth", "one_quarter", "one_half", "full"): | ||
| budget_counts[name] = min(budget_counts[name], len(unique)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clamp every budget to the admitted population.
When admission excludes many points, partial budgets remain based on len(unique) while full becomes len(ordered). For example, eight inputs with one admitted point produce non-monotonic counts 1, 2, 4, 1.
Proposed fix
budget_counts["full"] = len(ordered)
for name in ("one_eighth", "one_quarter", "one_half", "full"):
- budget_counts[name] = min(budget_counts[name], len(unique))
+ budget_counts[name] = min(budget_counts[name], len(ordered))The supplied unit test treats these counts as a monotonic nested-budget invariant.
📝 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.
| budget_counts = { | |
| name: max(len(anchors), math.ceil(len(unique) / divisor)) for name, divisor in _BUDGET_DIVISORS.items() | |
| } | |
| budget_counts["full"] = len(ordered) | |
| for name in ("one_eighth", "one_quarter", "one_half", "full"): | |
| budget_counts[name] = min(budget_counts[name], len(unique)) | |
| budget_counts = { | |
| name: max(len(anchors), math.ceil(len(unique) / divisor)) for name, divisor in _BUDGET_DIVISORS.items() | |
| } | |
| budget_counts["full"] = len(ordered) | |
| for name in ("one_eighth", "one_quarter", "one_half", "full"): | |
| budget_counts[name] = min(budget_counts[name], len(ordered)) |
🤖 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 `@collector/fpm_forward/sampling.py` around lines 336 - 341, Update the budget
count calculation around the budget_counts construction so every budget,
including "full", is clamped to the admitted population size len(ordered), not
len(unique). Preserve the existing minimum anchor behavior while ensuring
one_eighth, one_quarter, one_half, and full remain monotonic and never exceed
the admitted points.
| def __post_init__(self) -> None: | ||
| if self.workload_kind not in {"prefill", "decode"}: | ||
| raise ValueError(f"unknown workload_kind: {self.workload_kind}") | ||
| if self.batch_size < 1 or self.suffix_length < 1 or self.prefix_length < 0: | ||
| raise ValueError(f"invalid FPM point: {self}") | ||
| if self.workload_kind == "decode" and self.suffix_length != 1: | ||
| raise ValueError("decode points must execute exactly one new token per request") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Reject zero-prefix decode points at construction.
Decode points currently allow prefix_length=0; this later causes math.log2(0) in sampling and conflicts with the runtime’s prefix_length>0 requirement.
Proposed fix
if self.workload_kind == "decode" and self.suffix_length != 1:
raise ValueError("decode points must execute exactly one new token per request")
+ if self.workload_kind == "decode" and self.prefix_length < 1:
+ raise ValueError("decode points require a non-empty prefix")📝 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.
| def __post_init__(self) -> None: | |
| if self.workload_kind not in {"prefill", "decode"}: | |
| raise ValueError(f"unknown workload_kind: {self.workload_kind}") | |
| if self.batch_size < 1 or self.suffix_length < 1 or self.prefix_length < 0: | |
| raise ValueError(f"invalid FPM point: {self}") | |
| if self.workload_kind == "decode" and self.suffix_length != 1: | |
| raise ValueError("decode points must execute exactly one new token per request") | |
| def __post_init__(self) -> None: | |
| if self.workload_kind not in {"prefill", "decode"}: | |
| raise ValueError(f"unknown workload_kind: {self.workload_kind}") | |
| if self.batch_size < 1 or self.suffix_length < 1 or self.prefix_length < 0: | |
| raise ValueError(f"invalid FPM point: {self}") | |
| if self.workload_kind == "decode" and self.suffix_length != 1: | |
| raise ValueError("decode points must execute exactly one new token per request") | |
| if self.workload_kind == "decode" and self.prefix_length < 1: | |
| raise ValueError("decode points require a non-empty prefix") |
🤖 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 `@collector/fpm_forward/types.py` around lines 20 - 26, Update __post_init__ to
reject decode points with prefix_length == 0, while preserving the existing
validation for prefill points and other fields. Ensure the constructor enforces
the runtime requirement that decode points have a positive prefix_length before
sampling can occur.
| choices=["dynamo-j2", "dynamo-python", "llm-d-helm", "llm-d-kustomize", "fpm"], | ||
| default="dynamo-j2", | ||
| help="Deployment target platform. Options: dynamo-j2 (default, Jinja2 templates), " | ||
| help="Deployment target platform. Options: dynamo-j2 (default, typed Dynamo manifests), " | ||
| "dynamo-python (Dynamo Python config modifiers), llm-d-helm (llm-d Helm values), " | ||
| "llm-d-kustomize (llm-d Kustomize overlays).", | ||
| "llm-d-kustomize (llm-d Kustomize overlays), fpm (reusable resource Pod + run.sh).", |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the FPM target reachable from multi-GPU default mode.
build_default_tasks() adds a disaggregated task whenever total_gpus >= 2, so this validator rejects ordinary cli default --backend vllm --deployment-target fpm runs before the supported aggregate task executes. Constrain default task construction to FPM-compatible tasks, or reject default mode explicitly and document an exp-only workflow. Add a multi-GPU cli_main success regression test.
A one-click suggestion is unsafe because filtering versus rejecting changes CLI semantics.
Also applies to: 2307-2323, 2439-2439
🤖 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/cli/main.py` around lines 140 - 144, Update
build_default_tasks and the related deployment-target validation so default mode
with --deployment-target fpm remains reachable when total_gpus >= 2, without
allowing unsupported task combinations. Preserve existing behavior for other
targets, and add a cli_main regression test covering a successful multi-GPU
default invocation with the fpm target.
| def _build_worker(context: dict[str, Any], backend: str, resolved_facts: Any) -> DGDService: | ||
| docs = build_dgd(context, backend, resolved_facts=resolved_facts) | ||
| dgd_docs = [doc for doc in docs if isinstance(doc, DGD)] | ||
| if len(dgd_docs) != 1: | ||
| raise ValueError("FPM V1 requires exactly one DynamoGraphDeployment document") | ||
|
|
||
| workers = [(name, service) for name, service in dgd_docs[0].services.items() if service.component_type == "worker"] | ||
| if len(workers) != 1 or workers[0][0] != "VllmWorker": | ||
| raise ValueError("FPM V1 requires exactly one aggregated VllmWorker") | ||
|
|
||
| worker = workers[0][1] | ||
| if worker.replicas != 1: | ||
| raise ValueError("FPM V1 requires worker replicas=1") | ||
| return worker |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reviewed file and related generator helpers.
git ls-files src/aiconfigurator/generator/builders/fpm_builder.py src/aiconfigurator/generator | sed -n '1,120p'
printf '\n--- outline: fpm_builder.py ---\n'
ast-grep outline src/aiconfigurator/generator/builders/fpm_builder.py --view expanded
printf '\n--- search build_dgd references ---\n'
rg -n "def build_dgd|build_dgd\(" src/aiconfigurator/generator -g '!**/*.pyc'
printf '\n--- locate run.sh template/shebang mentions ---\n'
rg -n "^#!|env bash|/bin/bash" src/aiconfigurator/generator -g '!**/*.pyc'Repository: ai-dynamo/aiconfigurator
Length of output: 10201
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the relevant slices of fpm_builder.py and the build_dgd implementation.
python3 - <<'PY'
from pathlib import Path
paths = [
Path("src/aiconfigurator/generator/builders/fpm_builder.py"),
]
for path in paths:
print(f"\n### {path} ({sum(1 for _ in path.open())} lines)")
with path.open() as f:
for i, line in enumerate(f, 1):
if 1 <= i <= 260:
print(f"{i:4}: {line.rstrip()}")
PYRepository: ai-dynamo/aiconfigurator
Length of output: 12281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect surrounding builder code and the shebang line in the run script template.
python3 - <<'PY'
from pathlib import Path
targets = [
Path("src/aiconfigurator/generator/builders/fpm_builder.py"),
]
for path in targets:
print(f"\n### {path}")
with path.open() as f:
for i, line in enumerate(f, 1):
if 140 <= i <= 430:
print(f"{i:4}: {line.rstrip()}")
PY
printf '\n--- template search for run.sh shebang ---\n'
rg -n "^#!/usr/bin/env bash|^#!/bin/bash|set -e" src/aiconfigurator/generator -g '*run*' -g '*.j2' -g '*.jinja' -g '*.py'Repository: ai-dynamo/aiconfigurator
Length of output: 15506
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the exact reviewed region plus the related helper that produces the DGD docs.
python3 - <<'PY'
from pathlib import Path
for path in [
Path("src/aiconfigurator/generator/builders/fpm_builder.py"),
]:
print(f"\n### {path}")
with path.open() as f:
for i, line in enumerate(f, 1):
if 1 <= i <= 460:
if 150 <= i <= 430:
print(f"{i:4}: {line.rstrip()}")
PY
printf '\n--- build_dgd search ---\n'
rg -n "def build_dgd|build_dgd\(" src/aiconfigurator/generator
printf '\n--- run.sh shebang search ---\n'
rg -n "^#!/usr/bin/env bash|^#!/bin/bash|set -e" src/aiconfigurator/generatorRepository: ai-dynamo/aiconfigurator
Length of output: 15385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the reviewed region plus the related helper that produces the DGD docs.
python3 - <<'PY'
from pathlib import Path
path = Path("src/aiconfigurator/generator/builders/fpm_builder.py")
print(f"### {path}")
with path.open() as f:
for i, line in enumerate(f, 1):
if 150 <= i <= 430:
print(f"{i:4}: {line.rstrip()}")
PY
printf '\n--- build_dgd search ---\n'
rg -n "def build_dgd|build_dgd\(" src/aiconfigurator/generator
printf '\n--- run.sh shebang search ---\n'
rg -n "^#!/usr/bin/env bash|^#!/bin/bash|set -e" src/aiconfigurator/generatorRepository: ai-dynamo/aiconfigurator
Length of output: 15384
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the lowering path around resource conversion and compute-domain claim removal.
python3 - <<'PY'
from pathlib import Path
path = Path("src/aiconfigurator/generator/builders/fpm_builder.py")
with path.open() as f:
for i, line in enumerate(f, 1):
if 618 <= i <= 770 or 770 <= i <= 860:
print(f"{i:4}: {line.rstrip()}")
PYRepository: ai-dynamo/aiconfigurator
Length of output: 10371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the DGD model fields related to claims and compute-domain resources.
python3 - <<'PY'
from pathlib import Path
path = Path("src/aiconfigurator/generator/builders/dgd_model.py")
with path.open() as f:
for i, line in enumerate(f, 1):
if 1 <= i <= 320:
print(f"{i:4}: {line.rstrip()}")
PYRepository: ai-dynamo/aiconfigurator
Length of output: 13070
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("src/aiconfigurator/generator/builders/k8s_builder.py")
with path.open() as f:
for i, line in enumerate(f, 1):
if 1 <= i <= 220:
print(f"{i:4}: {line.rstrip()}")
PYRepository: ai-dynamo/aiconfigurator
Length of output: 10122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ComputeDomain|claim|claims|multinode|extra_pod_spec" src/aiconfigurator/generator/builders/dgd_model.py src/aiconfigurator/generator/builders/k8s_builder.py src/aiconfigurator/generator/builders/fpm_builder.pyRepository: ai-dynamo/aiconfigurator
Length of output: 12334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the lowering path around resource conversion and compute-domain claim removal.
python3 - <<'PY'
from pathlib import Path
path = Path("src/aiconfigurator/generator/builders/fpm_builder.py")
with path.open() as f:
for i, line in enumerate(f, 1):
if 618 <= i <= 860:
print(f"{i:4}: {line.rstrip()}")
PYRepository: ai-dynamo/aiconfigurator
Length of output: 10371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the DGD builder entry point and emitted document types.
python3 - <<'PY'
from pathlib import Path
path = Path("src/aiconfigurator/generator/builders/k8s_builder.py")
with path.open() as f:
for i, line in enumerate(f, 1):
if 1 <= i <= 220:
print(f"{i:4}: {line.rstrip()}")
PYRepository: ai-dynamo/aiconfigurator
Length of output: 10122
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate all compute-domain and claim handling sites.
rg -n "ComputeDomain|claim|claims|multinode|extra_pod_spec" src/aiconfigurator/generator/builders/dgd_model.py src/aiconfigurator/generator/builders/k8s_builder.py src/aiconfigurator/generator/builders/fpm_builder.pyRepository: ai-dynamo/aiconfigurator
Length of output: 12334
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect FPM tests and any explicit expectations around compute-domain claims.
rg -n "fpm_builder|FPM V1|compute-domain-channel|resourceClaims|LeaderWorkerSet|run.sh" tests src/aiconfigurator/generator -g '!**/*.pyc'Repository: ai-dynamo/aiconfigurator
Length of output: 15181
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the FPM unit tests if present.
fd -a "fpm" tests src/aiconfigurator/generator | sed -n '1,120p'Repository: ai-dynamo/aiconfigurator
Length of output: 557
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("tests/unit/generator/test_fpm_artifacts.py")
with path.open() as f:
for i, line in enumerate(f, 1):
if 1180 <= i <= 1225:
print(f"{i:4}: {line.rstrip()}")
PYRepository: ai-dynamo/aiconfigurator
Length of output: 1641
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("src/aiconfigurator/generator/builders/fpm_builder.py")
with path.open() as f:
for i, line in enumerate(f, 1):
if 860 <= i <= 930:
print(f"{i:4}: {line.rstrip()}")
PY
printf '\n--- search for container.resources.claims usage in tests/code ---\n'
rg -n "resources.*claims|\"claims\": \[\{\"name\": \"compute-domain-channel\"\}\]" src tests -g '!**/*.pyc'Repository: ai-dynamo/aiconfigurator
Length of output: 4111
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("tests/unit/generator/test_fpm_artifacts.py")
with path.open() as f:
for i, line in enumerate(f, 1):
if 880 <= i <= 940:
print(f"{i:4}: {line.rstrip()}")
PYRepository: ai-dynamo/aiconfigurator
Length of output: 2721
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("tests/unit/generator/test_fpm_artifacts.py")
with path.open() as f:
for i, line in enumerate(f, 1):
if 900 <= i <= 930:
print(f"{i:4}: {line.rstrip()}")
PYRepository: ai-dynamo/aiconfigurator
Length of output: 1746
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path("tests/unit/generator/test_fpm_artifacts.py")
with path.open() as f:
for i, line in enumerate(f, 1):
if 880 <= i <= 930:
print(f"{i:4}: {line.rstrip()}")
PY
printf '\n--- search for multinode-specific fpm tests ---\n'
rg -n "multinode|LeaderWorkerSet|compute-domain|node_count|node-rank" tests/unit/generator/test_fpm_artifacts.pyRepository: ai-dynamo/aiconfigurator
Length of output: 3529
Keep the multinode compute-domain wiring in the FPM output.
_lower_worker_pod_spec() strips the Pod-level resourceClaims, _lower_resources() drops the worker’s resources.claims, and build_fpm_artifacts() emits only the lowered Pod/LWS. That cuts off the compute-domain manifest chain from build_dgd() for multinode workers. Either carry the ComputeDomain doc through as a second YAML doc or make the alternate reservation path explicit.
Also change the generated shebang in src/aiconfigurator/generator/builders/fpm_builder.py:395 to #!/bin/bash.
🤖 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 170 - 183,
Preserve the multinode compute-domain manifest chain from build_dgd() through
build_fpm_artifacts(), either by emitting the ComputeDomain as a second YAML
document or explicitly implementing the alternate reservation path instead of
relying on stripped resourceClaims in _lower_worker_pod_spec() and
_lower_resources(). Also update the generated script shebang in
build_fpm_artifacts() to use /bin/bash.
Source: Path instructions
| def test_integrated_canary_failure_aborts_remaining_grid(adapter): | ||
| scheduler, _ = adapter | ||
| first = _BenchmarkPoint(point_type="prefill", isl=64) | ||
| second = _BenchmarkPoint(point_type="prefill", isl=128) | ||
| scheduler._bench_skipped_points = [] | ||
| scheduler._bench_grid = deque([second]) | ||
| scheduler._fpm_pending_execution_meta = deque([{"measured": True}]) | ||
| scheduler._fpm_active_execution_meta = {"measured": True} | ||
| scheduler._fpm_canary_completed = False | ||
|
|
||
| scheduler._bench_skip_point(first, "canary_failed") | ||
|
|
||
| assert list(scheduler._bench_grid) == [] | ||
| assert list(scheduler._fpm_pending_execution_meta) == [] | ||
| assert scheduler._bench_skipped_points == [(first, "canary_failed")] |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not silently drain queued points after a canary failure.
Both tests require the remaining grid to disappear while only the canary is recorded. Those queued points neither execute nor raise and become untraceable. Make the runtime surface a classified terminal error instead, then assert that behavior here.
A test-only suggested change is unsafe because the runtime adapter must be corrected first.
As per path instructions, “a queued case may only execute or raise,” and silent case skips must be flagged.
Also applies to: 568-586
🤖 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/collector/test_fpm_runtime_adapter.py` around lines 537 - 551, The
canary-failure path around scheduler._bench_skip_point must not silently drain
queued benchmark points; make the runtime raise a classified terminal error for
remaining queued cases while recording only the failed canary. Update
test_integrated_canary_failure_aborts_remaining_grid and the related test at the
referenced section to assert the error type and preserve the queued cases’
traceability according to the execute-or-raise contract.
Source: Path instructions
Signed-off-by: YZLi <yuanli@nvidia.com>
Signed-off-by: YZLi <yuanli@nvidia.com>
Signed-off-by: YZLi <yuanli@nvidia.com>
Signed-off-by: YZLi <yuanli@nvidia.com>
Signed-off-by: YZLi <yuanli@nvidia.com>
Signed-off-by: YZLi <yuanli@nvidia.com>
Signed-off-by: YZLi <yuanli@nvidia.com>
Signed-off-by: YZLi <yuanli@nvidia.com>
Signed-off-by: YZLi <yuanli@nvidia.com>
Signed-off-by: YZLi <yuanli@nvidia.com>
Signed-off-by: YZLi <yuanli@nvidia.com>
Arsene12358
changed the title
e9a878d to
56f59e6
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 `@README.md`:
- Line 133: Update the README sentence describing custom experiments: replace
“heterogenous” with “heterogeneous” and clarify that users pass the path to
exp.yaml via the --yaml-path option.
🪄 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: 3e953a86-cf1f-4383-abb3-e80cf8d124ea
📒 Files selected for processing (1)
README.md
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Collect snapshot (new)
- GitHub Check: Collect snapshot (old)
- GitHub Check: Build and Test (e2e)
- GitHub Check: Rust/Python engine-step parity
- GitHub Check: Build and Test (unit)
- GitHub Check: Cargo Deny
⚠️ CI failures not shown inline (4)
GitHub Actions: Lint PR / 0_Validate PR title and add label.txt: feature: FPM(ForwardPassMetrics) collector
Conclusion: failure
##[group]Run ytanikin/pr-conventional-commits@b628c5a234cc32513014b7bfdd1e47b532124d98
with:
task_types: ["feat", "fix", "docs", "test", "ci", "refactor", "perf", "chore", "revert", "style", "build"]
add_label: true
***REDACTED***
##[endgroup]
##[error]Invalid or missing task type: 'feature'. Must be one of: feat, fix, docs, test, ci, refactor, perf, chore, revert, style, build
GitHub Actions: Lint PR / Validate PR title and add label: feature: FPM(ForwardPassMetrics) collector
Conclusion: failure
##[group]Run ytanikin/pr-conventional-commits@b628c5a234cc32513014b7bfdd1e47b532124d98
with:
task_types: ["feat", "fix", "docs", "test", "ci", "refactor", "perf", "chore", "revert", "style", "build"]
add_label: true
***REDACTED***
##[endgroup]
##[error]Invalid or missing task type: 'feature'. Must be one of: feat, fix, docs, test, ci, refactor, perf, chore, revert, style, build
GitHub Actions: Copyright Checks / 0_copyright-checks.txt: feature: FPM(ForwardPassMetrics) collector
Conclusion: failure
stems/data/h200_sxm/vllm/0.18.0/context_attention_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.18.0/custom_allreduce_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.18.0/gdn_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.18.0/gemm_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.18.0/generation_attention_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.18.0/moe_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/context_attention_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/custom_allreduce_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/dsa_context_module_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/dsa_generation_module_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/encoder_attention_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/gdn_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/gemm_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/generation_attention_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/mla_context_module_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/mla_generation_module_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/moe_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/da...
GitHub Actions: Copyright Checks / copyright-checks: feature: FPM(ForwardPassMetrics) collector
Conclusion: failure
stems/data/h200_sxm/vllm/0.18.0/context_attention_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.18.0/custom_allreduce_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.18.0/gdn_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.18.0/gemm_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.18.0/generation_attention_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.18.0/moe_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/context_attention_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/custom_allreduce_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/dsa_context_module_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/dsa_generation_module_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/encoder_attention_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/gdn_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/gemm_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/generation_attention_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/mla_context_module_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/mla_generation_module_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/data/h200_sxm/vllm/0.19.0/moe_perf.parquet
[WARN] Unsupported: aic-core/src/aiconfigurator_core/systems/da...
🧰 Additional context used
📓 Path-based instructions (1)
**/*
📄 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:
README.md
⚙️ 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:
README.md
🪛 LanguageTool
README.md
[grammar] ~43-~43: Ensure spelling is correct
Context: ...tor` wheel contains the CLI, generator, webapp, and Spica. It depends on the exact mat...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🔇 Additional comments (1)
README.md (1)
32-37: LGTM!Also applies to: 39-47, 49-56, 58-74, 91-93, 132-132, 134-134
| - Use `--backend` to specify the inference backend: `trtllm` (default), `vllm`, or `sglang`. | ||
| - Use `--deployment-target` to specify the deployment platform: `dynamo-j2` (default, Jinja2 templates), `dynamo-python` (Dynamo Python config modifiers), or `llm-d` (llm-d Helm values). Backends vllm and sglang support llm-d; trtllm is Dynamo-only. | ||
| - Use `--deployment-target` to specify the artifact platform: `dynamo-j2` (default, typed Dynamo manifests), `dynamo-python`, `llm-d-helm`, `llm-d-kustomize`, or `fpm`. FPM V1 supports one aggregated vLLM worker group and emits exactly two artifacts: a reusable keepalive Pod or LeaderWorkerSet, and `run.sh`; see the [Generator overview](docs/generator_overview.md#fpm-v1-target). | ||
| - Use `exp`, pass in exp.yaml by `--yaml-path` to customize your experiments and even a heterogenous one. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the user-facing typo and wording.
Use “heterogeneous” and clarify the --yaml-path example.
As per path instructions, this is a small local fix suitable for an inline suggested change.
Suggested change
-- Use `exp`, pass in exp.yaml by `--yaml-path` to customize your experiments and even a heterogenous one.
+- Use `exp` with `--yaml-path exp.yaml` to customize your experiments, including heterogeneous ones.📝 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.
| - Use `exp`, pass in exp.yaml by `--yaml-path` to customize your experiments and even a heterogenous one. | |
| - Use `exp` with `--yaml-path exp.yaml` to customize your experiments, including heterogeneous ones. |
🤖 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 `@README.md` at line 133, Update the README sentence describing custom
experiments: replace “heterogenous” with “heterogeneous” and clarify that users
pass the path to exp.yaml via the --yaml-path option.
Source: Path instructions
1 participant
Add an integrated FPM collection workflow that derives representative
prefill/decode workloads from AIC model capabilities, executes them through
Dynamo/vLLM, validates runtime coverage, and writes validated results into the
AIC performance database.
The workflow remains part of the existing Collector entry point. It does not
introduce a separate user-facing FPM CLI.
Details:
yet registered in AIC.
backend matrices subject to model and runtime compatibility constraints.
block-aligned past-KV workloads.
execution.
Graph capture configuration for each collection cell.
for Kubernetes rendering, process launch, timeout handling, artifact
collection, and teardown.
formal collection.
artifacts introduced by Dynamo PR 11509.
ordering, measurement count, and artifact consistency before database
publication.
time, and end-to-end cell duration.
runtime artifact validation, Generator rendering, warmup semantics, and
database aggregation.
Validation:
uv run --frozen pytest tests/unit/collector -q563 passed, 5 skippedruff format --checkandruff checkpassed for all changed Python files.Where should the reviewer start?
collector/fpm_forward/entry.pyandcollector/fpm_forward/planner.pyfor the end-to-end workflow and planconstruction.
collector/fpm_forward/population.py,collector/fpm_forward/sampling.py, andcollector/fpm_forward/capacity.pyfor workload generation, selection, andmemory admission.
collector/fpm_forward/runner.pyfor the Generator boundary, Kubernetesexecution, artifact validation, checkpointing, timing, and cleanup.
collector/fpm_forward/runtime/vllm_scheduler.pyfor the integrated Dynamocapacity gate, global warmup, past-KV handling, and per-rank result format.
src/aiconfigurator/generator/builders/fpm_builder.pyfor the Generator FPMdeployment target.
tests/unit/collector/test_fpm_forward.py,tests/unit/collector/test_fpm_runner.py, andtests/unit/collector/test_fpm_runtime_adapter.pyfor the behavioralcontracts.
Summary by CodeRabbit
New Features
run.sh.Documentation
Validation