fix(megatron): isolate FP32 adapter optimizer state - #2153
Draft
bvolpato wants to merge 1 commit into
Draft
Conversation
Signed-off-by: bvolpato <brunocvcunha@gmail.com>
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
With Megatron multi-LoRA and
trainer.bf16=false, switching adapters restores the model weights but leaves the previous adapter's Adam moments live. A fresh adapter can therefore update in the opposite direction to its own gradient.Megatron keeps native FP32 optimizer parameters in
shard_fp32_groups; AdapterStore currently snapshots onlyshard_fp32_from_float16_groups. Use one iterator over both layouts for slot allocation, snapshot, and restore. Existing BF16 masters and chained optimizers retain their ordering.Testing
Eight CPU numerical regressions compare interleaved adapters against separately trained Adam baselines: FP32, BF16, mixed groups, and chained optimizers, each with per-parameter or TE-style group counters. The tests use the actual AdapterStore and real PyTorch Adam updates; CUDA container types and pinning are replaced for CPU execution.
All eight tests pass. Against the unchanged production module, the six FP32/mixed/chained cases fail while both BF16 controls pass. Executed with PyTorch 2.11.0+cpu through isolated
uv; an independent review also ran all eight successfully. Required pre-commit hooks pass. The existing lifecycle module skips without Megatron; GPU/Transformer Engine execution remains unverified.The GitHub GPU job exited before running tests:
ANYSCALE_CLI_TOKENwas empty and Anyscale rejected the credentials.Repro command
From the repository root:
Downsides
Each adapter slot, including the pristine template, now stores native FP32 parameter shards and their optimizer state. For ordinary FP32 Adam this adds about 12 bytes per local FP32 parameter element, plus scalar state, and corresponding copies during swaps. The parameter copy duplicates values already in the DDP buffer snapshot; keeping the existing slot layout avoids a separate state-only path. BF16 parameter storage is unchanged.
Risk and rollback
This affects Megatron multi-LoRA swaps. Incorrect group ordering would show up as adapter-specific parameter or moment mismatches, which the mixed and chained numerical tests cover. Revert the commit and restart workers from saved checkpoints to roll back; the saved checkpoint format is unchanged.