Repository navigation
Conversation
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
|
Self-review completed with independent correctness, test, and upstream-compatibility passes. Resolved findings:
Validation:
Remaining dependency: the real TRTLLM GPU integration requires vLLM PR #52204 in NeMo-RL's dependency pin. This PR remains draft until that test can run. |
|
/ok to test eb05783 |
MXFP8 dense backend: generation performance comparisonTested PR3638's native-linear implementation integrated onto main Generation performanceMoE backend: flashinfer_trtllm. Dense linear: flashinfer_cutlass → flashinfer_cutedsl. MXFP8 covers eligible MoE and dense linear layers; explicit ignore list is 20 actual GRPO steps per arm, statistics over steps 2–20. Each pair uses the same nodes, model, seed, dataset and configuration, with only the selected dense backend changed. These are single-run comparisons. Lower generation time and higher throughput are better.
Definition: Generation time is rollout batch completion time, including reward/postprocessing, not a pure decode-kernel timer. Sync excludes preparation/refit and includes final sleep/offload. Async includes in-flight pauses inside the batch and excludes prelaunch refit waiting. Throughput is actual accepted output tokens divided by summed batch durations; overlapping Async batches mean it is not aggregate engine wall throughput. Responses use natural EOS. Qwen235B memory-pressure qualification (*): CUTLASS logged repeated non-fatal CUDA allocator OOM warnings during initial refit and measured steps; CuTeDSL had no OOM messages. Both completed 20 steps with finite losses/gradient norms, zero reported failed generations and scheduler exit 0. The warnings' timing impact has not been isolated, so the +11.2% is the observed difference under this memory configuration. Super Async overlap qualification (†): CuTeDSL has lower batch-normalized generation throughput in this pair, but this is not a 15.9% kernel or aggregate-engine regression. The batch-duration mix changed in this workload with in-flight refit: 9/19 CUTLASS batches versus 2/19 CuTeDSL batches completed under 70 s. Median batch duration was 97.285 → 94.716 s while the mean was 79.271 → 90.060 s. Aggregate engine wall throughput was 10,488.1 → 10,355.2 output tok/s (-1.27%), and mean output length was 3217.3 → 3073.2 tokens. The duration pattern is consistent with different overlap with refit pauses, but individual pause attribution is not proven. The effects of overlap timing and output-length variation have not been isolated. Both runs completed 20 finite steps with zero generation failures and no OOM messages. Large-workload status (updated October 9)
Workload, output lengths and quantization scope
The explicit exclusion is This was checked against the running nightly image: SHA256 of qwen3_moe.py, Earlier Super GBS 32 compatibility workload: generation resultsThese reduced workloads use 8 prompts × 4 responses, maximum input/output 512/512 and total sequence 1024. They have different GPU allocations from the performance workloads and must not be used to rank backends for large batches. Each pair uses the same nodes; compare backends only within a model/mode. Super Sync's CUTLASS training arm completed 20 steps before an allocation cleanup failure; its CuTeDSL continuation used the exact same nodes.
Reference: training E2E, refit and aggregate engine throughputGeneration is the main comparison above. These previously requested measurements remain supplemental. Backend weight layouts/refit and memory use can also influence the full training cycle. Training E2E uses the wall interval between logging after steps 1 and 20, including inter-step logging. Engine counters use that same window, including idle/refit and unused rollouts.
|
Follow-up: generation impact of direct CuTeDSL scale refitThis is an additional experimental optimization, separate from the CUTLASS-versus-CuTeDSL comparison above. Both arms use Measured source: experimental commit Generation performanceQwen/Qwen3-30B-A3B, performance recipe, GBS 2048, 16 GB200 GPUs, official October 7 nightly with vLLM 0.29. Each mode ran baseline then optimized in one allocation on identical nodes, 20 actual GRPO steps per arm; statistics use steps 2–20. MXFP8 covers eligible MoE and dense linear layers with
Interpretation: observed generation differences are small. One ordered pair does not establish a repeatable generation speedup; the complete refit timing also shows no consistent improvement. The earlier 42–48% reduction was layer postprocessing microbenchmark time, approximately 55–70 µs saved per invocation, not generation or complete refit time. The experimental flag remains disabled by default. Generation time is rollout batch completion time, including reward/postprocessing. Sync excludes preparation/refit; Async uses the native full-batch duration and can include in-flight refit pauses, but excludes prelaunch waiting. Throughput is actual accepted output tokens divided by summed batch durations. With overlapping Async batches this is a batch-normalized rate, not aggregate engine wall throughput. Natural EOS is enabled. All four arms completed 20 steps and exited successfully, with finite loss/gradient metrics and zero reported generation failures. Worker logs confirm the optimized path was active only in the optimized arms. GPU validation also passed 17 new tests, 260 existing tests and four repeated-refit CUDA Graph lifecycle probes. Supplemental refit and training-wall measurements
The Sync baseline's step-2 refit was 13.198 s. This accounts for its apparent mean-refit difference: diagnostic steps 3–20 averages are 9.141797 s versus 9.141367 s, effectively equal. The primary generation table retains the predefined steps 2–20 window. Async mean refit was slightly higher with the optimization. Training wall time uses the logging-boundary interval after steps 1 and 20, including logging overhead.
|
|
A few follow-up suggestions:
I would prioritize the config-context unit test and real-kernel GPU integration validation. These are coverage recommendations, not confirmed correctness bugs. |
Signed-off-by: seonjinn <sna@nvidia.com>
|
Thanks for the follow-up. In 8a39ac8 I renamed the helper to |
Signed-off-by: seonjinn <sna@nvidia.com>
Resolve conflicts with NVIDIA-NeMo#3294 (MXFP8 refit transfer overheads): - vllm_backend: drop the removed _maybe_process_fp8_kv_cache second pass; finalize() already runs process_weights_after_loading. - fp8: keep the native MXFP8 linear scale rename before main's grouped expert scale reroute. - tests: keep both the PR's native MXFP8 tests and main's refit-info tests. Signed-off-by: seonjinn <sna@nvidia.com>
|
/ok to test 3e79c96 |
seonjinn
left a comment
There was a problem hiding this comment.
Self-review after merging latest main (head 3e79c96723). Seven threads: two medium correctness items (unreachable or inconsistent pending_roots branch, DSV4 widened to nccl_reshard), one import-scope item, one latent backend-demotion item, test gaps, and nits.
Validation: static review plus local compileall and ruff. Tests were not executed locally (no GPU, Linux-only lockfile).
Signed-off-by: seonjinn <sna@nvidia.com>
Signed-off-by: seonjinn <sna@nvidia.com>
Merged main tests build VllmInternalWorkerExtension doubles without model_runner.model, stub vllm without envs, and use fp8 config stubs without is_mx. The MXFP8 reload root scan now returns no roots for model-less doubles without caching, and imports the fp8 module only when a module carries a linear kernel. Signed-off-by: seonjinn <sna@nvidia.com>
|
/ok to test cf947cc |
Signed-off-by: seonjinn <sna@nvidia.com>
|
/ok to test 893a738 |
Signed-off-by: seonjinn <sna@nvidia.com>
|
/ok to test a7eeef4 |
Signed-off-by: seonjinn <sna@nvidia.com>
…end-refit-review-fixes Signed-off-by: seonjinn <sna@nvidia.com>
|
/ok to test 2870907 |
Signed-off-by: Seonjin Na <sna@nvidia.com>
|
/ok to test 83c655c |
Signed-off-by: seonjinn <sna@nvidia.com>
|
/ok to test |
terrykong
left a comment
There was a problem hiding this comment.
Thanks for this — opt-in native CuTeDSL/TRTLLM dense kernels with refit that is safe under CUDA graphs, plus a careful self-review round. Every self-review item is fixed at head (checked; no replies needed).
Done well, worth keeping:
- Refit goes through vLLM's public layerwise reload, so changed weights are copied into the storage CUDA graphs captured. vLLM private code is touched only on the abort path, and that path is tested against real vLLM.
- BF16 TRTLLM experts and native MXFP8 linears reload in one pass, so one can't undo the other's layout.
- Kernel detection uses
isinstanceon the real vLLM classes, and the name test fails (not skips) on a rename. - A startup reject, plus a runtime check on the kernel vLLM actually built.
Must fix before merge:
- (C1) The new per-refit cache reset clears only the GPU worker's image cache, not the scheduler's. Non-colocated and in-flight-update VLM runs can crash with
Encoder cache miss. Remove the helper.
Smaller items:
2. (C2) Run one native GB200 leg with REFIT_PREQUANTIZE=false.
3. (C3) Read linear_backend the same way vLLM does.
4. (C4) Keep the backend → kernel pairing in one place.
5. (C5) Document the new option in docs/fp8.md.
6. (C6, C7, C8) Comment and docstring fixes.
GPU CI has not run on this head yet (waiting for approval); the GB200 MXFP8 shard passed on 2870907.
Visual explainer: https://terrykong.github.io/gh-pages-poc/terryk/pr-3638-native-refit.html
Visual explainer: https://terrykong.github.io/gh-pages-poc/terryk/pr-3638-encoder-cache-reset.html
Generated by Claude Code
| def _reset_model_runner_caches_after_refit(self) -> None: | ||
| """Mirror the cache invalidation performed by vLLM reload_weights.""" | ||
| for method_name in ( | ||
| "reset_lora_state", | ||
| "reset_encoder_cache", | ||
| "reset_mm_cache", | ||
| ): | ||
| reset = getattr(self.model_runner, method_name, None) | ||
| if reset is None: | ||
| continue | ||
| if not callable(reset): | ||
| raise RuntimeError( | ||
| f"vLLM model runner {method_name} exists but is not callable" | ||
| ) | ||
| reset() |
There was a problem hiding this comment.
C1 · 1 action item — correctness bug, please fix in this PR.
TL;DR — after every refit, this helper clears the image/video encoder cache on the GPU worker but not in vLLM's scheduler. The scheduler still thinks the media is cached, skips the encoder, and the worker crashes with Encoder cache miss.
PR-introduced: on main this only ran behind refit_with_reload_api=True; this PR runs it on every refit, for every model.
- Both finalizers now call this helper: the native one (L1804) and the default one used by every ipc / collective / checkpoint-engine / nccl_reshard refit (L1850).
GPUModelRunner.reset_encoder_cacheonly runsself.encoder_cache.clear()on the worker.- The scheduler's
EncoderCacheManager.check_and_update_cachestill has the hash incached(finished requests leave it there as "freeable"), so it returns True and the encoder is not scheduled. - The worker then raises
RuntimeError(f"Encoder cache miss for {mm_hash}."). OnlyEngineCore.reset_encoder_cacheclears both sides.
Who hits it: VLM runs that refit without first pausing the engine with clear_cache=True — non-colocated runs, and async in-flight updates with recompute_kv_cache_after_weight_updates: false. The shipped Nemotron-Omni async video recipe is exactly that (in-flight updates, no KV recompute; colocated off). A later turn of a multi-turn Gym episode, a sibling rollout, or a preempted request sends the same media again after the refit and hits the stale entry. Colocated runs are safe: sleep clears both sides first.
AI-1
Delete _reset_model_runner_caches_after_refit and its two calls (L1804, L1850). In test_mxfp8_native_linear_refit_uses_vllm_layerwise_reload, drop the last two expected entries "reset_encoder_cache", "reset_mm_cache" but keep the two recording lambdas, so the test fails if the worker-only reset comes back. Not a suggestion block: three locations in two files.
Why not keep part of the helper, or swap in the engine-level reset
reset_lora_statereturns at once without alora_config, so it does nothing on every path that reaches this helper: only native LoRA setsenable_lora, and native LoRA skips_weight_update_lifecycle.reset_mm_cacheonly clears a worker-local profiling cache.- Clearing stale image embeddings after refit already has a safe home: the opt-in
vllm_cfg.reset_encoder_cache_after_weight_updatecalls the engine-level reset that clears both sides. The exemplar says it is "Only safe when generation is quiesced". - Calling the engine-level reset from this helper is not the fix either: the worker extension cannot reach the scheduler, and with in-flight updates even that call puts the two caches out of step.
Upstream vLLM has the same worker-only clear
vLLM's own GPUModelRunner.reload_weights does the same worker-only clear. That is why refit_with_reload_api=True already had this bug on main.
Test coverage
Not verified on GPU — no GPU environment. No GB200 leg covers it: all legs are text-only Qwen3-0.6B.
| run_test env LINEAR_BACKEND=flashinfer_trtllm uv run --no-sync bash \ | ||
| ./tests/functional/grpo_vllm_mxfp8_rollout_gb200.sh |
There was a problem hiding this comment.
C2 · 1 action item — test gap.
TL;DR — both native GB200 legs run with REFIT_PREQUANTIZE=true, so the new native branch on the vLLM-side quantize path (fp8.py#L751-L758) never runs on real kernels.
The script defaults REFIT_PREQUANTIZE=true. With it on, refit takes the _scale_from_checkpoint rename branch. The prequant=false branch that picks _scale for native modules is covered only by a mocked unit test (test_load_weights_uses_scale_name_for_mxfp8_linear_backend).
AI-1
Run the TRTLLM leg with prequant off. This swaps an existing leg, so it adds no runtime, and EXP_NAME already includes both knobs.
| run_test env LINEAR_BACKEND=flashinfer_trtllm uv run --no-sync bash \ | |
| ./tests/functional/grpo_vllm_mxfp8_rollout_gb200.sh | |
| run_test env REFIT_PREQUANTIZE=false LINEAR_BACKEND=flashinfer_trtllm uv run --no-sync bash \ | |
| ./tests/functional/grpo_vllm_mxfp8_rollout_gb200.sh |
| def _requested_linear_backend(config: VllmConfig) -> str | None: | ||
| vllm_kwargs = config.get("vllm_kwargs") or {} | ||
| kernel_config = vllm_kwargs.get("kernel_config") or {} | ||
| if isinstance(kernel_config, Mapping) and kernel_config.get("linear_backend"): | ||
| return kernel_config["linear_backend"] | ||
| return vllm_kwargs.get("linear_backend") |
There was a problem hiding this comment.
C3 · 1 action item — config check can miss or wrongly reject a backend.
TL;DR — _requested_linear_backend reads the setting differently from vLLM, so the startup reject misses flashinfer-cutedsl and can reject a config vLLM would run as CUTLASS.
- vLLM lowercases the name and turns
-into_.flashinfer-cutedslpasses this check, vLLM builds the native kernel, and the run only fails at the runtime reject, after model load. - vLLM lets a top-level
linear_backendthat is not"auto"overridekernel_config.linear_backend. Herekernel_configwins, so top-levelflashinfer_cutlass+kernel_config.linear_backend: flashinfer_cutedslruns CUTLASS in vLLM but is rejected here.
AI-1
| def _requested_linear_backend(config: VllmConfig) -> str | None: | |
| vllm_kwargs = config.get("vllm_kwargs") or {} | |
| kernel_config = vllm_kwargs.get("kernel_config") or {} | |
| if isinstance(kernel_config, Mapping) and kernel_config.get("linear_backend"): | |
| return kernel_config["linear_backend"] | |
| return vllm_kwargs.get("linear_backend") | |
| def _requested_linear_backend(config: VllmConfig) -> str | None: | |
| # Match vLLM: a top-level non-"auto" value overrides kernel_config, and | |
| # names are case-insensitive with "-" and "_" interchangeable. | |
| vllm_kwargs = config.get("vllm_kwargs") or {} | |
| backend = vllm_kwargs.get("linear_backend") | |
| if not backend or backend == "auto": | |
| kernel_config = vllm_kwargs.get("kernel_config") or {} | |
| if isinstance(kernel_config, Mapping): | |
| backend = kernel_config.get("linear_backend") or backend | |
| if isinstance(backend, str): | |
| backend = backend.lower().replace("-", "_") | |
| return backend |
Also add the dash spelling and the both-keys-set case to test_native_mxfp8_linear_backend_rejected_on_non_native_refit_transports.
| ) | ||
|
|
||
|
|
||
| _NATIVE_MXFP8_LINEAR_BACKENDS = ("flashinfer_cutedsl", "flashinfer_trtllm") |
There was a problem hiding this comment.
C4 · 1 action item — design, low.
TL;DR — the backend → kernel pairing is written twice, backend names here and kernel class names in _NATIVE_MXFP8_LINEAR_REFIT_KERNELS, with nothing tying them together.
If a third native kernel is added to one list and not the other, the startup reject passes a config that only the runtime check on the GPU worker catches. (The class names themselves are already guarded by test_mxfp8_linear_kernel_names_resolve_on_installed_vllm; this is only about the pairing.)
AI-1
Keep one dict in config.py (it already has no vLLM import) and build the fp8.py set from it. Three edits, two files, so not a suggestion block:
- Replace
_NATIVE_MXFP8_LINEAR_BACKENDSon this line with:
# Native MXFP8 linear backends and the vLLM kernel each one builds
# (vllm/model_executor/kernels/linear/__init__.py). fp8.py derives its set from this.
NATIVE_MXFP8_LINEAR_KERNELS = {
"flashinfer_cutedsl": "FlashInferCutedslMxfp8LinearKernel",
"flashinfer_trtllm": "FlashInferTrtllmMxfp8LinearKernel",
}- In
_reject_native_mxfp8_linear_refit_transport, checklinear_backend not in NATIVE_MXFP8_LINEAR_KERNELS. - In fp8.py, replace the kernel-name set with
set(NATIVE_MXFP8_LINEAR_KERNELS.values()), imported from config.py.
| return vllm_kwargs.get("linear_backend") | ||
|
|
||
|
|
||
| def _reject_native_mxfp8_linear_refit_transport( |
There was a problem hiding this comment.
C5 · 1 action item — missing docs.
TL;DR — the new vllm_kwargs.linear_backend: flashinfer_cutedsl | flashinfer_trtllm option and its limits are only in the PR description; nothing under docs/ mentions linear_backend.
AI-1
In docs/fp8.md, add a short subsection after "Optional trainer-side MXFP8 refit" with the PR's usage YAML and its limits:
refit_transportmust be null (IPC/collective); nccl_reshard, sparse, NIXL and checkpoint-engine are rejected.- No FP8 KV cache.
- No co-trained MTP drafter.
- The default stays CUTLASS.
Not a suggestion block: the file is outside this PR's diff.
| # Under linear_backend=auto only CuTe-DSL is demoted to CUTLASS. A TRTLLM | ||
| # kernel that vLLM selects keeps the native refit path below. |
There was a problem hiding this comment.
C6 · 1 action item — nit, code comment.
TL;DR — in vLLM 0.29, auto never selects TRTLLM, so this comment describes a case that cannot happen.
On CUDA, vLLM tries Cutedsl, Cutlass, Marlin, B12x, Emulation, Humming, then Trtllm, and Emulation is always supported, so the list never reaches Trtllm.
AI-1
| # Under linear_backend=auto only CuTe-DSL is demoted to CUTLASS. A TRTLLM | |
| # kernel that vLLM selects keeps the native refit path below. | |
| # Under linear_backend=auto only CuTe-DSL is demoted to CUTLASS. An | |
| # explicitly selected TRTLLM kernel keeps the native refit path below. |
| return isinstance(kernel_type, type) and isinstance(kernel, kernel_type) | ||
|
|
||
|
|
||
| def uses_native_mxfp8_linear_refit(module: torch.nn.Module) -> bool: |
There was a problem hiding this comment.
C7 · 1 action item — nit, missing docstring.
TL;DR — uses_native_mxfp8_linear_refit is public and called from _get_mxfp8_linear_reload_roots, but has no docstring.
AI-1
| def uses_native_mxfp8_linear_refit(module: torch.nn.Module) -> bool: | |
| def uses_native_mxfp8_linear_refit(module: torch.nn.Module) -> bool: | |
| """Return True if the layer uses vLLM's FlashInfer CuTeDSL or TRTLLM MXFP8 linear kernel. | |
| These kernels replace the weight and scale with new tensors in their own | |
| layout after loading, so refit must go through vLLM's layerwise reload, | |
| which copies the result back into the storage CUDA graphs captured. | |
| """ |
What does this PR do?
Allows NeMo-RL MXFP8 rollout refit to preserve an explicitly selected vLLM CuTeDSL or TRTLLM dense linear backend.
These native kernels replace checkpoint-layout parameters with backend-specific runtime layouts. Re-running their post-load hook directly during refit would replace CUDA-Graph-visible storage. This change uses vLLM's layerwise reload lifecycle for native MXFP8 linear layers, so each transformed weight is copied into the original runtime storage. Failed or incomplete reloads restore saved runtime parameters without processing partial checkpoint tensors.
The current
autobehavior remains unchanged: NeMo-RL keeps FlashInfer CUTLASS as its default refit backend. Non-native refit paths now runprocess_weights_after_loadingunderset_current_vllm_configand reset the LoRA, encoder and multimodal caches after each refit. The reset calls depend on vLLM model runner method names.NeMo-RL's vLLM 0.29 pin includes vLLM PR #52204, which provides the TRTLLM MXFP8 linear backend used by this PR.
Update (review fixes, cf947cc)
normalize_vllm_refit_configrejectsprecision: fp8+is_mx+ a nativelinear_backend(flashinfer_cutedsl,flashinfer_trtllm) onnccl_reshardand the sparse and NIXL transports.pending_rootsbranch is removed.nccl_reshardagain, as on main (feat: add DeepSeek V4 Flash GRPO support - Automodel path #3595).process_weights_after_loading_mxfp8_linearraises when no vLLM config context is set. It no longer demotes an explicit CuTe-DSL backend to CUTLASS.nccl_reshardcase restored, random-byte layout oracle with pad-row checks, fused q/k/v shards in separate batches, real-vLLM kernel name resolution, no-context raise, DSV4nccl_reshardreject.Usage
Before your PR is "Ready for review"
50 passed.0 errors.flashinfer_cutedslandflashinfer_trtllmlegs to the L1 GB200 MXFP8 shard (grpo_vllm_mxfp8_rollout_gb200.sh, Qwen3-0.6B, 2 GPUs,precision: fp8,is_mx: true,vllm_kwargs.linear_backend). Each native leg checks that the native kernel owns the refit (NRL_MXFP8_NATIVE_LINEAR_REFIT kernels=<kernel> modules=N). a7eeef4 runs every leg of the script, the defaultrefit_prequantizelegs included, for 3 GRPO steps with CUDA graphs on (enforce_eager=false). All 4 legs passed on OCI GB200 (modules=56 for both native kernels).Additional Information
The repeated-refit tests cover CuTeDSL-style transpose and TRTLLM-style padding/layout changes while asserting stable
Parameteridentity anddata_ptr()across two refits. Failure-path tests cover partial transfer abort, initialization failure, and one-shot finalization cleanup.