diff --git a/.agents/skills/check-trainer-consistency/SKILL.md b/.agents/skills/check-trainer-consistency/SKILL.md new file mode 100644 index 00000000000..103d78a2e44 --- /dev/null +++ b/.agents/skills/check-trainer-consistency/SKILL.md @@ -0,0 +1,42 @@ +--- +name: check-trainer-consistency +description: Keep duplicated trainer code aligned across TRL trainers. Use when modifying or reviewing code in any trainer (GRPO, RLOO, SFT, DPO, ...) that also exists in sibling trainers, e.g. generation paths, reward computation, metric logging, or weight syncing. +--- + +# Check trainer consistency + +Trainers in TRL are **self-contained by design**. Shared logic (vLLM generation paths, `_get_per_token_logps_and_entropies`, `_calculate_rewards`, `_prepare_inputs`, metric logging, weight syncing) is deliberately duplicated across trainers instead of being abstracted into a base class, so each trainer stays readable and evolvable in isolation. + +The tradeoff: duplication is accepted, but **consistency is mandatory**. + +## Rules for duplicated blocks + +- Same variable names (`self._last_loaded_step`, `self._metrics[mode]`, ...). +- Same control-flow structure (if/elif/else branches in the same order). +- Same comments, word-for-word when the logic is identical. +- Divergences only where the trainer's semantics require it (e.g. GRPO extracts logprobs from vLLM, RLOO discards them). + +**Consistency over correctness.** When duplicating code, reproduce it exactly — even if you believe the original has a bug. Do not silently fix the issue in your copy: keep it consistent and report the problem so it can be fixed across all trainers in one dedicated PR. A consistently-wrong codebase can be fixed in a single sweep; an inconsistent one cannot. + +## When modifying duplicated code + +1. Identify the duplicated block you are changing. +2. Find every copy. Grep a distinctive line of the block across the main and experimental trainers: + ```sh + grep -rn "self._last_loaded_step" trl/trainer/ trl/experimental/ + ``` +3. Apply the same change to every copy. A fix in GRPO usually implies the same fix in RLOO and vice versa. Not propagating a change is a bug. +4. Verify the copies stayed aligned by diffing the corresponding regions, e.g.: + ```sh + diff <(sed -n '/def _generate_single_turn/,/def /p' trl/trainer/grpo_trainer.py) \ + <(sed -n '/def _generate_single_turn/,/def /p' trl/trainer/rloo_trainer.py) + ``` + Remaining diffs must all be semantic divergences, not drift. + +## When reviewing + +If a PR touches duplicated logic, check that all copies were updated consistently. The most common mistake is fixing one trainer and forgetting the others. + +## Scope + +Main code (`trl/trainer/`) must stay stable and consistent. Experimental code (`trl/experimental/`) may lag; small non-invasive alignment improvements are welcome, large refactors are not. diff --git a/.agents/skills/update-paper-index/SKILL.md b/.agents/skills/update-paper-index/SKILL.md new file mode 100644 index 00000000000..a4b9c37035b --- /dev/null +++ b/.agents/skills/update-paper-index/SKILL.md @@ -0,0 +1,36 @@ +--- +name: update-paper-index +description: Add or review a paper entry in TRL's paper index. Use when a PR implements a method, algorithm, or training approach from a research paper, or when reviewing such a PR. +--- + +# Update the paper index + +Any PR that implements a method, algorithm, or training approach from a research paper must add a corresponding subsection to `docs/source/paper_index.md`. When reviewing such a PR, check that the file was updated. + +## Entry format + +The file is organized as one `##` section per method family (usually one per trainer), each holding `###` subsections, one per paper. + +Each entry contains: + +1. The paper title as the `###` heading. +2. A paper link line, using the Hugging Face paper page (same ID as arXiv), never an arxiv.org link: + ``` + **📜 Paper**: https://huggingface.co/papers/ + ``` +3. A few sentences on what the paper introduces and how it maps to TRL. +4. A Python snippet showing the TRL config that reproduces the paper's setting, with the paper's hyperparameters quoted in comments: + ```python + from trl import GRPOConfig, GRPOTrainer + + training_args = GRPOConfig( + beta=0.001, # "the KL coefficient to 0.001" + num_generations=16, # "For each question, we sample 16 outputs..." + ) + ``` + When the paper doesn't specify hyperparameters, say so in a comment rather than inventing values. + +## Placement + +- If the paper belongs to an existing method family, add it under that `##` section. +- If it introduces a new trainer or family, add a new `##` section with a one-line pointer to the trainer, e.g. `Papers relating to the [`GRPOTrainer`].` diff --git a/.gitignore b/.gitignore index 58239cfec94..825e54719e1 100644 --- a/.gitignore +++ b/.gitignore @@ -147,5 +147,4 @@ wandb/ uv.lock # AI agent generated artifacts -/.agents/skills /.claude/skills \ No newline at end of file diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 3ec168688b2..53a35bbb4e6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -421,10 +421,8 @@ By following this classification, you ensure that warnings, information, and exc ## Coding with AI agents -This repository keeps AI-agent configuration in `.ai/` and exposes local agent files via symlinks. +This repository keeps AI-agent configuration in `.ai/` and skills in `.agents/skills/`. -Skills can be exposed to agents by running `make codex` or `make claude` +`AGENTS.md`, `CLAUDE.md`, and `.cursor/BUGBOT.md` all point to `.ai/AGENTS.md`. Cursor reads `AGENTS.md` and Bugbot reads `.cursor/BUGBOT.md`. -`AGENTS.md`, `CLAUDE.md`, and `.cursor/BUGBOT.md` all point to `.ai/AGENTS.md`. - -Cursor reads `AGENTS.md` and Bugbot reads `.cursor/BUGBOT.md`. Cursor reads skills from Claude or Codex paths, so setting up the repository for Claude or Codex will work for Cursor. +Codex, Cursor and Gemini CLI read `.agents/skills/` directly. Claude Code reads `.claude/skills/`, so run `make claude` to symlink it to `.agents/skills/` (and `make clean-ai` to remove it). diff --git a/Makefile b/Makefile index 76732b63afb..36c0dda1ac6 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,4 @@ -.PHONY: test precommit common_tests slow_tests tests_gpu test_experimental codex claude clean-ai +.PHONY: test precommit common_tests slow_tests tests_gpu test_experimental claude clean-ai check_dirs := examples tests trl @@ -17,15 +17,10 @@ slow_tests: test_experimental: pytest -n auto -s -v tests/experimental -codex: - mkdir -p .agents - rm -rf .agents/skills - ln -snf ../.ai/skills .agents/skills - claude: mkdir -p .claude rm -rf .claude/skills - ln -snf ../.ai/skills .claude/skills + ln -snf ../.agents/skills .claude/skills clean-ai: - rm -rf .agents/skills .claude/skills \ No newline at end of file + rm -rf .claude/skills \ No newline at end of file