fix: forward calibrated KV cache scales on colocated ZMQ IPC refit path#3226
Conversation
terrykong
left a comment
There was a problem hiding this comment.
@sharonyu-115 to review
Head branch was pushed to by a user without write access
5400cdb to
a6ed634
Compare
|
Thanks a lot @babyplutokurt. Fix works — median max_seq_mult_prob_error drops 20× (132 → 6.5), median policy_kl_error drops and median policy_kl_error drops 0.27 → 0.05 (5×). |
|
LGTM. Just 2 nits:
nemo_rl/models/policy/interfaces.py:189 currently uses *args, **kwargs, which is why the type checker couldn't warn the two IPC call sites that dropped kv_scales. All four concrete implementations (lm_policy, MegatronPolicyWorker, DTensorPolicyWorker, DTensorPolicyWorkerV2) already have the concrete typed signature, and both callers (grpo.py::refit_policy_generation, IPCWeightSynchronizer.sync_weights) pass compatible kwargs — so tightening the abstract is a no-op at runtime and safe against every existing call:
HTTPWeightSynchronizer is wired exclusively for SGLang (nemo_rl/weight_sync/factory.py:91), and SGLangGeneration never overrides the default requires_kv_scale_sync = False, so refit_policy_generation never sends non-None kv_scales down this path today. Not a live bug. But HTTPWeightSynchronizer.sync_weights still accepts and silently drops the kv_scales kwarg (http_weight_synchronizer.py:67) — exactly the pattern this PR is fixing on the IPC path. A one-line assert keeps it interface-uniform without adding real behavior, and makes any future misconfig loud: |
|
/ok to test a6ed634 |
|
By the way, could you also rebase onto main branch? CICD NeMo RL / Check if PR branch is up to date (push)Failing after 3s |
a6ed634 to
f9bd628
Compare
|
@ZhiyuLi-Nvidia Cool, added your suggested changes and rebased. |
|
/ok to test f9bd628 |
f9bd628 to
5ee617c
Compare
|
/ok to test 5ee617c |
|
@ZhiyuLi-Nvidia Weird, one of the presubmit failed, but I don't think it's related to my code changes.
And the job dies at mid-run, can you take a look? |
|
Sure. Let me rerun might be transition error. |
|
From logs, looks like a huggingface flaky outage issue |
Retrying in 2s [Retry 2/5]. ... Retrying in 8s [Retry 5/5].
...
File "/opt/nemo-rl/nemo_rl/data/datasets/response_datasets/avqa.py", line 97, in __init__
self.dataset = load_dataset("gijs/avqa-processed", split=split)
FileNotFoundError: Couldn't find any data file at /opt/nemo-rl/gijs/avqa-processed.
Couldn't find 'gijs/avqa-processed' on the Hugging Face Hub either:
LocalEntryNotFoundError: An error happened while trying to locate the file on the Hub |
The trainer-calibrated per-tensor FP8 KV cache scales (calibrate_qkv_fp8_scales) were only forwarded to the generation worker on the non-colocated NCCL path (broadcast_weights_for_collective). The colocated ZMQ-IPC refit path called stream_weights_via_ipc_zmq without kv_scales, so _iter_params_with_optional_kv_scales received None and defaulted every per-tensor K/V scale to 1.0. As a result, colocated FP8 KV cache runs (the common co-located GRPO/PPO setup) silently used scale 1.0 instead of the calibrated values: the calibration ran every step but its output was discarded at the refit call site. FP8 activations outside the e4m3 range then clamp/round to 1.0-scaled buckets, degrading KV-cache fidelity with no error. Forward kv_scales on both colocated IPC call sites (refit_policy_generation and IPCWeightSynchronizer.sync_weights) so the calibrated scales reach the worker, matching the collective path. Signed-off-by: babyplutokurt <attaboykurt.yang@gmail.com>
5ee617c to
36aef4b
Compare
1.
|
terrykong
left a comment
There was a problem hiding this comment.
lgtm. thanks @babyplutokurt for the fix
|
/ok to test 36aef4b |
Fixes #3225
What
Forward the trainer-calibrated per-tensor FP8 KV cache scales (
kv_scales)on the colocated ZMQ-IPC refit path, so they actually reach the generation
worker.
Why
calibrate_qkv_fp8_scalescomputes per-tensor K/V scales every step, but theywere only forwarded on the non-colocated NCCL path
(
broadcast_weights_for_collective(kv_scales=...)). The colocated ZMQ-IPC pathcalled
stream_weights_via_ipc_zmq(buffer_size_bytes=...)withoutkv_scales, so_iter_params_with_optional_kv_scalesreceivedNoneanddefaulted every per-tensor K/V scale to 1.0
(
megatron_policy_worker.py,scale_value = 1.0).Net effect: in the common colocated GRPO/PPO setup, FP8 KV cache runs
silently used scale 1.0 instead of the calibrated values. The calibration ran
each step but its output was discarded at the refit call site — no error, just
uncalibrated KV cache.
Change
Two one-line additions, passing
kv_scaleson both colocated IPC call sites sothey match the collective path:
nemo_rl/algorithms/grpo.py—refit_policy_generation, the ZMQ IPC branch.nemo_rl/weight_sync/ipc_weight_synchronizer.py—IPCWeightSynchronizer.sync_weights(the method already accepted
kv_scalesbut dropped it at the call).No new machinery; precision-independent; the receiving
stream_weights_via_ipc_zmq→_iter_params_with_optional_kv_scaleschainalready threads
kv_scalesend-to-end.Verification
On a colocated run, the streamed
kv_scalesnow populate the worker'sper-tensor scales each step (calibrated, non-1.0, drifting with activations)
instead of defaulting to 1.0.