Repository navigation
Conversation
hchings
left a comment
There was a problem hiding this comment.
Team review: Qwen3.5 routed-expert block-FP8 / MXFP8 refit
Reviewed by a team of 4 agents plus a leader re-verification pass over the full diff, with every upstream claim checked against the TensorRT-LLM commit this repo pins rather than main. 8 comments below.
The quantization module itself is good, and that's the bulk of the work here. Worth saying explicitly:
quantization/fp8.pyimports onlymath/re/torchand takes TRT-LLM types as parameters (configure_fp8_moe_backend(llm_kwargs, moe_config_type, ...)) rather than importing them. That's why the 223-line test file exercises the real quantization math on CPU with a 6-line stub and no engine mocking — a strictly better shape than thevllm/quantization/fp8.pyanalog, which routes the same decisions through a module-level mutable global.- We checked the numerics against the pinned consumer and they're right: the VANILLA per-expert split is what
Qwen3_5MoeHfWeightMapperexpects, the[ceil(N/128), ceil(K/128)]FP32 scale orientation anddequant = fp8 * scale_invmatchcreate_weights, the MXFP8 plain-uint8[..., K/32]layout matchesMXFP8CutlassFusedMoEMethod(which does its own packing and swizzle, so not pre-swizzling is correct), and all 13modules_to_not_convertpatterns translate correctly through upstream's Qwen3.5 normalizer. The block-FP8 caster was validated on CPU across aligned, unaligned, batched and non-contiguous inputs. - The multi-worker result aggregation fix in
trtllm_worker_async.pyis a genuine improvement — the oldresults[0]masked failures on ranks > 0, and the updated tests cover it.
The findings share one root cause, which is why several read similarly: this was developed against a newer TensorRT-LLM than the repo pins, and the branch appears not to have been run against the pinned build. Three of the comments below are consequences of that (recompute_active_requests, the FP8 refit hooks, the torch.compile hooks); one is an unrelated rebase artifact that breaks all TRT-LLM runs (engine_cfg); and three are test/doc drift. None of them touch the design.
Generated by Claude Code
0fbfddf to
9d6eb42
Compare
Retested on GB200 (SLURM job 3126032) after the fixes: all 44 trtllm unit tests pass (was 41 passed / 3 failed). - tests/.../test_trtllm_backend.py: force the manual finalize fallback in test_collective_refit_runs_at_async_engine_boundary via `monkeypatch.setattr(backend.WorkerExtension, "finalize_weight_update", None, raising=False)`. rc28's WorkerExtension now has finalize_weight_update (rc21 didn't), so trtllm_backend.py's _finalize_weight_update silently switched to that fast path and the test's hardcoded call-order assertion (written only for the fallback) started failing. Ported verbatim from the equivalent fix already landing in #4122 (which adds the real finalize_weight_update-aware FP8 refit logic later) — this commit only makes the test deterministic across TRT-LLM versions, it does not implement anything new. - nemo_rl/models/generation/trtllm/trtllm_backend.py: import control_action_decorator from tensorrt_llm.executor.ray.utils (the rc28 location) with a fallback to the old tensorrt_llm._ray_utils path. The old path still works but emits "will be removed in a future release"; fixing now avoids a silent break on the next bump. - uv.lock: regenerated against the bumped ref (was still rc21-era). Resolves cleanly (529 packages), confirms tensorrt/tensorrt-cu13/ tensorrt-cu13-bindings/tensorrt-cu13-libs are no longer pulled in (matches upstream dropping its own tensorrt pin). Note: it resolves torch and nvidia-cudnn-frontend via this repo's override-dependencies (torch==2.11.0+cu130, nvidia-cudnn-frontend==1.25.0) rather than what TRT-LLM's own dependencies actually declare (torch>=2.13.0, nvidia-cudnn-frontend>=1.27.0) — uv's override mechanism forces the version project-wide without validating the sub-dependency is actually satisfied. This is the same conflict already flagged in 3rdparty/TensorRT-LLM-workspace/pyproject.toml's "KNOWN UNRESOLVED CONFLICTS" comment; still unresolved, not addressed by this commit.
Retested on GB200 (SLURM job 3126032) after the fixes: all 44 trtllm unit tests pass (was 41 passed / 3 failed). - tests/.../test_trtllm_backend.py: force the manual finalize fallback in test_collective_refit_runs_at_async_engine_boundary via `monkeypatch.setattr(backend.WorkerExtension, "finalize_weight_update", None, raising=False)`. rc28's WorkerExtension now has finalize_weight_update (rc21 didn't), so trtllm_backend.py's _finalize_weight_update silently switched to that fast path and the test's hardcoded call-order assertion (written only for the fallback) started failing. Ported verbatim from the equivalent fix already landing in #4122 (which adds the real finalize_weight_update-aware FP8 refit logic later) — this commit only makes the test deterministic across TRT-LLM versions, it does not implement anything new. - nemo_rl/models/generation/trtllm/trtllm_backend.py: import control_action_decorator from tensorrt_llm.executor.ray.utils (the rc28 location) with a fallback to the old tensorrt_llm._ray_utils path. The old path still works but emits "will be removed in a future release"; fixing now avoids a silent break on the next bump. - uv.lock: regenerated against the bumped ref (was still rc21-era). Resolves cleanly (529 packages), confirms tensorrt/tensorrt-cu13/ tensorrt-cu13-bindings/tensorrt-cu13-libs are no longer pulled in (matches upstream dropping its own tensorrt pin). Note: it resolves torch and nvidia-cudnn-frontend via this repo's override-dependencies (torch==2.11.0+cu130, nvidia-cudnn-frontend==1.25.0) rather than what TRT-LLM's own dependencies actually declare (torch>=2.13.0, nvidia-cudnn-frontend>=1.27.0) — uv's override mechanism forces the version project-wide without validating the sub-dependency is actually satisfied. This is the same conflict already flagged in 3rdparty/TensorRT-LLM-workspace/pyproject.toml's "KNOWN UNRESOLVED CONFLICTS" comment; still unresolved, not addressed by this commit. Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
Retested on GB200 (SLURM job 3126032) after the fixes: all 44 trtllm unit tests pass (was 41 passed / 3 failed). - tests/.../test_trtllm_backend.py: force the manual finalize fallback in test_collective_refit_runs_at_async_engine_boundary via `monkeypatch.setattr(backend.WorkerExtension, "finalize_weight_update", None, raising=False)`. rc28's WorkerExtension now has finalize_weight_update (rc21 didn't), so trtllm_backend.py's _finalize_weight_update silently switched to that fast path and the test's hardcoded call-order assertion (written only for the fallback) started failing. Ported verbatim from the equivalent fix already landing in #4122 (which adds the real finalize_weight_update-aware FP8 refit logic later) — this commit only makes the test deterministic across TRT-LLM versions, it does not implement anything new. - nemo_rl/models/generation/trtllm/trtllm_backend.py: import control_action_decorator from tensorrt_llm.executor.ray.utils (the rc28 location) with a fallback to the old tensorrt_llm._ray_utils path. The old path still works but emits "will be removed in a future release"; fixing now avoids a silent break on the next bump. - uv.lock: regenerated against the bumped ref (was still rc21-era). Resolves cleanly (529 packages), confirms tensorrt/tensorrt-cu13/ tensorrt-cu13-bindings/tensorrt-cu13-libs are no longer pulled in (matches upstream dropping its own tensorrt pin). Note: it resolves torch and nvidia-cudnn-frontend via this repo's override-dependencies (torch==2.11.0+cu130, nvidia-cudnn-frontend==1.25.0) rather than what TRT-LLM's own dependencies actually declare (torch>=2.13.0, nvidia-cudnn-frontend>=1.27.0) — uv's override mechanism forces the version project-wide without validating the sub-dependency is actually satisfied. This is the same conflict already flagged in 3rdparty/TensorRT-LLM-workspace/pyproject.toml's "KNOWN UNRESOLVED CONFLICTS" comment; still unresolved, not addressed by this commit. Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
43efd59 to
0807be9
Compare
c8e7667 to
018aa77
Compare
17b0cac to
83d03a0
Compare
|
/ok to test b3f365f |
|
/ok to test 6908199 |
641a23d to
db997dd
Compare
|
/ok to test db997dd |
|
/ok to test da64e72 |
yuki-97
left a comment
There was a problem hiding this comment.
Merge blockers — please rebase on main and resolve conflicts.
The PR delivers block-FP8 / MXFP8 refit for Qwen3.5 on TRT-LLM, and this round closes most of the earlier threads cleanly: the Dynamo packing rule, the always-BF16 set, the docs example and the stale comments are all fixed.
Scope: fix commits 2071d49 through da64e72, checked against the open threads from the previous review, plus a scan of the changed files for new issues.
Checked:
- Dynamo keeps the old packing rule end to end:
use_legacy_packinggoes fromCollectiveSenderSpecthrough the synchronizer andlm_policyto both policy workers and the producer, and the legacy branch is byte-for-byte the old loop. - Deciding linear weights from the built engine model works: the new real-model test (
test_pattern_mode_quantizes_what_the_engine_holds_as_fp8) ran in the L0_Unit_Tests_Trtllm job at da64e72 and passed for dense block-FP8, dense MXFP8 and MoE block-FP8, with the router,shared_expert_gateandin_proj_*staying BF16. - The MoE MXFP8 case of that test is skipped on SM90 and only the GB200 nightly covers it.
Remaining comments are on the routed-expert decision still going through pattern matching (in the thread on trtllm_backend.py), and on the supported-model wording and check (on refit.md).
64bd0fe to
591f1e0
Compare
|
/ok to test 591f1e0 |
- Add the routed-expert block-FP8 / MXFP8 refit path for TRT-LLM (trtllm/quantization/fp8.py, is_mx, quantization_ignore_patterns, MoE backend selection, dummy-weight start, per-rank refit result checks, failed-FP8-refit poisoning). - Quiesce the executor loop while the refit group is built. - Re-prefill in-flight requests with recompute_active_requests() when recompute_kv is set. - Isolate oversized packed-tensor batches in packed_tensor.py. - Allow offload_policy_before_refit for non-colocated TRT-LLM generation. - Docs and unit tests. Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
…dd model-type check FP8 refit now quantizes routed experts only, which is what the MLPerf 397B and nightly recipes use. Drops the HF-name to engine-module mapping and the engine-side lookup it needed, and updates docs and tests to match. Adds validate_model_type so setup fails for models other than Qwen3.5 MoE and Qwen3 MoE, and aligns the refit guide's supported-model wording. Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
9ff9bf7 to
6d36a23
Compare
|
/ok to test 6d36a23 |
yuki-97
left a comment
There was a problem hiding this comment.
Scope: fix commits e3da39a through 6d36a23, checked against the open threads from the previous review, plus a scan of the changed files for new issues.
Checked:
- Removing the pattern mode closes the open backend thread and the ignore-pattern threads: refit quantizes only names that fully match the routed-expert regex, and the fixed engine-form exclude list matches no expert module or ancestor at the pinned TRT-LLM f94eeb3738, so both sides agree for Qwen3.5 and Qwen3 MoE.
validate_model_typeruns on the samemodel_nameTRT-LLM loads and resolves toqwen3_5_moefor the nightly's Qwen3.5-35B-A3B-Base.- Both refit paths abort and re-raise on failure, BF16 and FP8 alike, so the engine-side cause reaches the caller.
- The MPI launcher hook only changes the environment in venvs that contain
tensorrt_llmand keeps the root-allow variables the TRT-LLM worker sets.
Remaining comments are on the new job-wide Ray worker_process_setup_hook: it breaks runtime_env reuse in _init_ray on the cluster-tag-mismatch path, and the question of scoping it to the TRT-LLM generation path or dropping it is open to the author. The Qwen3 MoE "Verified" line on the open docs thread is also still pending, and the reply on the backend thread records that the pattern mode was dropped in favor of the fixed format.
nit, not blocking merging — a redundant local import importlib in virtual_cluster.py, and a leftover "By default" in refit.md.
6d36a23 to
e3da39a
Compare
… refit guide The routed-expert scope is fixed now that quantization_ignore_patterns is gone, so 'By default' implied a non-default scope that no longer exists. Qwen3.5 MoE is verified end to end by the MXFP8 nightly; Qwen3 MoE is only unit-tested, so say that instead of listing it as verified. Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
Signed-off-by: Erin Ho <14718778+hchings@users.noreply.github.com>
There was a problem hiding this comment.
for CI, will need to wait #4802 get into main first, otherwise CI will fail.
for #4122 (comment), seems all TRTLLM nightly will fail due to #4607, which is out of the scope of this PR.
@shuyixiong will fix it in another PR, thanks!
the newly added recipe in this PR validated pass locally w/ temporary fix for env vars, so kept it in this PR along.
|
/ok to test 9268b55 |
What does this PR do ?
Enable the TRT-LLM generation engine to run Qwen3.5 MoE with block-FP8 or MXFP8 routed experts, refit from the BF16 Megatron trainer at each step, plus the refit fixes and a GB200 nightly that go with it.
Issues
List issues that this PR closes (syntax):
Usage
# Add a code snippet demonstrating how to use thisBefore your PR is "Ready for review"
Pre checks:
Additional Information