Skip to content

Fix cross-rank metric averaging in the experimental self-distillation trainers - #7654

Open
ucalyptus wants to merge 4 commits into
huggingface:mainfrom
ucalyptus:sdft-metric-averaging-fix
Open

ucalyptus wants to merge 4 commits into
huggingface:mainfrom
ucalyptus:sdft-metric-averaging-fix

Conversation

@ucalyptus

@ucalyptus ucalyptus commented Oct 12, 2026 •

Copy link
Copy Markdown
Contributor

Split out of #6237, which grew to cover several unrelated changes. This is the first piece.

Two fixes that SDFTTrainer and SDPOTrainer share:

  • The distillation-loss metric averaged each rank's own masked mean with an unweighted gather(...).mean(). When completion lengths vary, ranks hold different numbers of valid tokens, so a rank with few valid tokens counted as much as one with many. The logged value was biased. _global_masked_mean sums the local numerator and denominator and divides once, which is what GRPO and RLOO do.
  • local_sum is cast to float32 before torch.stack. values can be bf16 or fp16 under mixed precision while the count is float32, and torch.stack needs matching dtypes. The cast also avoids precision loss when many low-precision values are summed.
  • train_dataset is checked for type at construction, so passing a list or a DataFrame fails with a clear message rather than later.

Both trainers get the same code, per the repository's consistency rule for duplicated logic.

Tests

CPU, transformers 5.x, trl-internal-testing/tiny-Qwen2ForCausalLM-2.5:

pytest tests/experimental/test_sdft_trainer.py -q                       18 passed
pytest tests/experimental/test_sdpo_trainer.py -q                       21 passed
pytest tests/experimental/test_self_distillation_trainer_behavior.py -q 12 passed

Note

Low Risk
Changes affect logged metrics and constructor validation only; optimization loss computation is untouched.

Overview
Fixes distributed logging for self-distillation metrics in SDFTTrainer and SDPOTrainer so masked means are token-weighted across ranks instead of averaging per-rank means with gather(...).mean().

A local global_masked_mean helper sums masked values and valid-token counts per rank, accelerator.reduces them, and divides once—matching GMPO-style aggregation. This applies to logged distillation loss (local and vLLM server paths) and the server logprob abs-diff diagnostic. Training loss math is unchanged.

Adds early train_dataset type checks: SDFTTrainer requires a Hugging Face Dataset; SDPOTrainer accepts Dataset or IterableDataset, with a clear TypeError for invalid inputs.

Reviewed by Cursor Bugbot for commit 17dc011. Bugbot is set up for automated code reviews on this repo. Configure here.

@ucalyptus
ucalyptus marked this pull request as ready for review October 13, 2026 00:03
Each rank's own masked mean was averaged with an unweighted `gather(...).mean()`,
which is biased when completion lengths vary and ranks hold different numbers of
valid tokens: a rank with few valid tokens counted as much as one with many.
`_global_masked_mean` sums the local numerator and denominator and divides once,
which is the GRPO/RLOO pattern.

The numerator is cast to float32 before `torch.stack`, because the values can be
bf16 or fp16 under mixed precision while the count is float32, and the cast also
avoids precision loss when many low-precision values are summed.

`train_dataset` is type-checked at construction, so a wrong type fails with a
clear message instead of later. Both trainers get the same code, per the
repository's consistency rule for duplicated logic.
@ucalyptus
ucalyptus force-pushed the sdft-metric-averaging-fix branch from 8ee66d0 to f598ca9 Compare October 13, 2026 01:08
ucalyptus added a commit to ucalyptus/trl that referenced this pull request Oct 13, 2026
The combined branch: DOPD advantage-gap routing, the distillation plumbing
(kl_clip, teacher top-k support, teacher chat-template kwargs with untruncated
teacher tokenization, QLoRA, EMA teacher adapter pre-registration, parallelism
rejection), and the cross-rank metric averaging fix, with the config validation
and tests for each.

Split into huggingface#7654 (metric), huggingface#7655 (plumbing), and huggingface#7656 (DOPD) for review. The
PEFT + ZeRO-3 guard that this branch also carried was dropped: it rejected a
combination that is known to work, and huggingface#6829 owns that fix.

@qgallouedec qgallouedec left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Pushed 7dcf875 to inline global_masked_mean like GRPO.

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 1925617. Configure here.

def global_masked_mean(x):
local_sum, local_count = (x * mask).sum(), mask.sum().float()
totals = self.accelerator.reduce(torch.stack([local_sum, local_count]), reduction="sum")
return (totals[0] / totals[1].clamp(min=1.0)).item()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mixed-precision metric average dtype crash

High Severity

global_masked_mean stacks local_sum with local_count after casting only the count with .float(). With bf16 or fp16 losses, the masked sum stays low precision, so torch.stack raises and the step dies while logging distillation metrics. The same helper is copied in both trainers, including the server-loss paths.

Additional Locations (2)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 1925617. Configure here.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants