Fix rotary frequency precision - #462
Merged
Merged
Conversation
le1nux
marked this pull request as ready for review
September 7, 2026 12:04
BlueCrescent
approved these changes
Sep 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
RotaryTransform._update_cos_sin_tablescast the inverse frequencies to the activation dtypebefore scaling them by token positions (
self.inv_freq.to(x.dtype)), so under BF16 training therotary phase
t * inv_freqwas accumulated from BF16 frequencies. Because the phase grows with theabsolute token position, a 2^-9 relative error on a frequency becomes an O(1) error in radians at
long context.
FP32, and
cos/sinare still cast back to the activation dtype, exactly asLlamaRotaryEmbeddinginsrc/modalities/conversion/gpt2/modeling_gpt2.pyalready did — thatpath forced FP32 all along, so the codebase disagreed with itself
FP32, but it makes the reference independent of the input dtype)
Impact
Max phase error of the
cos/sintables against an FP64 reference (n_embd=4096,n_head=32,base_freq=10000, BF16 activations):At 4k tokens and beyond the positional phase was effectively meaningless. Every shipped FSDP2
config reaches this code via
qkv_transforms: RotaryTransform.The remaining 2.8e-03 rad is the irreducible cost of storing
cos/sinin BF16, and it is thesame residual the reference implementations carry: after this change the tables are bit-identical
to
LlamaRotaryEmbeddingwith FP32 inverse frequencies (max |diff| = 0.0e+00 for bothcosandsinat seq 4096). The training path now computes standard RoPE, so HF export and vLLM inferencecompute the same positions as training.
Effect on existing FSDP2 checkpoints
The fix applies in full when warm-starting an older checkpoint. FSDP2's
MixedPrecisionPolicyhasno
buffer_dtypeand does not cast buffers, soinv_freqstayed FP32 throughout every FSDP2 runand the stored value was never damaged — the bug was only in the einsum. Checked against a real
pre-fix DCP checkpoint (32
inv_freqkeys, alltorch.float32): the loaded values match freshlycomputed FP32 frequencies to 3.0e-08, i.e. ~1 FP32 ulp, and are not BF16-rounded (which would
have cost 1.8e-03). Warm-starting that checkpoint gives 3.1e-03 rad at seq 8192, against π before.
Loading is in-place into the model's own FP32 buffer via
StateDictOptions(strict=False)(
app_state.py:206), so a checkpoint withoutinv_freqkeys is equally fine — the buffer keepsits freshly computed FP32 value.
Worth being deliberate about: this changes the positional encoding a resumed run sees. A run that
trained under the old, self-consistent-but-nonstandard phase and then resumes with standard RoPE is
undergoing a mid-training change to the position function, so expect a loss bump on resume. An
anneal or an export is the natural moment to take the correction.
Known limitation: FSDP1
.float()can only preserve precision while the buffer is still FP32. Two paths castinv_freqto BF16 before it reaches this line, and upcasting cannot recover the discarded mantissa bits, so
the change is a no-op there:
mixed_precision_settings: BF_16—bfSixteensetsbuffer_dtype=torch.bfloat16(
env_utils.py:43-49) and FSDP1 casts buffers on every root pre-forward. Measured: still3.14e+00 rad at seq 4096. Reaches 5 shipped configs that pair
fsdp1_wrapped+BF_16+RotaryTransform.MIXED_PRECISION_MEGATRONleavesbuffer_dtypeunset and is unaffected.fsdp1_checkpointedwithprecision: BF16—TorchCheckpointLoading.load_model_checkpointdoes
model.to(self.device, dtype=...)(torch_checkpoint_loading.py:44), which casts thebuffer at load time regardless of what the file holds. Measured: 1.2e-01 rad at only seq 128 with
base_freq=500000. Same root cause inconversion_model.py:25, which builds the HF model with.to(dtype=torch.bfloat16); note thatcheck_converted_modelthen compares two equally-affectedmodels, and that exported checkpoints themselves are fine because
inv_freqispersistent=Falseon the HF side, so transformers and vLLM recompute it in FP32.
This is left as-is deliberately: FSDP1 has carried a
FutureWarningsince 0.4(
model_factory.py:112) and is not planned to be supported going forward. Closing it properlymeans not trusting the buffer's dtype at all — regenerating the frequencies when
self.inv_freq.dtype != torch.float32rather than upcasting (verified to bring both paths to2.8e-03 rad) — which is more surgery than a deprecated path warrants.
Related:
inv_freqis a persistent buffer, so a checkpoint written by an FSDP1 +BF_16runstores BF16-rounded frequencies and
load_state_dictcasts rather than rejects them(max |loaded - correct| = 1.2e-03, exactly BF16-rounded). Such a checkpoint stays affected after
this fix. FSDP2 +
dcpcheckpoints are unaffected, as above.Validation
pytest tests(excludingend2end_tests, single node): 497 passed, 44 skipped, no regressionspytest tests/conversion tests/test_rotary_qkv_transform.py tests/nn: 37 passed