-
Notifications
You must be signed in to change notification settings - Fork 102
Refactor SFT forward_backward to use TrainBatch #731
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
base: main
Are you sure you want to change the base?
Changes from all commits
de78fe3
96af886
2113a24
f274f17
2484949
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -9,6 +9,50 @@ | |||||
| import torch | ||||||
| import torch.nn.functional as F | ||||||
| from forge.data.utils import CROSS_ENTROPY_IGNORE_IDX | ||||||
| from forge.types import TrainBatch | ||||||
|
|
||||||
|
|
||||||
| def collate_sft(batch: list[dict[str, Any]]) -> TrainBatch: | ||||||
| """ | ||||||
| Collate function for SFT that returns a TrainBatch directly. | ||||||
|
|
||||||
| This is a wrapper around collate_padded that separates model inputs | ||||||
| from loss inputs, following the same pattern as GRPO's collate. | ||||||
|
Contributor
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.
Suggested change
|
||||||
|
|
||||||
| Args: | ||||||
| batch: List of samples, each containing tensor and non-tensor fields. | ||||||
| Expected keys: "tokens" (model input), "labels" (loss input), | ||||||
| and optionally "metrics" and other metadata. | ||||||
|
Comment on lines
+23
to
+25
Contributor
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. its not really "expected". If the keys are not there, nothing will break. Maybe we should enforce it? something like model_inputs["tokens"] = collated.pop("tokens") |
||||||
|
|
||||||
| Returns: | ||||||
| TrainBatch with model_inputs, loss_inputs, and meta fields separated. | ||||||
| """ | ||||||
| collated = collate_padded(batch) | ||||||
|
|
||||||
| # Separate model inputs, loss inputs, and metadata | ||||||
| model_inputs = {} | ||||||
| loss_inputs = {} | ||||||
| meta = {} | ||||||
|
|
||||||
| for key, value in collated.items(): | ||||||
| if key == "tokens": | ||||||
| model_inputs[key] = value | ||||||
| elif key == "labels": | ||||||
| loss_inputs[key] = value | ||||||
| elif key == "metrics": | ||||||
| meta[key] = value | ||||||
| else: | ||||||
| # Other tensor fields go to model_inputs, non-tensor to meta | ||||||
| if isinstance(value, torch.Tensor): | ||||||
| model_inputs[key] = value | ||||||
| else: | ||||||
| meta[key] = value | ||||||
|
Comment on lines
+45
to
+49
Contributor
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. not sure how if this would work for flexattn or varlen masks. Can you double check please? |
||||||
|
|
||||||
| return TrainBatch( | ||||||
| model_inputs=model_inputs, | ||||||
| loss_inputs=loss_inputs, | ||||||
| meta=meta, | ||||||
| ) | ||||||
|
|
||||||
|
|
||||||
| def collate_padded(batch: list[dict[str, Any]]) -> dict[str, Any]: | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,12 +4,15 @@ | |
| # This source code is licensed under the BSD-style license found in the | ||
| # LICENSE file in the root directory of this source tree. | ||
|
|
||
| from __future__ import annotations | ||
|
Contributor
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. i dont think that we need this |
||
|
|
||
| import logging | ||
| from enum import Enum | ||
| from typing import Any, Iterator, Literal, Union | ||
|
|
||
| import torch | ||
| import torch.distributed as dist | ||
| from forge.types import TrainBatch | ||
| from torch.nn.attention.flex_attention import BlockMask | ||
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
@@ -256,7 +259,7 @@ def __init__( | |
| def __iter__(self): | ||
| return self | ||
|
|
||
| def __next__(self) -> dict: | ||
| def __next__(self) -> TrainBatch: | ||
| """Get next batch from current epoch. | ||
|
|
||
| Returns: | ||
|
|
@@ -302,32 +305,34 @@ def __next__(self) -> dict: | |
| return current_batch | ||
|
|
||
|
|
||
| def extract_epoch_from_batch(batch: dict) -> int: | ||
| def extract_epoch_from_batch(batch: TrainBatch) -> int: | ||
| """Extract epoch number from batch metrics. Useful to detect epoch changes during validation. | ||
|
|
||
| Assumes batch contains field "metrics" with at least one Metric containing "num_epochs" in its key, as it is done in | ||
| `forge.src.data.datasets.HfIterableDataset`. | ||
|
|
||
| Args: | ||
| batch (dict): Batch dictionary with 'metrics' field | ||
| batch (TrainBatch): TrainBatch with 'metrics' field in meta | ||
|
|
||
| Returns: | ||
| int: Max epoch number from metrics | ||
|
|
||
| Raises: | ||
| ValueError: If metrics key is missing or no metric with 'num_epochs' found | ||
| """ | ||
| if "metrics" not in batch: | ||
| metrics = batch.meta.get("metrics") | ||
|
|
||
| if metrics is None: | ||
| raise ValueError( | ||
| "Batch missing 'metrics' field. Cannot extract epoch from batch." | ||
| "Batch missing 'metrics' field in meta. Cannot extract epoch from batch." | ||
| ) | ||
|
|
||
| # Match metrics where 'num_epochs' appears in the key (handles prefixed keys like 'dataset/name/num_epochs') | ||
| epochs = [metric.value for metric in batch["metrics"] if "num_epochs" in metric.key] | ||
| epochs = [metric.value for metric in metrics if "num_epochs" in metric.key] | ||
| if epochs: | ||
| return max(epochs) | ||
|
|
||
| raise ValueError( | ||
| f"No 'num_epochs' metric found in batch. Got metrics: " | ||
| f"{[m.key for m in batch['metrics']]}" | ||
| f"{[m.key for m in metrics]}" | ||
| ) | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i dont think that this captures the spirit of TrainBatch, which is to be able to call
if we handpick the args from the batch input, then it defeats the flexibility of inputs that we wanted to provide in the first place. Does this make sense?
also, inputs may also have some varlen or flexatnn mask, so we should check what this means for SFT. Perhaps take a look at the torchtitan train.py and see how they handle this mask that we removed in this PR because we couldnt use titan nightlies #614