FP8 RL Training and Rollout Weight Synchronization - #1898
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces blockwise FP8 serialization for rollout weight synchronization (specifically targeting Qwen3.5 layouts), adds chunking to vocab_parallel_entropy to bound temporary memory usage, and implements a colocated worker memory barrier with hard eviction to manage HBM footprint. It also updates sequence packing and alignment logic for FP8/TP/CP layouts and supports persistent FP8 parameter training. The review feedback highlights two critical issues: first, the bytes_per_token calculation for vocab entropy chunking does not account for batch dimensions, which could lead to OOMs; second, accessing megatron_config directly in prepare_runtime_environment will crash FSDP training runs due to an AttributeError, requiring defensive guards.
9113224 to
5c463c2
Compare
2b1a0a3 to
787a4c5
Compare
erictang000
left a comment
There was a problem hiding this comment.
Reviewed as a distributed-systems pass (correctness / concurrency / distributed-sync / numerical-stability / memory / perf). FP8 quantization numerics are sound (fuzz-verified), the lossy-master-seed window is correctly closed, and the BF16 path is preserved. Four issues below: one crash-on-valid-input (High) and three latent distributed hazards (Medium) that surface on specific hardware/config combos. Line-anchored comments follow.
Also worth confirming against the pinned Megatron/Bridge (couldn't verify here): (1) fp8_param.py:152 calls _copy_model_params_to_main_params(state_dict=...) but stock Megatron's method takes no args and copies from live (quantized) params — assumes a patched Bridge version; (2) the hybrid path re-seeds param_to_fp32_param/CPU copies but not optimizer.state[param]['master_param'], which megatron_strategy.py's own patch treats as authoritative; (3) the 3D fused-MoE weight_loader fast path in new_inference_worker_wrap.py assumes vLLM accepts a batched 3D tensor in one call.
|
@jinghanyao1-hub ai code review above, you can ignore for now, still taking a deeper pass, will resolve the above if i find it's not reasonable |
8460420 to
4a5cfa5
Compare
|
high level comment on file structure: can we reorganize to create a new folder and then can we split the |
|
another high level comment: can we add a gpu ci test in the style of Essentially this test: but with fp8 quantization on both vllm and megatron.This will help us maintain full fp8 as a tracked feature across transformer engine/megatron/vllm updates. We want to test vllm/megatron init, weight sync, and logprob diff. We can start with qwen3.5-0.8b and qwen3.5-35B-a3B tests with blockwise quantization only. |
erictang000
left a comment
There was a problem hiding this comment.
left some comments:
- let's design a more extensible/maintainable solution for model specific quantization mappings/logic and try to keep instances of using a model name to a single config/abstraction
- i'm a little confused by the fp8 param init logic but i might be missing something
- ideally we can find areas where we could upstream logic to megatron-bridge rather than maintaining it ourselves for things like quantizing weights to fp8 for transfer
- gpu ci tests for both regular full fp8 and fp8 params for hopper (+ both moe and dense tests for full fp8, so 3 total gpu ci tests)
otherwise the weight sync changes for cuda ipc/nccl look pretty clean, looking forward to seeing the PR with updates!
Review reorg for NovaSky-AI#1898: move _fp8_block_amax_epsilon_patch.py (renamed to amax_epsilon_patch.py), fp8_param.py, and fake_int4_qat.py into a new workers/megatron/quantization/ package and update import sites.
Review reorg for NovaSky-AI#1898: break weight_sync/serialized_fp8.py into weight_sync/fp8/quantize.py (blockwise casts and scale helpers), weight_sync/fp8/vllm_format.py (wire format, quantization config, tensor iterators), and weight_sync/fp8/models/qwen35.py (Qwen3.5 layer mapping), with the public API re-exported from weight_sync/fp8/__init__.py. _normalize_block_size becomes normalize_block_size now that it is shared across modules. No functional changes.
Answers PR NovaSky-AI#1898 review question: the env var is SkyRL's, the amax_epsilon field is TE's (no upstream knob); needed only for Float8BlockScaling with power-of-2 block scales (Blackwell emulation), where zero-token MoE experts produce amax=0 blocks whose scale_inv overflows the grad norm. FP32-scale Hopper and native MXFP8BlockScaling do not exhibit it (verified empirically).
…ric code Review item (PR NovaSky-AI#1898): the sender-side quantization policy, vLLM ignored-layer derivation, and batched-MoE wire mapping were Qwen3.5-specific free functions imported by generic code. ModelFp8Spec (weight_sync/fp8/ models/base.py) groups them behind one interface, resolved once per checkpoint via resolve_fp8_spec(hf_config): - matches / should_quantize / ignored_layers / moe_expert_spec follow the reviewer's sketch; MoeExpertSpec/MoeProjection additionally carry the vLLM fused-loader targets (vllm_param, shard_id) and the split dimension. - qwen35.py becomes a registered QWEN35_FP8_SPEC; unsupported layouts resolve to None and are rejected with the registered-spec list. - The vLLM worker extension derives _BATCHED_MOE_TARGETS from the registered specs (batched_moe_wire_targets) instead of re-encoding the fused-loader mapping - sender and receiver now share one source of truth. - SerializedFp8Config carries the resolved spec; MegatronWeightExtractor resolves it from hf_config at construction. - inference_servers/utils.py and the fp8 package API no longer name Qwen. User-configurable quantized-layer lists stay out of scope per review note.
Documents generator.inference_engine.fp8_weight_sync_mode and the Megatron-side FP8 surface it composes with (transformer_config_kwargs.fp8 / fp8_recipe incl. the "auto" per-architecture default / fp8_param, ddp_config.fp8_param_gather), plus the auto-managed block-scale env contract. Addresses PR NovaSky-AI#1898 review item on config documentation.
Review reorg for NovaSky-AI#1898: move _fp8_block_amax_epsilon_patch.py (renamed to amax_epsilon_patch.py), fp8_param.py, and fake_int4_qat.py into a new workers/megatron/quantization/ package and update import sites.
Review reorg for NovaSky-AI#1898: break weight_sync/serialized_fp8.py into weight_sync/fp8/quantize.py (blockwise casts and scale helpers), weight_sync/fp8/vllm_format.py (wire format, quantization config, tensor iterators), and weight_sync/fp8/models/qwen35.py (Qwen3.5 layer mapping), with the public API re-exported from weight_sync/fp8/__init__.py. _normalize_block_size becomes normalize_block_size now that it is shared across modules. No functional changes.
Answers PR NovaSky-AI#1898 review question: the env var is SkyRL's, the amax_epsilon field is TE's (no upstream knob); needed only for Float8BlockScaling with power-of-2 block scales (Blackwell emulation), where zero-token MoE experts produce amax=0 blocks whose scale_inv overflows the grad norm. FP32-scale Hopper and native MXFP8BlockScaling do not exhibit it (verified empirically).
…ric code Review item (PR NovaSky-AI#1898): the sender-side quantization policy, vLLM ignored-layer derivation, and batched-MoE wire mapping were Qwen3.5-specific free functions imported by generic code. ModelFp8Spec (weight_sync/fp8/ models/base.py) groups them behind one interface, resolved once per checkpoint via resolve_fp8_spec(hf_config): - matches / should_quantize / ignored_layers / moe_expert_spec follow the reviewer's sketch; MoeExpertSpec/MoeProjection additionally carry the vLLM fused-loader targets (vllm_param, shard_id) and the split dimension. - qwen35.py becomes a registered QWEN35_FP8_SPEC; unsupported layouts resolve to None and are rejected with the registered-spec list. - The vLLM worker extension derives _BATCHED_MOE_TARGETS from the registered specs (batched_moe_wire_targets) instead of re-encoding the fused-loader mapping - sender and receiver now share one source of truth. - SerializedFp8Config carries the resolved spec; MegatronWeightExtractor resolves it from hf_config at construction. - inference_servers/utils.py and the fp8 package API no longer name Qwen. User-configurable quantized-layer lists stay out of scope per review note.
Documents generator.inference_engine.fp8_weight_sync_mode and the Megatron-side FP8 surface it composes with (transformer_config_kwargs.fp8 / fp8_recipe incl. the "auto" per-architecture default / fp8_param, ddp_config.fp8_param_gather), plus the auto-managed block-scale env contract. Addresses PR NovaSky-AI#1898 review item on config documentation.
85d056e to
c60c8d1
Compare
|
was all passing but ran into =========================== short test summary info ============================
FAILED tests/backends/skyrl_train/gpu/gpu_ci/megatron/test_router_replay.py::test_forward_backward[tp2_pp2_ep2] - SystemExit: 1
==== 1 failed, 7 passed, 4 deselected, 2075 warnings in 4324.38s (1:12:04) =====timeout |
…rl-weight-sync
…rl-weight-sync
A `labeled` event starts a run of every workflow triggered on `labeled`, whatever the label was -- applying three labels to #1898 produced 15 runs, of which 3 did work. The other 12 skip, which is correct, but they published their skip under the same check name as the real run. GitHub keeps only the newest check of a given name, so whether a live suite showed as running came down to whether its own run happened to be created last. Two of the three running suites reported "skipped" on the PR. Name the job by whether it is going to run, so a skip lands on its own check context and can only ever supersede another skip. The running branch is the job id -- the existing context name -- so any branch protection pointing at it is unaffected.
…rl-weight-sync
|
h100 ci: https://github.com/NovaSky-AI/SkyRL/actions/runs/33453865623 |
…rl-weight-sync
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
Reviewed by Cursor Bugbot for commit 98f58a0. Configure here.
…LELANG on Blackwell Qwen3.5 loads through the VL bridge, which packs sequences in its own forward and is rejected with SkyRL sample packing; set language_model_only on the policy, ref and inference engine like the other Qwen3.5 examples. The Blackwell scripts also export FLA_TILELANG=0: fla's TileLang GDN packed backward aborts on B200. Document both in the README. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Eric Tang <etang@anyscale.com>



This PR adds end-to-end FP8 training and rollout support. It covers persistent Megatron FP8 parameters, serialized blockwise weight synchronization, mixed-dtype transport, and FP8 vLLM execution.
Changes
fp8_weight_sync_mode=serialized_blockwiseconverts eligible weights to E4M3 and sends matching FP32weight_scale_invtensors in vLLM checkpoint format.quantization=fp8,load_format=dummy, and a matching 128 x 128 block quantization configuration; the first full sync replaces the dummy weights.fp8_param_gather, which refreshes persistent FP8 parameters from updated FP32 masters.Scale Support
Without persistent
fp8_param, serialized FP8 synchronization supports both FP32 and power-of-two block scales. Hopper supports both modes and defaults to FP32 scales; the current Blackwell path requires power-of-two scales.Persistent
fp8_paramis supported only with FP32 scales. The main reason is that at a1e-6learning rate, 99.97-99.999% of sampled FP32 master values changed per update, no power-of-two block scale changed. The configuration therefore rejects power-of-two scales withfp8_param. Because Blackwell currently requires power-of-two scales, persistent FP8 parameters are not enabled there.Configuration
<role>denotespolicyorref. All FP8 features are disabled by default, so existing BF16 configurations retain their current behavior.generator.inference_engine.fp8_weight_sync_modenullserialized_blockwisegenerator.inference_engine.engine_init_kwargs.quantizationfp8trainer.<role>.megatron_config.transformer_config_kwargs.fp8e4m3trainer.<role>.megatron_config.transformer_config_kwargs.fp8_recipeblockwisetrainer.<role>.megatron_config.transformer_config_kwargs.fp8_paramfalse)truetrainer.policy.megatron_config.ddp_config.fp8_param_gatherfalsetruewith policyfp8_paramNVTE_FP8_BLOCK_SCALING_FP32_SCALES1when FP8 is active11uses FP32 block scales;0uses power-of-two scales. Persistentfp8_paramrequires1.NVTE_FP8_BLOCK_AMAX_EPSILON01e-4VLLM_USE_DEEP_GEMM_E8M00with serialized FP32 scales0serialized_blockwiserequires Megatron and full-weight synchronization; adapter-only Megatron LoRA is rejected. On Blackwell, use power-of-two scales and keepfp8_paramdisabled.Performance
vLLM Rollout Throughput
On
Qwen/Qwen3.5-9B-Base, NVIDIA H100 80GB, TP1, vLLM 0.23.0, using DAPO recipe dataset.This gain comes from FP8 vLLM execution and does not require persistent FP8 Megatron parameters.
End-to-End RL Step Timing
The end-to-end comparison uses
Qwen/Qwen3.5-9B-Baseon eight NVIDIA H100 80GB GPUs with Megatron TP2, eight TP1 vLLM engines, and a training batch size of 32.Persistent FP8 Policy HBM
Quality Checks
The following WANDB runs log the metrics. All FP8 runs aligns well with the BF16 baseline.
Qwen3.5-4B runs: https://wandb.ai/sky-posttraining-uc-berkeley/qwen35_4b_dapo_mainclean_fp8align_fastbatch_bf16_fp8_20260630?nw=nwuserjinghanyao1
Qwen3.5-9B runs: https://wandb.ai/sky-posttraining-uc-berkeley/qwen35_9b_h100_tp2_bf16_fp8_prready_20260709?nw=nwuserjinghanyao1
Qwen3.5-35B-A3B runs: https://wandb.ai/sky-posttraining-uc-berkeley/qwen35_35b_a3b_fp8_amaxeps_20260705?nw=nwuserjinghanyao1
Reproduce key configs
H100:
B200 (only show the difference):
Note
High Risk
Changes weight sync, vLLM loading, FP8 packing alignment, and optimizer initialization on the critical train↔rollout path; misconfiguration is partly guarded by validation but numerical or MoE-loader mismatches could still break rollouts silently without tests on your exact stack.
Overview
Adds end-to-end FP8 for Megatron RL: TE FP8 GEMMs, optional persistent
fp8_param, andfp8_weight_sync_mode=blockwiseso rollouts receive trainer FP8 payloads and block scales instead of re-quantizing BF16.Serialized FP8 weight sync introduces a
weight_sync/fp8stack (blockwise quantize, vLLM checkpoint wire format, per-model specs starting with Qwen3.5). The Megatron extractor expands weights into mixed fp8/fp32/bf16 chunks withderives_metadata_from_chunks; NCCL sends one update per chunk, CUDA IPC splits packs by dtype, and delta / sharded_rdt are rejected at validation and send time. vLLM startup is auto-configured (quantization=fp8,load_format=dummy, quantization_config + ignored layers);NewInferenceWorkerWraploads batched MoE FP8 tensors via FusedMoE.Training correctness:
fp8_recipe(blockwise vs mxfp8) drives TP/CP/FP8 sequence alignment in packing, collators, and workers;fp8_recipe=autoresolves on workers (Hopper blockwise, Blackwell mxfp8) with validation for mxfp8 + nofp8_param. FP8 param uses deferred bridge load, exact FP32 masters from unquantized shards, and requiresfp8_param_gather. Driver validation and Ray runtime env pinNVTE_FP8_BLOCK_SCALING_FP32_SCALES/VLLM_USE_DEEP_GEMM_E8M0to match the scale contract.Docs and
examples/train/fp8/scripts cover Hopper blockwise (with optional fp8_param) and Blackwell MXFP8 DAPO runs for Qwen3.5.Reviewed by Cursor Bugbot for commit c127af6. Bugbot is set up for automated code reviews on this repo. Configure here.