Skip to content

Reduce inconsistency across trainer test files#5678

Open
qgallouedec wants to merge 12 commits intomainfrom
less-inconsistency
Open

Reduce inconsistency across trainer test files#5678
qgallouedec wants to merge 12 commits intomainfrom
less-inconsistency

Conversation

@qgallouedec
Copy link
Copy Markdown
Member

@qgallouedec qgallouedec commented Apr 29, 2026

What changed

  • Renamed test_training_*test_train_* (stragglers in DPO, GRPO, RLOO, and 10 experimental files)
  • Aligned wording: parametersparams, Check theCheck that the
  • Removed redundant section comments that describe WHAT instead of WHY (# Get the dataset, # Initialize the trainer, # Train the model, etc. — per CLAUDE.md guidance)
  • Fixed within-file outliers (period style, torch.equal vs torch.allclose, missing inline comments on shared config args)

Note

Low Risk
Test-only refactors (renames, comment cleanup, and assertion consistency) with minor dataset-loading adjustments; low risk outside of potentially changing which splits are exercised in a few tests.

Overview
Improves consistency across trainer test suites by renaming remaining test_training* methods to test_train*, tightening/standardizing assertion messaging, and removing redundant narrative comments.

Also aligns dataset usage in several tests (e.g., consistently using load_dataset(..., split="train") where a single-split dataset is expected) and standardizes parameter-change checks to use torch.equal (and params wording) for clearer, uniform test behavior.

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

@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

Comment thread tests/test_dpo_trainer.py Outdated
- Updated assertions in `test_grpo_trainer.py`, `test_reward_trainer.py`, `test_rloo_trainer.py`, and `test_sft_trainer.py` to use `torch.allclose` instead of `torch.equal` for better numerical stability when checking if parameters have changed.
- Ensured consistency in assertion messages across all modified tests.
Comment thread tests/test_grpo_trainer.py Outdated
Copy link
Copy Markdown

@cursor cursor Bot left a comment

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 and found 1 potential issue.

There are 2 total unresolved issues (including 1 from previous review).

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 c5a2467. Configure here.

Comment thread tests/test_callbacks.py Outdated
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.

3 participants