Skip to content

feat(collector): upgrade the TRT-LLM collector to 1.3.0rc20 (code + SM90 validation)#1356

Draft
tianhaox wants to merge 4 commits into
ai-dynamo:mainfrom
tianhaox:data/hopper-trtllm-1.3.0rc20
Draft

feat(collector): upgrade the TRT-LLM collector to 1.3.0rc20 (code + SM90 validation)#1356
tianhaox wants to merge 4 commits into
ai-dynamo:mainfrom
tianhaox:data/hopper-trtllm-1.3.0rc20

Conversation

@tianhaox

@tianhaox tianhaox commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

feat(collector): upgrade the TRT-LLM collector to 1.3.0rc20 (code + SM90 validation)

Release at a Glance

  • TRT-LLM stock collector pin: 1.3.0rc10 → 1.3.0rc20
    (nvcr.io/nvidia/tensorrt-llm/release:1.3.0rc20, digest
    sha256:1532b38814b3faf2affdb5ef01ca91468685d314ffb7e8926a0567595355ed88).
  • All 16 stock trtllm registry ops import, smoke, and run shuffled-100 gates
    inside the rc20 container on SM90 (8x H20-3e bring-up node).
  • Three rc10→rc20 API/behavior breaks fixed in the collector; three framework
    regressions identified with source-cited root causes (below), left as
    classified runtime failures per the observe-don't-predict doctrine.
  • Code-only PR: no perf data files change. Hopper (h100_sxm/h200_sxm) data
    collection runs next on real H100/H200 nodes from this branch; B200,
    RTX 6000, L40S follow.

Runtime provenance

  • Image: nvcr.io/nvidia/tensorrt-llm/release:1.3.0rc20 (amd64), digest above.
  • tensorrt_llm.__version__ = 1.3.0rc20 (verified in-container, not from tag).
  • torch 2.11.0a0+eb65b36914.nv26.02, CUDA 13.1, transformers 5.5.4.
  • Bring-up node: 8x NVIDIA H20-3e (SM90, 143771 MiB, 500 W) — used for
    collector validation only; no H20-3e data is published under any system.

Collector changes

  1. collect_mla: 1.3.0rc20 removed the per-request C++
    KVCacheManager.impl.add_sequence binding (serving now batches through
    add_sequence_batch). Context sequences are now registered through the
    framework's own add_dummy_requests warmup path, matching the sibling
    attention/module collectors. The previously loop-leaked ctx_len sizing
    is replaced by sum(context_sequence_lengths) (identical for the uniform
    grids in use).
  2. collect_mla / collect_mla_module: SM90 tokens_per_block=64.
    Serving forces tokens_per_block=64 whenever FlashMLA is eligible
    (model_config.enable_flash_mla: head_dim==576 && SM90;
    py_executor_creator.py override; thop attentionOp.cpp:1235@v1.3.0rc20
    requires SM90 && tpb==64 && head==576). With the old tpb=32 the op takes
    the FMHA generation fallback, which rc20 no longer supports for FP8 KV on
    SM90 ("Deepseek should be supported by fmha in generation part",
    common/attentionOp.cpp:3091) — 13/20 shuffled generation-MLA cases
    crashed before the fix, 0/100 after; the measured path now matches serving
    (FlashMLA) on SM90. Blackwell keeps P32 (trtllm-gen cubins, PR #10261).
  3. collect_mla_module: follows the rc20 sparse-attention APIs —
    cache-manager kwargs sparse_attention_config= + pretrained_config=
    (rc10's sparse_attn_config is silently swallowed by **kwargs in rc20,
    which broke every DSA case), and attention metadata now receives lowered
    SparseMetadataParams instead of the raw config (the raw kwarg was
    removed from TrtllmAttentionMetadata). __compat__ floor raised to
    >=1.3.0rc20 accordingly.
  4. collect_moe: __compat__ ceiling 1.3.0rc15 → 1.3.0rc20 after
    auditing create_moe (signature, backend literals CUTLASS/DEEPGEMM/
    TRITON/TRTLLM, ActivationType) against rc20 — no changes needed beyond the
    ceiling; the existing inspect-based kwargs adaptation covers rc20's new
    optional params.
  5. framework_manifest.yaml: stock trtllm pin + image → 1.3.0rc20
    (wideep pin intentionally untouched — separate runtime/scope).

Validation (SM90, in-container)

  • Import probe: 10/10 collector modules import clean on rc20.
  • Plan audit: --model-cases-full --sm 90 --plan-only resolves 16 stock ops +
    22 model case files (wideep op excluded from stock runs).
  • Smoke (--smoke): 16/16 ops, zero errors.
  • Shuffled gate (--shuffle --limit 100 per op): 10/16 ops zero errors
    (gemm, compute_scale, mla_context, mla_generation, encoder_attention,
    mla_bmm_gen_pre/post, moe, mamba2, gdn). Remaining errors are classified
    below; retry-from-checkpoint separates transient from deterministic.
  • pytest tests/unit/collector -q: 493 passed, 5 skipped.
  • pytest tests/unit/sdk/test_common.py tests/unit/sdk/database -q
    (excl. torch-dependent test_moe_dispatch.py, not installable in the
    frozen host env): 471 passed.
  • ruff check + ruff format --check on collector + tests: clean.
  • Sample parquet checks: rows carry framework=TRTLLM, version=1.3.0rc20,
    finite positive latencies; generation-MLA fp8/bf16 both present; case count
    parity with rc10 (e.g. generation MLA grid 2896 = rc10's 2896 rows).

Framework regressions found (recorded failures, upstream reports to file)

  • FR-1 — SM90 fp8-KV attention without weight quant asserts in
    generation.
    rc20 forces use_paged_context_fmha=True on SM90
    (nvbugs/5624818 workaround) → mFP8ContextFMHA = hasFp8KvCache && use_paged_context_fmha (thop attentionOp.cpp:1155) → generation dispatch
    requires an out_scale that kv-cache-only-quant models never provide
    (serving's _use_quantize_output() returns False for them). bf16-weights +
    fp8-KV serving on SM90 hits the same assert — regression vs rc10. Affected
    collector cases (fp8_kv + bf16 fmha) fail classified; substituting a
    synthetic out_scale would mislabel the FP8-FMHA kernel as bf16, so it is
    not done.

  • FR-2 — SM90 long-context large-head GQA context FMHA IMA.
    (h=96,kv=8,hd=256) and (h=48,kv=4,hd=256) at 131072 total tokens die with
    cudaErrorIllegalAddress; nearest shorter-token siblings pass. Same family
    as the documented SM89-rc15 long-context GQA failures. Deterministic on
    retry.

  • FR-3 — GLM-5.2 (glm_moe_dsa) unloadable with image transformers 5.5.4.
    ModelConfig.from_pretrained("nvidia/GLM-5.2-NVFP4") fails strict
    layer_types validation (deepseek_sparse_attention not allowed) through
    TRT-LLM's own glm_moe_dsa -> DeepseekV3Config registry mapping — serving
    in this image is equally broken. GLM-5.2 DSA rows will be absent from rc20
    datasets until upstream fixes the transformers pin/config path.

  • FR-4 — GLM-5 DSA context + fp8_block projections: shape-dependent IMA.
    3 isolated shape points across the three GLM-5 artifacts fail with an async
    illegal-memory-access (deterministic for at least one point across two
    runs); 6 fp8_block and 26 bf16 sibling rows pass in the same sweep.
    Recorded, no skip.

Transient/infra (clears on fresh rerun with warm HF cache): HuggingFace
_remote_code.lock contention in module ops (21 → 4 residual sporadic
timeouts); one non-reproducing mla-module IMA reclassified as peer-crash
collateral.

Also observed (pre-existing, executor scope, NOT touched here):
--resume --resume-retry-failed deadlocked at 0/17 on attention_generation
(parent + 8 workers alive but blocked ~55 min) — matches the existing
debug/collector_versions_resume_stuck investigation branch. Fresh
same-seed reruns used as the retry evidence instead.

Remaining risks / follow-ups

  • Hopper data delivery (h100_sxm/h200_sxm) still to run from this branch on
    real hopper nodes; then B200 (SM100), RTX PRO 6000 (SM120), L40S (SM89).
  • The three parked FIXME(kernel-limit) notes (SM100/120 GQA ratio, FP8 MLA
    BMM SM window, module FP8-KV platform splits) remain hardware-unvalidated
    on this SM90-only node; re-verify during the Blackwell/Ada phases.
  • nvfp4 GEMM custom ops (fp4_quantize, block_scale_interleave_reverse)
    exist in rc20 (source probe) but are SM100+ and hardware-unvalidated here.

Follow-up alignment work landed in this PR (post-review findings)

  • GLM-5.2 (glm_moe_dsa) DSA collection unblocked: mirrored the upstream
    main config normalization (drop the HF-bookkeeping-only layer_types) so
    ModelConfig.from_pretrained loads GLM-5.2 in the rc20 image. Full-grid
    validation shows the remaining GLM-5.2 gap is its cross-layer indexer
    reuse (index_topk_freq=4/index_skip_topk_offset=3): large-shape
    indexer-skip buffer sizing fails in-runtime, and modeling those layers
    needs an sglang-style dsa_*_module_skip_indexer decomposition that
    trtllm does not have yet — deferred as a scoped follow-up (like DSV4).
  • Module dry-run failures now raise instead of silently returning with
    no perf row (the executor records a classified failure; previously a
    green run could silently miss rows).
  • Kimi-K2.5 MoonViT encoder attention enabled for trtllm: rc20 natively
    serves the full K2.5 VLM (modeling_kimi_k25.py); the vision
    encoder_attention profile (16 heads x head_dim 72, TP shards) now
    collects on trtllm — 1100 cases, zero errors on SM90. Closes the vision
    parity gap vs the vLLM 0.24.0 dataset.
  • Capability parity audit vs sglang 0.5.14 / vllm 0.24.0 (h200):
    remaining true gaps are MoE int4_wo (TRT-LLM torch flow has no plain
    W4A16 MoE method on any SM — CUTLASS maps int4-per-group to W4AFP8,
    Marlin is NVFP4-only, Triton is MXFP4-only; Blackwell datasets also have
    zero int4_wo rows) and the DSV4 op family (deferred by owner direction).
    FP8-KV DSA on SM90 is verified kernel-fact (FlashMLA sparse is bf16-only;
    the fp8-capable flash_mla_with_kvcache ships unwired in rc20).

- pin trtllm stock runtime to 1.3.0rc20 (nvcr.io/nvidia/tensorrt-llm/release:1.3.0rc20)
- collect_mla: register context sequences via the framework
  add_dummy_requests path; 1.3.0rc20 removed the per-request
  impl.add_sequence binding in favor of add_sequence_batch
- collect_mla / collect_mla_module: use tokens_per_block=64 on SM90 to
  match serving (FlashMLA is forced for head_dim==576 on SM90 and the
  rc20 SM90 FMHA generation fallback rejects FP8 KV cache)
- collect_mla_module: follow 1.3.0rc20 sparse APIs
  (sparse_attention_config/pretrained_config cache-manager kwargs,
  SparseMetadataParams attention metadata)
- collect_moe: bump validated __compat__ ceiling to 1.3.0rc20

Smoke-validated on 8x H20-3e (SM90): all 16 stock trtllm registry ops
pass --smoke with zero errors inside the 1.3.0rc20 release container.

Signed-off-by: Tianhao Xu <49143331+tianhaox@users.noreply.github.com>
@copy-pr-bot

copy-pr-bot Bot commented Jul 14, 2026

Copy link
Copy Markdown

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions github-actions Bot added the feat label Jul 14, 2026
@coderabbitai

coderabbitai Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

TRT-LLM support now targets 1.3.0rc20. MLA configuration and execution handle updated GLM-MoE-DSA compatibility, SM90 sizing, sparse metadata APIs, dummy-request cache registration, packed context tensors, and Kimi K2.5 encoder attention planning.

Changes

TRT-LLM 1.3.0rc20 support

Layer / File(s) Summary
Compatibility contract
collector/framework_manifest.yaml, collector/trtllm/collect_mla_module.py, collector/trtllm/collect_moe.py, tests/unit/collector/*
Framework metadata, compatibility gates, and test fixtures now target TRT-LLM 1.3.0rc20.
MLA configuration and cache setup
collector/trtllm/collect_mla_module.py
GLM-MoE-DSA configs are normalized before loading; SM90 FlashMLA eligibility controls block sizing, and updated cache-manager and sparse metadata parameters are wired into construction.
MLA runtime execution
collector/trtllm/collect_mla.py
MLA sizing considers SM90, KV cache registration uses dummy requests, and context tensors use packed totals for variable sequence lengths.
Kimi K2.5 planning
collector/cases/models/KimiK25ForConditionalGeneration_cases.yaml, tests/unit/collector/test_model_cases.py
Kimi K2.5 model cases and TRT-LLM plan expectations now include encoder_attention.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Poem

RC twenty takes the stage,
Packed tokens turn the page.
SM90 sparks and caches align,
Sparse metadata flows in time,
Kimi’s encoder joins the play.

🚥 Pre-merge checks | ✅ 3 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is detailed, but it does not follow the required template and omits the reviewer-start and related-issues sections. Add the required Overview, Details, Where should reviewer start?, and Related Issues sections, including a closing issue reference.
✅ Passed checks (3 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main change: upgrading the TRT-LLM collector to 1.3.0rc20 with validation.

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
collector/trtllm/collect_mla_module.py (1)

523-523: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Prefer the shared get_sm_version() helper over a raw torch.cuda.get_device_capability() call.

collect_mla.py's equivalent check uses helper.get_sm_version() (with a cuda-python fallback when torch isn't available); this file calls torch.cuda.get_device_capability() directly. Using the shared helper keeps SM-detection semantics consistent across collectors and avoids duplicating fallback logic.

Also applies to: 588-588

🤖 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_module.py` at line 523, Update the SM90 checks
assigned to is_sm90_flash_mla to use the shared helper.get_sm_version() logic,
matching the equivalent collect_mla.py implementation and its cuda-python
fallback when torch is unavailable. Apply the same change to both occurrences
while preserving the existing head_dim == 576 condition.
🤖 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_mla_module.py`:
- Around line 507-527: The TRT-LLM FlashMLA kernel-limit workaround comments
lack the required FIXME tag and verification status. In
collector/trtllm/collect_mla_module.py lines 507-527 and
collector/trtllm/collect_mla.py lines 37-53, prefix each existing comment block
with “FIXME(kernel-limit):” and explicitly state the documented framework limit,
its TRT-LLM/source origin, and that the workaround is unverified and must be
rechecked when versions change.

---

Nitpick comments:
In `@collector/trtllm/collect_mla_module.py`:
- Line 523: Update the SM90 checks assigned to is_sm90_flash_mla to use the
shared helper.get_sm_version() logic, matching the equivalent collect_mla.py
implementation and its cuda-python fallback when torch is unavailable. Apply the
same change to both occurrences while preserving the existing head_dim == 576
condition.
🪄 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: 87cd5275-b5b6-4660-9180-0101277d3ae1

📥 Commits

Reviewing files that changed from the base of the PR and between 2feebac and ab52e90.

📒 Files selected for processing (6)
  • collector/framework_manifest.yaml
  • collector/trtllm/collect_mla.py
  • collector/trtllm/collect_mla_module.py
  • collector/trtllm/collect_moe.py
  • tests/unit/collector/test_framework_manifest.py
  • tests/unit/collector/test_getter_deduplication.py
📜 Review details
⏰ Context from checks skipped due to timeout. (6)
  • GitHub Check: Collect snapshot (new)
  • GitHub Check: Collect snapshot (old)
  • GitHub Check: Rust/Python engine-step parity
  • GitHub Check: Cargo Deny
  • GitHub Check: Build and Test (unit)
  • GitHub Check: Build and Test (e2e)
🧰 Additional context used
📓 Path-based instructions (10)
{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/framework_manifest.yaml
  • collector/trtllm/collect_moe.py
  • tests/unit/collector/test_getter_deduplication.py
  • tests/unit/collector/test_framework_manifest.py
  • collector/trtllm/collect_mla.py
  • collector/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-level min_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 only collector/ and tests/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/framework_manifest.yaml
  • collector/trtllm/collect_moe.py
  • collector/trtllm/collect_mla.py
  • collector/trtllm/collect_mla_module.py

⚙️ CodeRabbit configuration file

collector/**: - Enforce the collector rules from .claude/rules/collector/layer_permissions.md, failure_handling.md, and case_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.yaml is for hang/node-killers only, dated.
  • Collector changes must stay within collector/ and tests/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/framework_manifest.yaml
  • collector/trtllm/collect_moe.py
  • collector/trtllm/collect_mla.py
  • collector/trtllm/collect_mla_module.py
collector/**/*

📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)

collector/**/*: When editing or reviewing collector/**, load and apply layer_permissions.md and failure_handling.md; also load case_authoring.md for case YAML work.
Do not apply generator-module rules to collector/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/framework_manifest.yaml
  • collector/trtllm/collect_moe.py
  • collector/trtllm/collect_mla.py
  • collector/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/framework_manifest.yaml
  • collector/trtllm/collect_moe.py
  • tests/unit/collector/test_getter_deduplication.py
  • tests/unit/collector/test_framework_manifest.py
  • collector/trtllm/collect_mla.py
  • collector/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 actual kernel_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, and kernel_source must 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 inside get_*_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 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.
On framework version bumps, re-verify every pinned backend and every FIXME(kernel-limit) against framework source before trusting collected data.

Files:

  • collector/trtllm/collect_moe.py
  • collector/trtllm/collect_mla.py
  • collector/trtllm/collect_mla_module.py
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Use the project’s uv-managed environment and run linting with ruff check . and ruff format --check ..

Files:

  • collector/trtllm/collect_moe.py
  • tests/unit/collector/test_getter_deduplication.py
  • tests/unit/collector/test_framework_manifest.py
  • collector/trtllm/collect_mla.py
  • collector/trtllm/collect_mla_module.py
tests/unit/collector/**/*.py

📄 CodeRabbit inference engine (.claude/rules/collector/layer_permissions.md)

Mark every new collector unit test with pytest.mark.unit; otherwise tests that work only with -m unit are invisible to CI.

Files:

  • tests/unit/collector/test_getter_deduplication.py
  • tests/unit/collector/test_framework_manifest.py
tests/**/*.{py,yaml,txt,sh}

📄 CodeRabbit inference engine (.claude/rules/generator/testing.md)

Use integration tests for the full input-to-artifacts pipeline, comparing output against golden snapshots without external dependencies.

Files:

  • tests/unit/collector/test_getter_deduplication.py
  • tests/unit/collector/test_framework_manifest.py
tests/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Run unit tests with pytest -m unit; run the PR build subset with pytest -m "unit or build" when applicable.

Files:

  • tests/unit/collector/test_getter_deduplication.py
  • tests/unit/collector/test_framework_manifest.py
tests/**

⚙️ CodeRabbit configuration file

tests/**: - Check that tests cover the changed behavior rather than only the happy path.

  • Watch for fixtures or golden outputs that mask backend drift, support-matrix ordering changes, or CLI output regressions.

Files:

  • tests/unit/collector/test_getter_deduplication.py
  • tests/unit/collector/test_framework_manifest.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_moe.py
  • tests/unit/collector/test_getter_deduplication.py
  • tests/unit/collector/test_framework_manifest.py
  • collector/trtllm/collect_mla.py
  • collector/trtllm/collect_mla_module.py
🔇 Additional comments (9)
collector/framework_manifest.yaml (1)

14-16: LGTM!

collector/trtllm/collect_mla_module.py (2)

12-14: LGTM!


569-576: LGTM!

Also applies to: 593-625

collector/trtllm/collect_moe.py (1)

12-12: LGTM!

tests/unit/collector/test_framework_manifest.py (1)

31-32: LGTM!

tests/unit/collector/test_getter_deduplication.py (1)

57-57: LGTM!

collector/trtllm/collect_mla.py (3)

415-443: LGTM!


37-53: 🎯 Functional Correctness

No issue: this collector only emits the fixed DeepSeek MLA geometry on TRTLLM. The extra head_dim/backend guard isn’t needed here.

			> Likely an incorrect or invalid review comment.

367-396: 🩺 Stability & Availability

No issue here — add_dummy_requests() seeds the same request IDs that impl.add_token() advances in the generation branch.

			> Likely an incorrect or invalid review comment.

Comment on lines 507 to +527

kv_lora_rank = config.kv_lora_rank
qk_rope_head_dim = config.qk_rope_head_dim
head_dim = kv_lora_rank + qk_rope_head_dim

# SM90 serving forces tokens_per_block=64 for FlashMLA-eligible MLA models
# (head_dim==576): model_config.enable_flash_mla
# (tensorrt_llm/_torch/model_config.py@v1.3.0rc20) plus the
# py_executor_creator.py override; the attention op enables FlashMLA only
# for SM90 && tokens_per_block==64
# (cpp/tensorrt_llm/thop/attentionOp.cpp:1235@v1.3.0rc20), and its
# 1.3.0rc20 SM90 FMHA generation fallback rejects FP8 KV cache.
# TRT-LLM PR #10261 (>=1.3.0rc0) dropped numTokensPerPage=64 trtllm-gen MLA
# cubins for DeepSeek-V3 dims (headDimQk=576, headDimV=512) on Blackwell;
# only P32 remains there.
if tensorrt_llm.__version__ >= "1.3.0rc0":
is_sm90_flash_mla = torch.cuda.get_device_capability() == (9, 0) and head_dim == 576
tokens_per_block = 64 if is_sm90_flash_mla else 32
else:
tokens_per_block = 64

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Tag framework kernel-limit workarounds as FIXME(kernel-limit). Both sites encode the same TRT-LLM SM90/head_dim/version-gated FlashMLA cubin limitation with good source citations, but neither uses the guideline-mandated FIXME(kernel-limit) tag stating the limit, origin, and unverified status — required so future version bumps re-verify this against framework source.

  • collector/trtllm/collect_mla_module.py#L507-L527: prefix the existing comment block with FIXME(kernel-limit): and add an explicit "unverified" note.
  • collector/trtllm/collect_mla.py#L37-L53: same tagging change on _mla_tokens_per_block's comment block.
📍 Affects 2 files
  • collector/trtllm/collect_mla_module.py#L507-L527 (this comment)
  • collector/trtllm/collect_mla.py#L37-L53
🤖 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_module.py` around lines 507 - 527, The TRT-LLM
FlashMLA kernel-limit workaround comments lack the required FIXME tag and
verification status. In collector/trtllm/collect_mla_module.py lines 507-527 and
collector/trtllm/collect_mla.py lines 37-53, prefix each existing comment block
with “FIXME(kernel-limit):” and explicitly state the documented framework limit,
its TRT-LLM/source origin, and that the workaround is unverified and must be
rechecked when versions change.

Sources: Coding guidelines, Path instructions

glm_moe_dsa checkpoints tag layers with
layer_types=['deepseek_sparse_attention', ...], which the transformers
5.5.x validator rejects, so ModelConfig.from_pretrained() cannot load
GLM-5.2 at all in the 1.3.0rc20 image (43/59 module-op failures in the
shuffled-100 gate). Upstream main drops the HF-bookkeeping-only field
before building the config (config_utils.py glm_moe_dsa branch, not in
any released rc yet); mirror that by materializing a config-only local
copy without the inert field and letting ModelConfig.from_pretrained run
unmodified on it — DSA layer routing stays driven by index_topk_* as
upstream documents, so the invoked kernels are unchanged.

Signed-off-by: Tianhao Xu <49143331+tianhaox@users.noreply.github.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_mla_module.py`:
- Around line 391-405: Harden the staging flow around the atomic rename: ensure
the temporary directory created by `staging` is removed on any failure before
successful completion, including copy, JSON load, and JSON dump errors. In the
`os.replace` exception path, only treat the failure as another worker winning
when `dst` now exists; otherwise clean up `staging` and propagate the original
error instead of returning an invalid path. Preserve the existing successful
rename and cleanup behavior.
- Around line 369-405: Update _config_dir_without_layer_types to detect local
directories with os.path.isdir(model_path) before calling snapshot_download. For
local model paths, copy the directory into the existing cache staging flow and
remove layer_types from its config.json there; only use snapshot_download for
Hub model identifiers, preserving the current cached atomic-rename behavior.
🪄 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: da005fba-5a25-4113-b818-a9076d6586cd

📥 Commits

Reviewing files that changed from the base of the PR and between ab52e90 and 8635f2b.

📒 Files selected for processing (1)
  • collector/trtllm/collect_mla_module.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 (unit)
  • GitHub Check: Build and Test (e2e)
  • GitHub Check: Cargo Deny
  • GitHub Check: Rust/Python engine-step parity
🧰 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_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-level min_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 only collector/ and tests/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_mla_module.py

⚙️ CodeRabbit configuration file

collector/**: - Enforce the collector rules from .claude/rules/collector/layer_permissions.md, failure_handling.md, and case_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.yaml is for hang/node-killers only, dated.
  • Collector changes must stay within collector/ and tests/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_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 actual kernel_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, and kernel_source must 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 inside get_*_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 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.
On framework version bumps, re-verify every pinned backend and every FIXME(kernel-limit) against framework source before trusting collected data.

Files:

  • collector/trtllm/collect_mla_module.py
collector/**/*

📄 CodeRabbit inference engine (.claude/rules/repo-guide.md)

collector/**/*: When editing or reviewing collector/**, load and apply layer_permissions.md and failure_handling.md; also load case_authoring.md for case YAML work.
Do not apply generator-module rules to collector/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_mla_module.py
**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Use the project’s uv-managed environment and run linting with ruff check . and ruff format --check ..

Files:

  • collector/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/trtllm/collect_mla_module.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_mla_module.py
🪛 ast-grep (0.44.1)
collector/trtllm/collect_mla_module.py

[warning] 393-393: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(staging, "config.json"), encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(open-filename-from-request)


[warning] 396-396: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(staging, "config.json"), "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(open-filename-from-request)

🔇 Additional comments (5)
collector/trtllm/collect_mla_module.py (5)

560-580: 📐 Maintainability & Code Quality | ⚡ Quick win

Reiterate: tag the SM90 FlashMLA tokens_per_block kernel-limit workaround as FIXME(kernel-limit).

This logic (SM90 + head_dim==576 eligibility check gating tokens_per_block) is the same TRT-LLM cubin-limit workaround flagged in a prior review round requesting a FIXME(kernel-limit) tag stating the limit, origin, and unverified status, so future rc bumps re-verify it against framework source. The code isn't shown in this pass to confirm whether the tag was added — please confirm it's present, or add it per the path instructions.

As per path instructions, "Framework kernel limits must remain as FIXME(kernel-limit) comments at the owning collector invocation site, stating the limit, origin, and unverified status."


12-14: LGTM!


408-453: Well-documented GLM-5.2 shim; only concern is the underlying helper (see comment on lines 369-405).

The upstream citation and rationale for the layer_types removal are clear and the framework code paths are left untouched, consistent with the "manual pin requires source citation" principle.


622-627: 🎯 Functional Correctness | ⚡ Quick win

Verify the rc20 sparse-attention kwargs against actual TRT-LLM source.

The switch from sparse_attn_config= to sparse_attention_config=/pretrained_config=config, and from a sparse_attention_config kwarg to sparse_metadata_params computed via to_sparse_metadata_params(pretrained_config=config), is exactly the kind of framework-surface change the guidelines call out for re-verification on version bumps. The PR's stated 16/16 smoke pass and unit-test results give some confidence, but please confirm these exact kwarg names/signatures match the pinned tensorrt_llm==1.3.0rc20 release (not a newer main-branch API) since a silent kwarg mismatch would raise immediately rather than corrupt data, but is worth a final source check.

As per path instructions, "On framework version bumps, re-verify every pinned backend and every FIXME(kernel-limit) against framework source before trusting collected data."

Also applies to: 646-654, 677-677


383-385: 📐 Maintainability & Code Quality

No action needed: huggingface-hub is already pulled in transitively by gradio in uv.lock, so this import should resolve in uv-managed environments.

			> Likely an incorrect or invalid review comment.

Comment on lines +369 to +405
def _config_dir_without_layer_types(model_path: str) -> str:
"""Materialize a config-only local copy of ``model_path`` minus ``layer_types``.

Mirrors the upstream TRT-LLM main fix for glm_moe_dsa (config_utils.py
drops the HF-bookkeeping-only ``layer_types`` before building the config;
transformers 5.5.x rejects its 'deepseek_sparse_attention' entries). Only
JSON config files are fetched — module benchmarks never load weights —
and ``ModelConfig.from_pretrained`` then runs unmodified on the local dir,
so the framework's own config/quant handling stays authoritative.
"""
import hashlib
import json
import shutil

from huggingface_hub import snapshot_download

src = snapshot_download(model_path, allow_patterns=["*.json"])
dst = os.path.join(
os.path.expanduser("~/.cache/aic_collector/glm_dsa_config_norm"),
hashlib.sha1(src.encode()).hexdigest()[:16],
)
config_path = os.path.join(dst, "config.json")
if not os.path.exists(config_path):
staging = f"{dst}.tmp-{os.getpid()}"
shutil.copytree(src, staging, dirs_exist_ok=True)
with open(os.path.join(staging, "config.json"), encoding="utf-8") as f:
cfg = json.load(f)
cfg.pop("layer_types", None)
with open(os.path.join(staging, "config.json"), "w", encoding="utf-8") as f:
json.dump(cfg, f, indent=2)
os.makedirs(os.path.dirname(dst), exist_ok=True)
try:
os.replace(staging, dst)
except OSError:
# Another worker won the atomic-rename race; use its copy.
shutil.rmtree(staging, ignore_errors=True)
return dst

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🌐 Web query:

Does huggingface_hub snapshot_download accept a local directory path instead of a repo_id?

💡 Result:

No, the snapshot_download function in the huggingface_hub library does not accept a local directory path as a replacement for the repo_id parameter [1][2]. The repo_id parameter is required and must correspond to the identifier of a repository hosted on the Hugging Face Hub (e.g., "username/repo_name") [3][4]. While snapshot_download is designed to fetch files from a remote repository, it does provide a local_dir parameter [1][2]. This parameter allows you to specify a local directory where the downloaded repository files should be replicated or symlinked, rather than relying solely on the default Hugging Face cache directory [1][2]. If you need to work with files already existing in a local directory, you should use standard file system operations or other relevant libraries instead of snapshot_download [1].

Citations:


Avoid snapshot_download for local model_path
snapshot_download() requires a Hub repo id. If model_path is already a local checkpoint directory, this helper will fail before ModelConfig.from_pretrained() runs. Branch on os.path.isdir(model_path) and rewrite the JSON in a local copy instead.

🧰 Tools
🪛 ast-grep (0.44.1)

[warning] 393-393: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(staging, "config.json"), encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(open-filename-from-request)


[warning] 396-396: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(staging, "config.json"), "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(open-filename-from-request)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@collector/trtllm/collect_mla_module.py` around lines 369 - 405, Update
_config_dir_without_layer_types to detect local directories with
os.path.isdir(model_path) before calling snapshot_download. For local model
paths, copy the directory into the existing cache staging flow and remove
layer_types from its config.json there; only use snapshot_download for Hub model
identifiers, preserving the current cached atomic-rename behavior.

Comment on lines +391 to +405
if not os.path.exists(config_path):
staging = f"{dst}.tmp-{os.getpid()}"
shutil.copytree(src, staging, dirs_exist_ok=True)
with open(os.path.join(staging, "config.json"), encoding="utf-8") as f:
cfg = json.load(f)
cfg.pop("layer_types", None)
with open(os.path.join(staging, "config.json"), "w", encoding="utf-8") as f:
json.dump(cfg, f, indent=2)
os.makedirs(os.path.dirname(dst), exist_ok=True)
try:
os.replace(staging, dst)
except OSError:
# Another worker won the atomic-rename race; use its copy.
shutil.rmtree(staging, ignore_errors=True)
return dst

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Broad except OSError assumes a race-win without verifying it, and staging leaks on other failures.

Two related robustness gaps in the atomic-rename block:

  1. Any OSError from os.replace (permission error, disk full, cross-device, etc.) is swallowed under the "another worker won" assumption without checking that dst actually exists — silently returning a path that may not be valid.
  2. If shutil.copytree, json.load, or json.dump raise before the rename is reached, staging is never cleaned up, leaking a partial directory under ~/.cache/aic_collector/.
🛠️ Proposed fix
         staging = f"{dst}.tmp-{os.getpid()}"
-        shutil.copytree(src, staging, dirs_exist_ok=True)
-        with open(os.path.join(staging, "config.json"), encoding="utf-8") as f:
-            cfg = json.load(f)
-        cfg.pop("layer_types", None)
-        with open(os.path.join(staging, "config.json"), "w", encoding="utf-8") as f:
-            json.dump(cfg, f, indent=2)
-        os.makedirs(os.path.dirname(dst), exist_ok=True)
-        try:
-            os.replace(staging, dst)
-        except OSError:
-            # Another worker won the atomic-rename race; use its copy.
-            shutil.rmtree(staging, ignore_errors=True)
+        try:
+            shutil.copytree(src, staging, dirs_exist_ok=True)
+            with open(os.path.join(staging, "config.json"), encoding="utf-8") as f:
+                cfg = json.load(f)
+            cfg.pop("layer_types", None)
+            with open(os.path.join(staging, "config.json"), "w", encoding="utf-8") as f:
+                json.dump(cfg, f, indent=2)
+            os.makedirs(os.path.dirname(dst), exist_ok=True)
+            os.replace(staging, dst)
+        except OSError:
+            shutil.rmtree(staging, ignore_errors=True)
+            # Only treat this as a benign lost race if a sibling worker's
+            # copy is actually there; otherwise surface the real error.
+            if not os.path.exists(config_path):
+                raise
🧰 Tools
🪛 ast-grep (0.44.1)

[warning] 393-393: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(staging, "config.json"), encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(open-filename-from-request)


[warning] 396-396: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(os.path.join(staging, "config.json"), "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').

(open-filename-from-request)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@collector/trtllm/collect_mla_module.py` around lines 391 - 405, Harden the
staging flow around the atomic rename: ensure the temporary directory created by
`staging` is removed on any failure before successful completion, including
copy, JSON load, and JSON dump errors. In the `os.replace` exception path, only
treat the failure as another worker winning when `dst` now exists; otherwise
clean up `staging` and propagate the original error instead of returning an
invalid path. Preserve the existing successful rename and cleanup behavior.

tianhaox added 2 commits July 14, 2026 20:53
… floor

- run_mla_module swallowed dry-run exceptions (print + return), leaving
  green runs with silently missing perf rows. Raise instead so the
  executor records a classified failure; the standalone main() loop
  already handles per-case exceptions.
- Verify the parked FIXME(kernel-limit) DSA claim on 1.3.0rc20/SM90:
  the SM90 DSA core dispatches to FlashMLA sparse kernels whose
  sparse_prefill_fwd asserts kv.dtype()==kBFloat16
  (flashmla-src/csrc/pybind.cpp:404), so FP8-KV DSA is genuinely
  Blackwell-only (trtllm-gen sparseMla) and the SM>=100 combo floor in
  _get_precision_combos is correct. FP8-KV MLA modules pass on SM90;
  the SM100+ MLA half of the note stays parked for the Blackwell phase.

Signed-off-by: Tianhao Xu <49143331+tianhaox@users.noreply.github.com>
TRT-LLM 1.3.0rc20 natively serves the full K2.5 VLM including the
MoonViT3d tower (modeling_kimi_k25.py: 27-layer encoder,
vt_hidden_size=1152 / vt_num_attention_heads=16 -> head_dim 72), so the
existing vLLM-only encoder_attention activation now applies to trtllm
too — closing the Kimi vision parity gap vs the vLLM 0.24.0 dataset.
Validated on SM90: the targeted trtllm plan generates 1100 encoder
cases (TP shards 16/8/4/2/1 heads x head_dim 72) and completes with
zero errors in the 1.3.0rc20 container.

Signed-off-by: Tianhao Xu <49143331+tianhaox@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant