Propagate rope_scaling_factor from args to GPT model construction - #6306
Draft
Alvorecer721 wants to merge 1 commit into
Draft
Propagate rope_scaling_factor from args to GPT model construction#6306Alvorecer721 wants to merge 1 commit into
Alvorecer721 wants to merge 1 commit into
Conversation
--rope-scaling-factor was parsed but never forwarded to the model: gpt_config_from_args, gpt_builder, and the ModelOpt model builder all passed rope_scaling without the factor, so GPTModel always fell back to its default of 8.0. Training with any non-default factor (e.g. 32.0 for Llama 3.2 style checkpoints) silently used 8.0 instead. Forward args.rope_scaling_factor at the three construction sites and add regression tests covering the config path and the builder path. Signed-off-by: Alvorecer721 <yixuan.xu@ai.ethz.ch>
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
2 participants
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.
What does this PR do?
Forwards
rope_scaling_factorfrom the parsed arguments toGPTModel, so--rope-scaling-factoractually takes effect.Issue tracking
Linked issue: Fixes #6305
Problem
All three GPT construction sites pass
rope_scaling=args.use_rope_scalingbut not the factor, soGPTModelalways falls back to its default of 8.0:gpt_config_from_argsinmegatron/training/argument_utils.py(pretrain_gpt.py, train_rl.py, inference utils)gpt_builderingpt_builders.py(train_rl.py, checkpoint converter tools)megatron/post_training/model_builder.pyRuns using the Llama 3.1 factor of 8.0 are unaffected because the defaults happen to match, which is why this went unnoticed. Anything else, e.g. 32.0 for Llama 3.2 style scaling, silently trains with 8.0.
#2902 fixed this on the old layout but was closed unmerged; the training migration (#4741) kept the gap in the new path.
Changes
args.rope_scaling_factorat the three construction sites (one line each).gpt_config_from_argswith a non-default factor must produce a config carrying that factor (tests/unit_tests/training/models/test_gpt_builder.py).gpt_buildermust includerope_scaling_factorin itsGPTModelcall (tests/unit_tests/models/test_gpt_model.py).Testing
pytest -q tests/unit_tests/training/models/test_gpt_builder.py pytest -q "tests/unit_tests/models/test_gpt_model.py::test_gpt_builder_forwards_rope_scaling_factor"Both new tests fail before the fix (the config keeps 8.0; the kwarg is missing from the call) and pass after. All 58 tests in
test_gpt_builder.pypass.Contribution process
Pre-checks