fix(collector): remove exception-smuggling skip guards; document permanent kernel limits#1359
fix(collector): remove exception-smuggling skip guards; document permanent kernel limits#1359kaim-eng wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (7)
WalkthroughTRT-LLM attention execution now includes FIXME comments documenting unsupported ChangesTRT-LLM attention documentation
Estimated code review effort: 1 (Trivial) | ~2 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.
🧹 Nitpick comments (2)
collector/trtllm/collect_mla.py (1)
47-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
get_sm_version()for consistency with the rest of the codebase.Every other skip predicate added in this PR compares
get_sm_version() == 90. Here,torch.cuda.get_device_capability() == (9, 0)is used directly, and the guard falls back to "no skip" iftorch.cuda.is_available()isFalse— a caseget_sm_version()'s cuda-python fallback would otherwise still resolve.♻️ Suggested fix
def _skip_sm90_mla_generation() -> bool: - if not torch.cuda.is_available(): - return False # TRT-LLM 1.3.0rc15 aborts in AttentionOp::initialize() for standalone # DeepSeek MLA generation on Hopper with: # "Deepseek should be supported by fmha in generation part." # This is a C++ SIGABRT, so Python cannot mark it expected after dispatch. # Pinned to rc15 only (verified on H200); re-evaluate on rc17+ before extending. - return tensorrt_llm.__version__.startswith("1.3.0rc15") and torch.cuda.get_device_capability() == (9, 0) + return tensorrt_llm.__version__.startswith("1.3.0rc15") and get_sm_version() == 90🤖 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/trtllm/collect_mla.py` around lines 47 - 56, Update _skip_sm90_mla_generation to use the codebase’s get_sm_version() helper and compare its result with 90, removing the direct torch.cuda availability and device-capability checks while preserving the existing TRT-LLM version condition.collector/trtllm/collect_attn.py (1)
103-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate crash-guard logic vs.
_skip_trtllm_sm89_rc15_long_context_gqa(lines 70-100).The SM90 helper repeats the SM89 body verbatim except for the SM check. Extract a shared implementation parameterized by
smto avoid the two copies silently drifting apart as thresholds get refined.♻️ Suggested consolidation
-def _skip_trtllm_sm89_rc15_long_context_gqa(batch_size, input_len, num_heads, num_key_value_heads, head_dim) -> bool: - if not (tensorrt_llm.__version__.startswith("1.3.0rc15") and get_sm_version() == 89): - return False - ... -def _skip_trtllm_sm90_rc15_long_context_gqa(batch_size, input_len, num_heads, num_key_value_heads, head_dim) -> bool: - if not (tensorrt_llm.__version__.startswith("1.3.0rc15") and get_sm_version() == 90): - return False - ... +def _skip_trtllm_rc15_long_context_gqa(sm: int, batch_size, input_len, num_heads, num_key_value_heads, head_dim) -> bool: + if not (tensorrt_llm.__version__.startswith("1.3.0rc15") and get_sm_version() == sm): + return False + # shared threshold logic🤖 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/trtllm/collect_attn.py` around lines 103 - 137, Extract the shared long-context GQA threshold logic from `_skip_trtllm_sm89_rc15_long_context_gqa` and `_skip_trtllm_sm90_rc15_long_context_gqa` into one helper parameterized by the target SM version. Keep each wrapper responsible only for its TRT-LLM version and SM check, and preserve the existing head-dimension, token-count, and supported-head behavior.
🤖 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 `@collector/trtllm/collect_attn.py`:
- Around line 103-137: Extract the shared long-context GQA threshold logic from
`_skip_trtllm_sm89_rc15_long_context_gqa` and
`_skip_trtllm_sm90_rc15_long_context_gqa` into one helper parameterized by the
target SM version. Keep each wrapper responsible only for its TRT-LLM version
and SM check, and preserve the existing head-dimension, token-count, and
supported-head behavior.
In `@collector/trtllm/collect_mla.py`:
- Around line 47-56: Update _skip_sm90_mla_generation to use the codebase’s
get_sm_version() helper and compare its result with 90, removing the direct
torch.cuda availability and device-capability checks while preserving the
existing TRT-LLM version condition.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 385c005b-0cd3-4e12-88b1-d90c1ef239b8
📒 Files selected for processing (7)
collector/cases/sm_exceptions/sm100_exceptions.yamlcollector/trtllm/collect_attn.pycollector/trtllm/collect_mamba2.pycollector/trtllm/collect_mla.pycollector/trtllm/collect_mla_module.pycollector/vllm/collect_gemm.pycollector/vllm/collect_mla_module.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (7)
collector/cases/**/*.yaml
📄 CodeRabbit inference engine (.claude/rules/collector/case_authoring.md)
Do not add YAML keys that condition on batch, sequence, token, or feature values.
Do not extend collector YAML schemas to express shape conditions or framework-version predicates; their deliberate inexpressibility is a guardrail.
Files:
collector/cases/sm_exceptions/sm100_exceptions.yaml
{collector/**,tests/unit/collector/**}
📄 CodeRabbit inference engine (.claude/rules/collector/failure_handling.md)
{collector/**,tests/unit/collector/**}: In collector failure handling, observe failures rather than predict them: do not use a declarative expected-failure layer or automatic skips; failing groups must be fixed.
Record every worker failure automatically in the relevant error and collection-summary JSON files with classification, case parameters, exception details, and model/dtype grouping.
Reset a worker after CUDA-fatal errors only after recording the failed task.
Treat missing data points as tolerable downstream because the SDK can interpolate, extrapolate, reuse sibling-version rows, or use HYBRID empirical estimates.
For a hanging case or one that kills the node, add a dated denylist.yaml entry with a reason.
For an entirely unvalidated operation/backend pair, mark the registry OpEntry as unverified=true.
For cases not validated on a specific SM, mark the registry OpEntry with unverified_sms=(sm,).
Represent physically impossible hardware capabilities with a positive floor in capabilities.yaml; do not use it for framework-version kernel gaps.
Treat out-of-memory failures as unclassified until reproduced on a clean GPU; only a genuine OOM may be excluded by the sanctioned generation-time memory filter.
Fix proven collector-code bugs in code rather than using skips, and re-check dispatch and skip rules.
Do not encode framework-version gaps in YAML; allow them to fail and be re-tested after version changes.
Treat isolated, explained, unclustered failures as acceptable; investigate around 10% unexpected failures or sooner when failures cluster.
Treat roughly one-third failing cases or an entire failing family as a systemic collector problem; stop collection and fix the collector.
Never hide failures with broad skips, retries, generic OOM labels, reduced coverage, synthetic rows, or weakened benchmarks.
Compare failure records with the previous run for the same backend and version; prioritize only new failure groups.
Do not mechanically translate failure records in...
Files:
collector/cases/sm_exceptions/sm100_exceptions.yamlcollector/trtllm/collect_mla.pycollector/vllm/collect_mla_module.pycollector/trtllm/collect_mamba2.pycollector/vllm/collect_gemm.pycollector/trtllm/collect_attn.pycollector/trtllm/collect_mla_module.py
collector/**
📄 CodeRabbit inference engine (.claude/rules/collector/layer_permissions.md)
collector/**: Keep collector-layer rules in their designated locations: base-op YAML for sweeps and axis-levelmin_sm; model case YAML for structural shapes and activation; capabilities YAML for positive hardware floors; registries for version routing and maturity markers; collectors for dispatch, classified errors, and memory-feasibility filtering; and the denylist only for hangs or node-killing cases.
Collector tasks may modify onlycollector/andtests/unit/collector/; they must not modify SDK, Rust, tools, systems data, or generator code.
Changing the collector data contract requires explicit human approval; do not independently add or rename columns, perf files, or SDK-parsed key dimensions.
Do not edit human-owned collector rule files under.claude/rules/collector/as a side effect of a fix task; propose policy changes instead.
For a failing case, default to no code change: verify that it is recorded and classified in the failure log, then stop unless the failure-handling decision tree requires escalation.
Mechanism changes to the executor, case generator, failure classification, or case-ID format require explicit human approval and must be proposed rather than included in a case fix.Review the collector when generated configuration formats change or when generator parameter names used in benchmark configurations change.
Files:
collector/cases/sm_exceptions/sm100_exceptions.yamlcollector/trtllm/collect_mla.pycollector/vllm/collect_mla_module.pycollector/trtllm/collect_mamba2.pycollector/vllm/collect_gemm.pycollector/trtllm/collect_attn.pycollector/trtllm/collect_mla_module.py
⚙️ CodeRabbit configuration file
collector/**: - Check collector changes for backend/runtime version accuracy, GPU resource assumptions, reproducible command construction, and clear failure evidence.
- Flag changes that make support-matrix or perf-data results harder to trace back to the command, model, system, quantization, or runtime version that produced them.
Files:
collector/cases/sm_exceptions/sm100_exceptions.yamlcollector/trtllm/collect_mla.pycollector/vllm/collect_mla_module.pycollector/trtllm/collect_mamba2.pycollector/vllm/collect_gemm.pycollector/trtllm/collect_attn.pycollector/trtllm/collect_mla_module.py
collector/**/*
📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)
collector/**/*: When editing or reviewingcollector/**, load and applylayer_permissions.mdandfailure_handling.md; also loadcase_authoring.mdfor case YAML work.
Do not apply generator-module rules tocollector/case_generator.py; it expands collection test cases and is unrelated to deployment-config generation.
Collector work is standalone GPU performance data collection and is not part of the wheel runtime.
Files:
collector/cases/sm_exceptions/sm100_exceptions.yamlcollector/trtllm/collect_mla.pycollector/vllm/collect_mla_module.pycollector/trtllm/collect_mamba2.pycollector/vllm/collect_gemm.pycollector/trtllm/collect_attn.pycollector/trtllm/collect_mla_module.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:
collector/cases/sm_exceptions/sm100_exceptions.yamlcollector/trtllm/collect_mla.pycollector/vllm/collect_mla_module.pycollector/trtllm/collect_mamba2.pycollector/vllm/collect_gemm.pycollector/trtllm/collect_attn.pycollector/trtllm/collect_mla_module.py
collector/**/collect_*.py
📄 CodeRabbit inference engine (.claude/rules/collector/layer_permissions.md)
collector/**/collect_*.py: Collector code may dispatch between kernel paths by SM or version, record the actualkernel_source, raise classified exceptions for runtime probes, and perform memory-feasibility filtering; it must not silently skip queued cases, perform other case filtering, or patch code for one shape.
A queued case must be executed or raise a classified error; branches may change how a case runs, but must not change whether it runs.
Use the framework's own dispatch path to select kernels. Manual backend pinning requires a source citation, andkernel_sourcemust record the actually invoked kernel.
Do not invent backend fallbacks or swap backends in exception handlers. Raise a classified error if the framework-selected path cannot run; only replicate fallback behavior performed by the framework itself.
Measurement-method degradation, such as CUDA-graph capture falling back to eager execution, is allowed only when recorded in the output row; API-compatibility shims may alter construction but never kernel selection.
Memory-feasibility filtering is permitted only during generation insideget_*_test_cases(), using footprint-versus-capacity arithmetic and live device memory where possible; drops must be counted and logged, never silently skipped.
Framework kernel limits must remain asFIXME(kernel-limit)comments at the owning collector invocation site, stating the limit, origin, and unverified status; do not encode them in YAML or implement guards from unverified claims.
On framework version bumps, re-verify every pinned backend and everyFIXME(kernel-limit)against framework source before trusting collected data.
Files:
collector/trtllm/collect_mla.pycollector/vllm/collect_mla_module.pycollector/trtllm/collect_mamba2.pycollector/vllm/collect_gemm.pycollector/trtllm/collect_attn.pycollector/trtllm/collect_mla_module.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the project’s
uv-managed environment and run linting withruff check .andruff format --check ..
Files:
collector/trtllm/collect_mla.pycollector/vllm/collect_mla_module.pycollector/trtllm/collect_mamba2.pycollector/vllm/collect_gemm.pycollector/trtllm/collect_attn.pycollector/trtllm/collect_mla_module.py
🧠 Learnings (5)
📚 Learning: 2026-06-05T09:02:37.734Z
Learnt from: YijiaZhao
Repo: ai-dynamo/aiconfigurator PR: 1204
File: collector/cases/models/DeepseekV4ForCausalLM_cases.yaml:209-227
Timestamp: 2026-06-05T09:02:37.734Z
Learning: In ai-dynamo/aiconfigurator case YAMLs, op execution order is determined by the registry insertion order as iterated by `build_collections()` (i.e., earlier registered ops run before later ones). `CollectionCasePlan.ops` may be a sorted list but it is used only to filter which ops run, not to control execution order—so do not assume lexicographic op-name order affects execution. Therefore, YAML comments asserting ordering guarantees (e.g., “must run AFTER …”) should be evaluated against registry insertion order, not name sorting. Also, sparse kernel workers should no-op gracefully when their source CSV inputs are absent; in sparse-only runs, missing CSVs should be treated as expected behavior rather than a data-corruption/wrong-write issue.
Applied to files:
collector/cases/sm_exceptions/sm100_exceptions.yaml
📚 Learning: 2026-06-29T16:39:41.097Z
Learnt from: tianhaox
Repo: ai-dynamo/aiconfigurator PR: 1256
File: collector/cases/models/Gemma4ForConditionalGeneration_cases.yaml:10-13
Timestamp: 2026-06-29T16:39:41.097Z
Learning: In `ai-dynamo/aiconfigurator` Collector planner case YAMLs, treat `base_ops` as an allowlist only for base-op files that are actually exposed/collectable via the corresponding base documents’ `model_ops` (i.e., there must be an explicit exposure path from a base document to that op). If an op is not exposed through `model_ops` in the base-file `collector/cases/base_ops/*.yaml`, do not add it to `base_ops`—instead, it should be selected through model-local sections such as `all_frameworks_op_cases` (or the equivalent model-specific framework case selectors). When reviewing, verify the exposure path for the op (e.g., `moe` should be included via `all_frameworks_op_cases.moe` for Gemma cases if—and only if—the relevant base-file `model_ops` exposure does not already make it collectable through `base_ops`).
Applied to files:
collector/cases/sm_exceptions/sm100_exceptions.yaml
📚 Learning: 2026-02-28T11:44:28.109Z
Learnt from: tianhaox
Repo: ai-dynamo/aiconfigurator PR: 466
File: collector/trtllm/collect_moe_v3.py:4-4
Timestamp: 2026-02-28T11:44:28.109Z
Learning: In collector-related Python files (e.g., collector/trtllm/collect_moe_v3.py), document and enforce that version incompatibilities are surfaced as non-fatal runtime errors during collection. Do not add aggressive preventive version-gating; instead, allow generating test cases that may not be supported across all versions within the __compat__ range and rely on runtime error handling to skip or flag unsupported cases. This should be verifiable by ensuring collection proceeds, errors are reported, and unsupported cases do not halt the overall process.
Applied to files:
collector/trtllm/collect_mla.pycollector/vllm/collect_mla_module.pycollector/trtllm/collect_mamba2.pycollector/vllm/collect_gemm.pycollector/trtllm/collect_attn.pycollector/trtllm/collect_mla_module.py
📚 Learning: 2026-07-05T07:16:31.423Z
Learnt from: tianhaox
Repo: ai-dynamo/aiconfigurator PR: 1264
File: collector/vllm/utils.py:29-34
Timestamp: 2026-07-05T07:16:31.423Z
Learning: In ai-dynamo/aiconfigurator’s vLLM collector modules, keep vLLM imports at top level (not lazily deferred) so a mismatched vLLM version fails during the per-op import and is recorded/classified as a per-op failure (via the existing per-op try/except and __compat gating), rather than altering failure order/granularity. Do not rely on moving vLLM imports into shared helpers like collector/vllm/utils.py to change this behavior. Exception: collector/vllm/collect_mhc_module.py is intentionally “import-vllm-free” (vLLM dependencies were localized) and should remain so to preserve the intended behavior there.
Applied to files:
collector/vllm/collect_mla_module.pycollector/vllm/collect_gemm.py
📚 Learning: 2026-07-11T03:07:50.673Z
Learnt from: tianhaox
Repo: ai-dynamo/aiconfigurator PR: 1344
File: tests/unit/collector/test_framework_manifest.py:42-49
Timestamp: 2026-07-11T03:07:50.673Z
Learning: For the vLLM “collector v2” registries (e.g., collector/vllm/registry.py and any other vLLM collector v2 registry modules), treat the registry entry `versions` field as a retired per-entry versioning mechanism. Code and tests should expect it to be empty/falsy for every registry entry (i.e., review changes to ensure `entry.versions` is not populated). Version pinning for the vLLM backend must be centralized in `collector/framework_manifest.yaml` via `get_collector_runtime("vllm")`, and each vLLM collector module must declare exactly one `__compat__ = "vllm==<manifest_version>"` line matching the pinned manifest version. If relevant, do not misread the framework-manifest test: `test_active_cuda_vllm_collectors_are_exactly_pinned_to_manifest_version` asserts `all(not entry.versions for entry in VLLM_REGISTRY)` and that `__compat__` matches the manifest version (it is not an inverted assertion).
Applied to files:
collector/vllm/collect_mla_module.pycollector/vllm/collect_gemm.py
🔇 Additional comments (11)
collector/trtllm/collect_attn.py (3)
171-180: 🎯 Functional CorrectnessSkip is broader than the documented verified scope.
The docstring says the crash was verified "across h=64 and h=128 MHA/GQA shapes," but
_skip_trtllm_sm90_rc15_fp8_generationtakes onlyuse_fp8_kv_cacheand blocks every head config on SM90/rc15 FP8-KV generation, including untested shapes (h=96/48/40). Per collector guidelines, a queued case must run or raise a classified error — filtering beyond the verified crash region risks silently dropping cases that would actually succeed.Please confirm whether all head counts were actually observed to crash, or narrow the guard to the verified
n/num_key_value_headscombinations.
183-198: 🎯 Functional CorrectnessHead-dim guard is not version-gated, unlike every other predicate in this file.
_skip_trtllm_unsupported_head_dimskipshead_dim > 256andhead_dim == 192on SM≥90 regardless oftensorrt_llm.__version__, whereas all other helpers here are pinned to specific rc versions. If this is meant as a permanent architectural kernel limit rather than a version bug, per coding guidelines it should be recorded as aFIXME(kernel-limit)comment at the invocation site (stating limit/origin/unverified status) rather than a hard guard with no way to "re-verify... on framework version bumps."Based on coding guidelines: "Framework kernel limits must remain as FIXME(kernel-limit) comments at the owning collector invocation site, stating the limit, origin, and unverified status; do not encode them in YAML or implement guards from unverified claims."
Source: Coding guidelines
457-543: LGTM!Also applies to: 568-613
collector/trtllm/collect_mamba2.py (1)
76-121: LGTM!collector/trtllm/collect_mla.py (1)
58-67: LGTM!collector/trtllm/collect_mla_module.py (1)
137-146: LGTM!Also applies to: 213-240
collector/vllm/collect_mla_module.py (1)
145-153: LGTM!Also applies to: 196-200
collector/cases/sm_exceptions/sm100_exceptions.yaml (1)
123-137: LGTM!collector/vllm/collect_gemm.py (3)
57-63: 🎯 Functional Correctness | ⚡ Quick winString-comparison fallback can misorder versions.
vllm_version >= targetlexicographically compares strings, so e.g."0.9.0" >= "0.22.0"evaluatesTrueeven though 0.9.0 predates 0.22.0. This fallback only fires ifpackaging.version.Versionparsing fails, but when it does, it silently produces wrong routing between the new/old FP8 weight-prep paths.🛠️ Proposed fix: use tuple-based numeric fallback
def _vllm_version_at_least(target: str) -> bool: try: from packaging.version import Version return Version(vllm_version) >= Version(target) except Exception: - return vllm_version >= target + def _parts(v: str) -> tuple[int, ...]: + return tuple(int(p) for p in v.split(".")[:3] if p.isdigit()) + return _parts(vllm_version) >= _parts(target)
179-203: 🎯 Functional Correctness | ⚡ Quick winVerify per-tensor FP8 path also needs an explicit
input_scale.The
fp8_blockbranch a few lines below (Line 225-226 in this file) explicitly setsgemm.input_scale = Nonebecause the dynamic activation scheme doesn't create it but the forward path reads it. This new per-tensor branch now also routes throughquant_method.process_weights_after_loading(gemm)for the first time (previously it only transposed the weight), yet does not setinput_scale. If the per-tensor cutlass kernel'sapply()readsinput_scaleunder dynamic activation, the dry-run forward at Line 244 could fail with anAttributeError.Please confirm whether vLLM's per-tensor FP8 dynamic path (e.g.
CutlassFP8ScaledMMLinearKernel.apply) requiresinput_scaleto be set, and if so, add the same guard as thefp8_blockbranch.
185-200: LGTM!
f5c8cb0 to
8f56d59
Compare
kaim-eng
changed the title
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/trtllm/collect_attn.py`:
- Around line 4-10: Move the canonical FIXME(kernel-limit) comment to the
runtime invocation beside run_attention_torch, preserving the pinned TRT-LLM
version/source and GQA-limit details. Remove the module-scope and
generation-helper copies, and do not add the note back to YAML.
- Around line 4-10: Update the kernel-limit FIXME comment in collect_attn.py to
remove SM120 from the affected SM list, limiting the claim to the SMs covered by
this change; alternatively, explicitly mark SM120 as unverified while preserving
the existing verification and probe-or-delete guidance.
🪄 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: 43dfe789-6a03-46bc-a1ec-3c131daf0b97
📒 Files selected for processing (1)
collector/trtllm/collect_attn.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
- GitHub Check: Collect snapshot (old)
- GitHub Check: Collect snapshot (new)
- GitHub Check: Build and Test (e2e)
- GitHub Check: Build and Test (unit)
- GitHub Check: Rust/Python engine-step parity
- GitHub Check: Cargo Deny
🧰 Additional context used
📓 Path-based instructions (6)
{collector/**,tests/unit/collector/**}
📄 CodeRabbit inference engine (.claude/rules/collector/failure_handling.md)
{collector/**,tests/unit/collector/**}: In collector failure handling, observe failures rather than predict them: do not use a declarative expected-failure layer or automatic skips; failing groups must be fixed.
Record every worker failure automatically in the relevant error and collection-summary JSON files with classification, case parameters, exception details, and model/dtype grouping.
Reset a worker after CUDA-fatal errors only after recording the failed task.
Treat missing data points as tolerable downstream because the SDK can interpolate, extrapolate, reuse sibling-version rows, or use HYBRID empirical estimates.
For a hanging case or one that kills the node, add a dated denylist.yaml entry with a reason.
For an entirely unvalidated operation/backend pair, mark the registry OpEntry as unverified=true.
For cases not validated on a specific SM, mark the registry OpEntry with unverified_sms=(sm,).
Represent physically impossible hardware capabilities with a positive floor in capabilities.yaml; do not use it for framework-version kernel gaps.
Treat out-of-memory failures as unclassified until reproduced on a clean GPU; only a genuine OOM may be excluded by the sanctioned generation-time memory filter.
Fix proven collector-code bugs in code rather than using skips, and re-check dispatch and skip rules.
Do not encode framework-version gaps in YAML; allow them to fail and be re-tested after version changes.
Treat isolated, explained, unclustered failures as acceptable; investigate around 10% unexpected failures or sooner when failures cluster.
Treat roughly one-third failing cases or an entire failing family as a systemic collector problem; stop collection and fix the collector.
Never hide failures with broad skips, retries, generic OOM labels, reduced coverage, synthetic rows, or weakened benchmarks.
Compare failure records with the previous run for the same backend and version; prioritize only new failure groups.
Do not mechanically translate failure records in...
Files:
collector/trtllm/collect_attn.py
collector/**
📄 CodeRabbit inference engine (.claude/rules/collector/layer_permissions.md)
collector/**: Keep collector-layer rules in their designated locations: base-op YAML for sweeps and axis-levelmin_sm; model case YAML for structural shapes and activation; capabilities YAML for positive hardware floors; registries for version routing and maturity markers; collectors for dispatch, classified errors, and memory-feasibility filtering; and the denylist only for hangs or node-killing cases.
Collector tasks may modify onlycollector/andtests/unit/collector/; they must not modify SDK, Rust, tools, systems data, or generator code.
Changing the collector data contract requires explicit human approval; do not independently add or rename columns, perf files, or SDK-parsed key dimensions.
Do not edit human-owned collector rule files under.claude/rules/collector/as a side effect of a fix task; propose policy changes instead.
For a failing case, default to no code change: verify that it is recorded and classified in the failure log, then stop unless the failure-handling decision tree requires escalation.
Mechanism changes to the executor, case generator, failure classification, or case-ID format require explicit human approval and must be proposed rather than included in a case fix.Review the collector when generated configuration formats change or when generator parameter names used in benchmark configurations change.
Files:
collector/trtllm/collect_attn.py
⚙️ CodeRabbit configuration file
collector/**: - Enforce the collector rules from.claude/rules/collector/layer_permissions.md,failure_handling.md, andcase_authoring.md.
- Flag any silent case skip in collector code (a queued case may only execute or raise); the sole sanctioned filter is generation-time memory feasibility with counted drops.
- Flag invented fallbacks on both ends: generation must raise on unresolvable declarations (never substitute defaults or another model's geometry); collectors must never swap in a different backend/kernel than the framework's own dispatch selects — manual pins require framework source citations.
- Flag any reintroduction of selector/exception machinery (case_ids/contains/indices/ranges/limit/rules, sm_exceptions-style shape or version predicates) in YAML or code.
- Capability floors (
cases/capabilities.yaml) may hold hardware facts only: no shapes, no framework versions, no per-backend nesting.cases/denylist.yamlis for hang/node-killers only, dated.- Collector changes must stay within
collector/andtests/unit/collector/; flag producer+consumer contract changes (perf row schema, PerfFile names) unless the PR explicitly declares them.- Check collector changes for backend/runtime version accuracy, GPU resource assumptions, reproducible command construction, and clear failure evidence.
- Flag changes that make support-matrix or perf-data results harder to trace back to the command, model, system, quantization, or runtime version that produced them.
Files:
collector/trtllm/collect_attn.py
collector/**/collect_*.py
📄 CodeRabbit inference engine (.claude/rules/collector/layer_permissions.md)
collector/**/collect_*.py: Collector code may dispatch between kernel paths by SM or version, record the actualkernel_source, raise classified exceptions for runtime probes, and perform memory-feasibility filtering; it must not silently skip queued cases, perform other case filtering, or patch code for one shape.
A queued case must be executed or raise a classified error; branches may change how a case runs, but must not change whether it runs.
Use the framework's own dispatch path to select kernels. Manual backend pinning requires a source citation, andkernel_sourcemust record the actually invoked kernel.
Do not invent backend fallbacks or swap backends in exception handlers. Raise a classified error if the framework-selected path cannot run; only replicate fallback behavior performed by the framework itself.
Measurement-method degradation, such as CUDA-graph capture falling back to eager execution, is allowed only when recorded in the output row; API-compatibility shims may alter construction but never kernel selection.
Memory-feasibility filtering is permitted only during generation insideget_*_test_cases(), using footprint-versus-capacity arithmetic and live device memory where possible; drops must be counted and logged, never silently skipped.
Framework kernel limits must remain asFIXME(kernel-limit)comments at the owning collector invocation site, stating the limit, origin, and unverified status; do not encode them in YAML or implement guards from unverified claims.
On framework version bumps, re-verify every pinned backend and everyFIXME(kernel-limit)against framework source before trusting collected data.
Files:
collector/trtllm/collect_attn.py
collector/**/*
📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)
collector/**/*: When editing or reviewingcollector/**, load and applylayer_permissions.mdandfailure_handling.md; also loadcase_authoring.mdfor case YAML work.
Do not apply generator-module rules tocollector/case_generator.py; it expands collection test cases and is unrelated to deployment-config generation.
Collector work is standalone GPU performance data collection and is not part of the wheel runtime.
Files:
collector/trtllm/collect_attn.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Use the project’s
uv-managed environment and run linting withruff check .andruff format --check ..
Files:
collector/trtllm/collect_attn.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:
collector/trtllm/collect_attn.py
🧠 Learnings (1)
📚 Learning: 2026-02-28T11:44:28.109Z
Learnt from: tianhaox
Repo: ai-dynamo/aiconfigurator PR: 466
File: collector/trtllm/collect_moe_v3.py:4-4
Timestamp: 2026-02-28T11:44:28.109Z
Learning: In collector-related Python files (e.g., collector/trtllm/collect_moe_v3.py), document and enforce that version incompatibilities are surfaced as non-fatal runtime errors during collection. Do not add aggressive preventive version-gating; instead, allow generating test cases that may not be supported across all versions within the __compat__ range and rely on runtime error handling to skip or flag unsupported cases. This should be verifiable by ensuring collection proceeds, errors are reported, and unsupported cases do not halt the overall process.
Applied to files:
collector/trtllm/collect_attn.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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/trtllm/collect_attn.py`:
- Around line 4-10: Move the canonical FIXME(kernel-limit) comment to the
runtime invocation beside run_attention_torch, preserving the pinned TRT-LLM
version/source and GQA-limit details. Remove the module-scope and
generation-helper copies, and do not add the note back to YAML.
- Around line 4-10: Update the kernel-limit FIXME comment in collect_attn.py to
remove SM120 from the affected SM list, limiting the claim to the SMs covered by
this change; alternatively, explicitly mark SM120 as unverified while preserving
the existing verification and probe-or-delete guidance.
🪄 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: 43dfe789-6a03-46bc-a1ec-3c131daf0b97
📒 Files selected for processing (1)
collector/trtllm/collect_attn.py
🛑 Comments failed to post (1)
collector/trtllm/collect_attn.py (1)
4-10: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the kernel-limit FIXME to the runtime invocation site. Keep one canonical
FIXME(kernel-limit)besiderun_attention_torch, include the pinned TRT-LLM version/source for the GQA limit, and remove the duplicate module-scope/generation-helper notes.🤖 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/trtllm/collect_attn.py` around lines 4 - 10, Move the canonical FIXME(kernel-limit) comment to the runtime invocation beside run_attention_torch, preserving the pinned TRT-LLM version/source and GQA-limit details. Remove the module-scope and generation-helper copies, and do not add the note back to YAML.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail git ls-files collector/trtllm/collect_attn.py .claude/rules/collector/layer_permissions.md .claude/rules/collector/failure_handling.md .claude/rules/collector/case_authoring.md collector/cases/capabilities.yaml collector/cases/denylist.yaml printf '\n--- collect_attn.py (top + relevant ranges) ---\n' sed -n '1,40p' collector/trtllm/collect_attn.py printf '\n--- collect_attn.py (around reported later lines) ---\n' sed -n '400,540p' collector/trtllm/collect_attn.py printf '\n--- collector layer rules (short excerpts) ---\n' sed -n '1,220p' .claude/rules/collector/layer_permissions.md printf '\n--- failure handling rules (short excerpts) ---\n' sed -n '1,220p' .claude/rules/collector/failure_handling.md printf '\n--- case authoring rules (short excerpts) ---\n' sed -n '1,220p' .claude/rules/collector/case_authoring.mdRepository: ai-dynamo/aiconfigurator
Length of output: 28611
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n--- exact ranges around later FIXME sites ---\n' nl -ba collector/trtllm/collect_attn.py | sed -n '410,430p' printf '\n--- generation section around line 523 ---\n' nl -ba collector/trtllm/collect_attn.py | sed -n '515,535p' printf '\n--- all SM120 references in file ---\n' rg -n "SM120|sm120" collector/trtllm/collect_attn.pyRepository: ai-dynamo/aiconfigurator
Length of output: 249
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n--- around lines 410-430 ---\n' sed -n '410,430p' collector/trtllm/collect_attn.py | cat -n printf '\n--- around lines 515-535 ---\n' sed -n '515,535p' collector/trtllm/collect_attn.py | cat -n printf '\n--- all SM120 references in file ---\n' rg -n "SM120|sm120" collector/trtllm/collect_attn.pyRepository: ai-dynamo/aiconfigurator
Length of output: 3318
Narrow the kernel-limit note to the covered SMs
Don’t include SM120 in this FIXME yet; keep the claim limited to the SMs this change covers, or add an explicit “SM120 unverified” caveat.Suggested change
-# against TRT-LLM kernel-selection source. On SM100/103/120, TRT-LLM attention +# against TRT-LLM kernel-selection source. On SM100/103, TRT-LLM attention +# SM120 remains unverified and out of scope for this note.📝 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.# FIXME(kernel-limit): retired sm_exceptions rule (PR `#1302`), not verified # against TRT-LLM kernel-selection source. On SM100/103, TRT-LLM attention # kernels reportedly reject GQA ratios (num_heads/num_kv_heads) >= 32 unless # the ratio is a multiple of 32. SM120 remains unverified and out of scope # for this note; affected cases currently fail at runtime. # On the next version bump: verify against the framework source, then either # probe-and-raise before invocation or delete this note. Never move this back # into YAML (see .claude/rules/collector/layer_permissions.md).🤖 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/trtllm/collect_attn.py` around lines 4 - 10, Update the kernel-limit FIXME comment in collect_attn.py to remove SM120 from the affected SM list, limiting the claim to the SMs covered by this change; alternatively, explicitly mark SM120 as unverified while preserving the existing verification and probe-or-delete guidance.Source: Path instructions
…ring power collection Each guard is pinned to the exact (SM version, framework version) combination verified during B200/H200 power data collection. Guards named sm100 use get_sm_version() == 100; SM120 coverage requires a separate predicate if confirmed on that hardware. TRT-LLM guards (rc15/rc17 on SM90/SM100): - collect_attn.py: skip SM90 long-context GQA (cudaErrorIllegalAddress), SM90 FP8-KV generation (SIGABRT), and head_dim>256 / head_dim==192+SM>=90 (MMHA std::terminate) — C++ abort paths Python cannot catch - collect_mamba2.py: skip Nemotron Ultra 550B context on SM90+rc15 (OOM/NoneType crash-loop) - collect_mla.py: skip standalone DeepSeek MLA generation on SM90+rc15 (AttentionOp::initialize SIGABRT); pinned to rc15 only - collect_mla_module.py: skip DSA context prefix on SM100+rc15/rc17 (CUDA context poison); skip GLM-5 DSA module on SM100+rc15/rc17 (DeepGEMM row-alignment assert) vLLM guards (0.22.0 on SM100): - collect_gemm.py: route per-tensor FP8 through process_weights_after_loading (required by CutlassFP8ScaledMMLinearKernel in 0.22.0+); weight_scale attribute confirmed on B200/SM100 via smoke test - collect_vllm/collect_mla_module.py: skip SM100 DSA context fp8_block (missing TRTLLM-GEN kernels / FlashInfer CUDA invalid argument) Known-exception data: - sm100_exceptions.yaml: classify GLM-5 DSA module failures on SM100+rc15/rc17 as framework_version_unsupported (safety net for any gap in the module guard) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Kai Ma <kaim@nvidia.com>
…anent kernel limits Per layer_permissions.md, framework-version and model-name conditions in getter-level skip guards are "exception smuggling" — the only legal responses to a queued case are execute-it or raise. The skip guards added during the B200/H200 power collection run violate this policy and are removed. Failure routing under the observe-don't-predict doctrine: - cudaErrorIllegalAddress (long-context GQA, FP8-KV generation): CUDA-fatal errors reset the worker and are recorded before the reset; run-and-record. - Version-specific SIGABRTs (SM90 MLA generation, GLM-5 DSA DeepGEMM assert, SM100 DSA context prefix): version-specific so the hang denylist cannot encode them without permanently suppressing fixed versions; run-and-record until the framework fixes the underlying issue. - Python-catchable failures (Mamba2 Ultra OOM/NoneType): run-and-record. Two permanent kernel limits in TRT-LLM dense attention are documented with FIXME(kernel-limit) comments at the iteration sites in collect_attn.py: - head_dim > 256: "Head size N is not supported by MMHA" (std::terminate) - head_dim == 192 on SM >= 90: "Unsupported HeadDim for BMM2-N 192" in TllmGenFmha (SM89 is unaffected) Both abort before Python can catch them. Re-verify on each version bump. The sm100_exceptions.yaml addition and all vLLM collector changes were dropped earlier in this rebase (sm-exceptions engine retired in #1302; vLLM collector upgraded to 0.24.0 in #1344). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Kai Ma <kaim@nvidia.com>
…un_attention_torch Two head_dim kernel-limit notes were parked inside get_context_attention_test_cases() and get_generation_attention_test_cases(), contrary to layer_permissions.md which requires FIXME(kernel-limit) comments to live at the runtime invocation site so the affected cases still run and fail at runtime. Consolidate into one canonical note above create_attention() in run_attention_torch(); remove the getter copies. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Kai Ma <kaim@nvidia.com>
87ac25b to
a57e49f
Compare
…in collect_attn Add TRT-LLM version (1.3.0rc15, SM100/SM90) and grep anchors to the head_dim kernel-limit FIXME so the upgrade audit knows exactly where to look to verify or retire the note. Signed-off-by: Kai Ma <kaim@nvidia.com> Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 participant
What changed (and why everything else was removed)
This PR was rebased onto main after a policy review against
layer_permissions.mdandfailure_handling.md. The original diff had seven skip guards and asm100_exceptions.yamladdition; all but twoFIXMEcomments were dropped.What remains: two
FIXME(kernel-limit)comments incollect_attn.pyTRT-LLM dense attention has two permanent kernel limits that abort the worker before Python can catch them:
head_dim > 256→"Head size N is not supported by MMHA"(std::terminateinAttentionOp::initialize)head_dim == 192on SM ≥ 90 →"Unsupported HeadDim for BMM2-N 192"inTllmGenFmha(SM89 is unaffected)These are documented with
FIXME(kernel-limit)comments at the iteration sites so the upgrade audit can grep them and re-verify on each version bump. The cases run and fail rather than being silently skipped.What was removed and why
sm100_exceptions.yamllayer_permissions.md: framework-version / model-name conditions in getter-level skips are "exception smuggling" — only legal responses are execute-or-raisecollect_gemm.pyvLLM 0.22 FP8 pathcollect_vllm/collect_mla_module.pySM100 DSA fp8_block skipFailure routing for the dropped guards
cudaErrorIllegalAddress(SM90 long-context GQA, FP8-KV generation): CUDA-fatal errors reset the worker and are recorded before the reset — run-and-record.All of the above will appear grouped by
(op, model, dtype)inerrors_by_groupin the collection summary, which is the intended fix-me signal.Test plan
collect_attn.pyruns against a head profile that includeshead_dim > 256andhead_dim == 192; verify failures are recorded inerrors_<module>.json, not silently dropped🤖 Generated with Claude Code
Summary by CodeRabbit
head_dimvalues, including cases that may fail before Python-level handling.