-
Notifications
You must be signed in to change notification settings - Fork 543
feat(megatron-bridge): SFT-masked data support in distillation #2113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
14ccdf0
fb1c84d
cf733d5
f015f01
8057060
44a2f42
7baef79
34bf2fe
9467589
9fd399a
8fe9543
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
kevalmorabia97 marked this conversation as resolved.
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,7 @@ | |
| from megatron.bridge.training.config import ( | ||
| CheckpointConfig, | ||
| ConfigContainer, | ||
| FinetuningDatasetConfig, | ||
| GPTDatasetConfig, | ||
| LoggerConfig, | ||
| MockGPTDatasetConfig, | ||
|
|
@@ -45,10 +46,11 @@ | |
| from megatron.bridge.training.distill import distill | ||
| from megatron.bridge.training.post_training.checkpointing import has_modelopt_state | ||
| from megatron.bridge.training.post_training.distillation import ModelOptDistillConfig | ||
| from megatron.bridge.utils.vocab_utils import calculate_padded_vocab_size | ||
| from megatron.core.datasets.utils import get_blend_from_list | ||
| from megatron.core.distributed import DistributedDataParallelConfig | ||
| from megatron.core.utils import unwrap_model | ||
| from transformers import AutoConfig | ||
| from transformers import AutoConfig, AutoTokenizer | ||
|
|
||
| import modelopt.torch.distill as mtd | ||
| import modelopt.torch.utils.distributed as dist | ||
|
|
@@ -125,6 +127,20 @@ def get_args(): | |
| parser.add_argument( | ||
| "--use_mock_data", action="store_true", help="Use mock data instead of --data_paths" | ||
| ) | ||
| parser.add_argument( | ||
|
kevalmorabia97 marked this conversation as resolved.
|
||
| "--sft", | ||
| action="store_true", | ||
| help="Distill on prompt-completion jsonl from --sft_dataset_root with the loss masked to " | ||
| "the completion, instead of pre-tokenized --data_paths.", | ||
| ) | ||
| parser.add_argument( | ||
| "--sft_dataset_root", | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we forsee a use case of providing HF SFT dataset name instead of a local SFT dataset root?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, and it's written — I've pulled it out of this PR to keep the scope to one thing (local prompt-completion JSONL) so this can land quickly, and will send it as a separate PR. Two reasons it isn't ready to review yet:
|
||
| type=str, | ||
| default=None, | ||
| help="Directory holding training.jsonl (and validation.jsonl when --eval_iters > 0) of " | ||
| '{"input": <prompt>, "output": <response>} records (used with --sft). See the README for ' | ||
| "how the fields are tokenized and truncated.", | ||
| ) | ||
| # Training & Eval arguments | ||
| parser.add_argument( | ||
| "--output_dir", type=str, required=True, help="Folder for logging and checkpoint saving" | ||
|
|
@@ -246,7 +262,7 @@ def get_args(): | |
| args = parser.parse_args() | ||
|
|
||
| # Sanity checks | ||
| if not args.use_mock_data and not args.data_paths: | ||
| if not args.sft and not args.use_mock_data and not args.data_paths: | ||
| raise ValueError("Must provide either --data_paths or set --use_mock_data.") | ||
|
|
||
| if args.student_hf_model is None: | ||
|
|
@@ -256,11 +272,59 @@ def get_args(): | |
| if args.validate_only and args.eval_iters == 0: | ||
| raise ValueError("--validate_only requires --eval_iters > 0.") | ||
|
|
||
| if args.sft and not args.sft_dataset_root: | ||
| raise ValueError( | ||
| "--sft requires --sft_dataset_root (a directory with training.jsonl, plus " | ||
| "validation.jsonl when --eval_iters > 0)." | ||
| ) | ||
| if args.sft and (args.data_paths or args.use_mock_data): | ||
| raise ValueError( | ||
| "--sft is mutually exclusive with --data_paths / --use_mock_data: the SFT branch wins " | ||
| "the dataset selection, so those inputs would be silently ignored." | ||
| ) | ||
| if args.sft_dataset_root and not args.sft: | ||
| raise ValueError("--sft_dataset_root requires --sft; without it the SFT path is not used.") | ||
| if args.sft: | ||
| # Fail on a mistyped root here rather than after both checkpoints have loaded onto GPUs. | ||
| required = ["training.jsonl"] + (["validation.jsonl"] if args.eval_iters > 0 else []) | ||
| absent = [f for f in required if not os.path.isfile(os.path.join(args.sft_dataset_root, f))] | ||
| if absent: | ||
| raise ValueError(f"--sft_dataset_root {args.sft_dataset_root} is missing: {absent}.") | ||
| # Decided once here so it reaches print_args and costs a single tokenizer load. | ||
| args.sft_add_bos = _tokenizer_prepends_bos(args) | ||
|
|
||
| _check_shared_vocabulary(args) | ||
|
|
||
|
Comment on lines
+280
to
+297
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SUGGESTION] The mutual-exclusion check covers if args.sft_dataset_root and not args.sft:
raise ValueError("--sft_dataset_root requires --sft; without it the SFT data path is not used.") |
||
| print_args(args) | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
|
|
||
| return args | ||
|
|
||
|
|
||
| def _check_shared_vocabulary(args) -> None: | ||
|
AAnoosheh marked this conversation as resolved.
|
||
| """Raise unless teacher and student use the same tokenizer.""" | ||
| _tok = {"trust_remote_code": args.trust_remote_code} | ||
| student_vocab = AutoTokenizer.from_pretrained(args.student_hf_path, **_tok).get_vocab() | ||
| teacher_vocab = AutoTokenizer.from_pretrained(args.teacher_hf_path, **_tok).get_vocab() | ||
| if student_vocab != teacher_vocab: | ||
| raise ValueError( | ||
| "Distillation scores the teacher on the student's token ids, so teacher and student " | ||
| "must use the same tokenizer." | ||
| ) | ||
|
|
||
|
|
||
| def _tokenizer_prepends_bos(args) -> bool: | ||
| """True when the student tokenizer prepends a BOS at inference. | ||
|
|
||
| Probes an encode: fast tokenizers prepend via a post-processor that exposes no attribute. | ||
| """ | ||
| tokenizer = AutoTokenizer.from_pretrained( | ||
| args.student_hf_path, trust_remote_code=args.trust_remote_code | ||
| ) | ||
| if not getattr(tokenizer, "bos_token", None): | ||
| return False | ||
| return tokenizer("x").input_ids[:1] == [tokenizer.bos_token_id] | ||
|
|
||
|
|
||
| def main(args: argparse.Namespace): | ||
| checkpoint_dir = os.path.join(args.output_dir, "checkpoints") | ||
| tensorboard_dir = os.path.join(args.output_dir, "tb_logs") | ||
|
|
@@ -279,6 +343,10 @@ def _build_model_provider(hf_path, load_weights=True): | |
| provider.expert_model_parallel_size = args.ep_size | ||
| provider.expert_tensor_parallel_size = 1 # Expert tensor parallelism is not supported | ||
| provider.seq_length = args.seq_length | ||
| if args.sft: | ||
| # A response-only loss mask needs per-token reduction to combine across CP ranks. | ||
| # Must stay in sync with ``average_in_collective=not args.sft`` on the DDP config. | ||
| provider.calculate_per_token_loss = True | ||
|
Comment on lines
+346
to
+349
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [SUGGESTION] Since this and line 450 are two halves of one decision that must stay in sync, a one-line note here pointing at the DDP setting — or moving the comment to mention both — would keep a future edit from flipping one without the other.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accepted — 8c19f38. Kept the flag where it is (it does have to agree with the single shared |
||
| if args.recompute_granularity is not None: | ||
| provider.recompute_granularity = args.recompute_granularity | ||
| provider.recompute_method = args.recompute_method | ||
|
|
@@ -302,14 +370,26 @@ def _build_model_provider(hf_path, load_weights=True): | |
| student_provider.gradient_accumulation_fusion = False | ||
| teacher_provider = _build_model_provider(args.teacher_hf_path) | ||
|
|
||
| # The KD losses compare logits elementwise over the vocab dim, so both output layers must have | ||
| # the same padded width. A shared tokenizer does not imply it: the HF configs can disagree. | ||
| padded = { | ||
| name: calculate_padded_vocab_size( | ||
| p.vocab_size, p.make_vocab_size_divisible_by, p.tensor_model_parallel_size | ||
| ) | ||
| for name, p in (("student", student_provider), ("teacher", teacher_provider)) | ||
| } | ||
| if padded["student"] != padded["teacher"]: | ||
| raise ValueError( | ||
| "Distillation needs student and teacher logits of equal width, but their padded vocab " | ||
| f"sizes differ ({padded['student']} vs {padded['teacher']})." | ||
| ) | ||
|
|
||
| kd_config = ModelOptDistillConfig( | ||
| skip_lm_loss=not args.no_skip_lm_loss, kd_loss_scale=args.kd_loss_scale | ||
| ) | ||
|
|
||
| # VLM detection convention: HF VLM configs expose a ``vision_config``, and Megatron-Bridge nests | ||
| # the text model under the ``language_model`` submodule (used as ``distill_submodule`` below). If a | ||
| # future model breaks either convention, the ``getattr(model, "language_model")`` in the provider | ||
| # will error loudly rather than silently distilling the wrong module. | ||
| # HF VLM configs expose ``vision_config``; Megatron-Bridge nests the text model under | ||
| # ``language_model`` (used as ``distill_submodule`` below). | ||
| is_vlm = hasattr( | ||
| AutoConfig.from_pretrained(args.student_hf_path, trust_remote_code=args.trust_remote_code), | ||
| "vision_config", | ||
|
|
@@ -368,7 +448,33 @@ def _restore_student_hook(model_chunks): | |
| "dataloader_type": "single", | ||
| "skip_getting_attention_mask_from_dataset": True, | ||
| } | ||
| if args.use_mock_data: | ||
| if args.sft: | ||
| # SFT-masked distillation via Bridge's FinetuningDatasetConfig -> NeMo-style GPTSFTDataset, | ||
| # reading {"input", "output"} jsonl. Fields are tokenized as written except that each is | ||
| # ``.strip(" ")``-ed; see --sft_dataset_root help. | ||
| dataset_config = FinetuningDatasetConfig( | ||
| seq_length=args.seq_length, | ||
| dataset_root=args.sft_dataset_root, | ||
| seed=args.seed, | ||
| dataloader_type="batch", | ||
| # Honour --eval_iters 0 so a training-only dataset_root does not have to carry a | ||
| # dummy validation.jsonl just to satisfy the builder. | ||
| do_validation=args.eval_iters > 0, | ||
| do_test=False, | ||
|
kevalmorabia97 marked this conversation as resolved.
|
||
| dataset_kwargs={ | ||
| "prompt_template": "{input}{output}", | ||
| "label_key": "output", | ||
| "truncation_field": "input", | ||
|
kevalmorabia97 marked this conversation as resolved.
|
||
| # Drop the oldest context. The default "right" would cut the prompt/answer | ||
| # boundary and then "output" itself, the only span the loss is computed on. | ||
| "truncation_method": "left", | ||
|
Comment on lines
+466
to
+470
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [IMPORTANT Compatibility] Two decisions in this same diff work against each other:
Why it matters. The result is the same train/inference skew the BOS warning exists to prevent, except now it is per-record and invisible: short records train with the BOS, long ones train without it, and nothing in the logs distinguishes them. At Suggestion. Say so where the user is told to bake in the BOS. Extend the
If you want a runtime signal rather than only docs,
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @yueshen2016 Can you take a look at this?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. add_bos now comes from a tokenizer probe instead of being hardcoded False, so Bridge prepends the BOS after truncation and budgets for it in total_ids . |
||
| "answer_only_loss": True, | ||
| # Prepended after truncation, so it survives a record that had to be cut. | ||
| "add_bos": args.sft_add_bos, | ||
| "add_eos": True, | ||
| }, | ||
|
coderabbitai[bot] marked this conversation as resolved.
Comment on lines
+464
to
+475
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [IMPORTANT Compatibility] For a model family whose tokenizer/chat template always prepends BOS (Llama The escape hatch (bake the full chat-formatted prompt, including BOS and role markers, into the Two options, either is fine:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accepted — took option 2, in 080f4de. Deriving So the requirement is now stated in all three places a user could look:
Each says the same thing: both fields are tokenized verbatim, no chat template is applied and no BOS is prepended, so bake in any role/turn markers and BOS the model expects.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Following up on this thread: I took option 2 (document the contract), and after digging further I think option 1 as written would have been actively wrong for this model.
and its Even the corrected predicate ( What your comment did expose is a real gap the documentation alone does not close: a model with def _warn_if_bos_missing(tokenizer, dataset_root: str) -> None:
bos = getattr(tokenizer, "bos_token", None)
if not bos or not getattr(tokenizer, "add_bos_token", False):
return
...
if not first_input.startswith(bos):
warn_rank_0(f"This tokenizer prepends {bos!r} at inference, but the first record ... ")Verified across five cases: silent for For data that is not pre-templated at all, the right answer is Bridge's
kevalmorabia97 marked this conversation as resolved.
|
||
| ) | ||
| elif args.use_mock_data: | ||
| dataset_config = MockGPTDatasetConfig(**dataset_kwargs) | ||
| else: | ||
| # Convert flat CLI list (e.g. ["1.0", "/path/data"]) to Megatron blend format | ||
|
|
@@ -399,7 +505,7 @@ def _restore_student_hook(model_chunks): | |
| grad_reduce_in_fp32=True, | ||
| overlap_grad_reduce=True, | ||
| overlap_param_gather=True, | ||
| average_in_collective=True, | ||
| average_in_collective=not args.sft, # per-token loss must not be pre-averaged | ||
| use_distributed_optimizer=True, | ||
| ), | ||
| dataset=dataset_config, | ||
|
|
@@ -412,8 +518,24 @@ def _restore_student_hook(model_chunks): | |
| wandb_entity=args.wandb_entity, # optional | ||
| wandb_exp_name=args.wandb_exp_name, | ||
| ), | ||
| tokenizer=TokenizerConfig( | ||
| tokenizer_type="NullTokenizer", vocab_size=distill_provider.vocab_size | ||
| tokenizer=( | ||
| # SFT reads raw text, so it needs the model's real tokenizer; the pretraining path | ||
| # consumes pre-tokenized data and keeps NullTokenizer. | ||
| TokenizerConfig( | ||
| tokenizer_type="HuggingFaceTokenizer", | ||
| tokenizer_model=args.student_hf_path, | ||
| hf_tokenizer_kwargs={ | ||
| "trust_remote_code": args.trust_remote_code, | ||
| # Default True would make text_to_ids inject a BOS at the answer boundary, | ||
| # since "{input}" and "{output}" are tokenized separately. Consumed by Bridge | ||
| # in training/tokenizers/config.py. | ||
| "include_special_tokens": False, | ||
| }, | ||
| ) | ||
|
kevalmorabia97 marked this conversation as resolved.
|
||
| if args.sft | ||
| else TokenizerConfig( | ||
| tokenizer_type="NullTokenizer", vocab_size=distill_provider.vocab_size | ||
| ) | ||
| ), | ||
| checkpoint=CheckpointConfig( | ||
| save_interval=( | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.