Skip to content

Remove the experimental BCO trainer - #7136

Open
qgallouedec wants to merge 2 commits into
mainfrom
remove-bco-trainer
Open

Remove the experimental BCO trainer#7136
qgallouedec wants to merge 2 commits into
mainfrom
remove-bco-trainer

Conversation

@qgallouedec

@qgallouedec qgallouedec commented Sep 9, 2026

Copy link
Copy Markdown
Member

Removes trl.experimental.bco (BCOTrainer, BCOConfig), added in #1599.

Why

Low adoption. 11 genuine trainings in the last 2 months as per our telemetry, and 1 model created this year: https://huggingface.co/models?other=bco,trl&sort=created

High maintenance cost for that. 1,600 lines carrying their own reference-model handling, DeepSpeed preparation, tokenization, and eval loop, plus a UDM density-ratio classifier that is the only reason TRL depends on scikit-learn and joblib at all. 106 commits touched it; the recent ones are all upkeep (#7102 peft floor, #6507 TQDM_DISABLE, #6192 ZeRO-3 + PEFT mixed dtype, #5665/#5666/#5683 PEFT plumbing, #5629 tokenizer type hints, #5567 processor support). It left the stable API 2025-10-21 (#4312) and the deprecated alias was dropped 2026-02-10 (#5045); no feature work since.

Notes

trl.experimental has no stability guarantee, so no deprecation cycle. The code stays in git history for anyone wanting to revive it.

The bco_pair DPO loss stays: it is the paired form of the same objective, is used by MPO, and does not need the trainer. The paper entry stays in paper_index.md for that reason, minus the pointer to the unpaired trainer.

BCO was the only consumer of the rest of what goes with it: the bco and scikit optional dependency groups (scikit-learn, joblib), is_joblib_available, and the require_sklearn test marker.


Note

Medium Risk
Breaking removal of an experimental public API (BCOTrainer); paired BCO via DPO bco_pair is unaffected.

Overview
Removes the experimental BCOTrainer / BCOConfig stack (trl.experimental.bco), including the full trainer implementation, its test suite, and the bco_trainer documentation page. Navigation and cross-links are updated so BCO no longer appears in the experimental trainer lists, dataset-format table, or docs index.

Dependency and test cleanup: optional extras bco and scikit (scikit-learn, joblib) are dropped from pyproject.toml, along with is_joblib_available, the require_sklearn pytest marker, and BCOTrainer registration in base_trainer.

What stays: DPOConfig loss_type="bco_pair" is unchanged; docs only drop the recommendation to use the removed trainer for unpaired data. The BCO paper entry in paper_index.md remains for paired bco_pair reproduction.

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

Removes `trl.experimental.bco` (`BCOTrainer`, `BCOConfig`), added in #1599, and
the pieces it was the only consumer of: the `bco` and `scikit` extras,
`is_joblib_available`, and `require_sklearn`. The `bco_pair` DPO loss stays.
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-09T14:48:09.467173Z a7783de PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@bot-ci-comment

bot-ci-comment Bot commented Sep 9, 2026

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.

@albertvillanova albertvillanova 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.

I'm not opposed to this, but I'd like us to be more careful about the reasoning, because whatever we settle here becomes the template for the other trainers sitting in trl.experimental for the same reason.

On the telemetry number. We should say exactly what is being counted because I read a different figure than the one here: 74 genuine users in the last month. Also, the window disqualifies the metric for this question: every BCO data point we have was collected after we demoted it, removed it from the docs index and made it print an experimental warning. It cannot distinguish "nobody wants this" from "we told everyone to stop".

None of this means BCO is widely used. It probably isn't. It means the numbers as presented can't carry the weight of the decision, and low usage of something we stopped supporting is partly something we caused. That inference deserves caution generally: it can reflect limited interest, but also gaps in support or usability. For BCO specifically, UDM (the thing that distinguishes it from KTO) requires the user to bring their own embedding model, embedding tokenizer and prepare_model glue, with no example and no CLI path. Near-zero adoption is what I would expect from that regardless of demand.

On the maintenance cost. The commit count overstates the marginal cost and invites the obvious rebuttal. Of the commits touching the BCO path, only a small fraction are BCO-specific; the rest are repo-wide sweeps (#5802 touched 88 files, #6978 42, #6180 24), where BCO's cost is one more file in a sed-and-review pass. The real argument is the one our contributing guidance makes: duplication is accepted but consistency is mandatory, so each extra trainer is a permanent tax on every sweep, and 1,600 lines of copied reference-model, DeepSpeed and tokenization code is a large surface to keep aligned. That case stands on its own. Likewise, scikit-learn and joblib are optional extras behind a lazy availability check, only needed when embedding_func is passed, so stock pip install trl never pulled them; the win is deleting two extras, not shrinking anyone's install.

What I'd rather do. #4223 moved these trainers with the stated plan to discuss later which get promoted, which stay and which get removed. That discussion never happened, #4223 is closed, and we are now making the call one PR at a time in threads only the people who already agree are reading.

2 ideas:

  1. Open a follow-up RFC and settle it once for everything in trl.experimental, agreeing the bar before the list so each case is decided by a rule rather than re-argued: time in experimental, no open issues or PRs from non-maintainers, no downstream consumers, and notice in the previous release. Worth separating the two populations in there too, since they arrived under different terms: the trainers we demoted in the #4223 wave had users on the stable API who never opted into "may vanish in any release", while everything incubated directly into experimental did. Notice is owed to the first group and not the second.
    • Happy to open it if you agree on this
  2. What about implementing a "removal" cycle (analog to the deprecation one) for every trainer we plan to remove? Give BCO one release of notice and merge it: a FutureWarning in BCOTrainer.__init__ naming KTOTrainer as the replacement and the target version, plus a line in the release notes.
    • That is the same three lines that used to sit in trl/trainer/bco_trainer.py until #5045, which told BCO users the old import path would go away in 0.29 and pointed them at #4223.
    • One cycle turns "removed without warning" into a record we can point at, and it gives us the one measurement telemetry can't: whether anyone turns up. If nobody does, the removal is unarguable.

CC: @huggingface/trl

@qgallouedec

Copy link
Copy Markdown
Member Author

Thks, fair on the reasoning, I'll fix the body.

On the telemetry gap: unfiltered I get the same ~70 you do, dropping the runs that report device=cpu takes it to ~10. Nobody genuinely trains on CPU, and #6996 shows what those rows are, the repro there was run on CPU by someone hunting bugs.

Which is the cost I'd really want in the RFC. #6996 and #6999 are an open issue plus a +145/-16 fix for the BCO encoder-decoder path, and the reporter said it plainly when I asked how he found it:

Bug hunt, not real use. I have never trained a BCO encoder-decoder model, and there is no user waiting on this that I know of.

Not isolated: 14 issues from that account since June, 11 still open, plus #6572 and #6581, the same BCO fix submitted twice. My point is triage costs, and every trainer we keep is more surface for machine-generated findings on code nobody runs. That is what's eating maintainer time.

It's also why I don't read trl.experimental as somewhere things can sit indefinitely. The overview frames it as a staging area with a conditional promotion path, removable in any release without prior deprecation, (+ #4223 has listed BCO as "May be removed" since 2025-10-07). Parking trainers there forever isn't the spirit of it.

So I'd push back on the FutureWarning. Not because notice is worthless, but because a cycle is a process we then have to run: pick the target version, land the warning, remember it next release, chase it when the release slips, answer this again in the removal PR. For 1,600 lines with no users, that overhead is exactly what we're trying to get back. A line in the release notes is enough.

RFC: yes, please open it, and let's settle the demoted vs incubated split there.

@albertvillanova albertvillanova 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.

That reconciles it, thanks. ~70 unfiltered is what I was reading, and the CPU filter explains the rest. Worth putting both numbers and the filter in the body, and the same CPU-filtered count for one trainer nobody disputes, so a reader can calibrate without asking

On triage, you're right and my commit-count point doesn't touch it. Sweeps are cheap, attention isn't, and the volume scales with agents deployed rather than with users. #6999 is the cost in the present tense: an open +145/-16 on the encoder-decoder path that someone would have to review, for a path the author says nobody is waiting on. Merging this makes that review and #6996 moot, which is a good argument.

Fair on the FutureWarning too. Your objection is to the two-step, and that's an accurate description of what a manual cycle costs. A release-notes line is proportionate here. One small ask: point that line at the RFC, so the notice doubles as an invitation to the discussion where it actually matters.

The thing I'd carry into the RFC is that the triage argument isn't really about BCO. Machine findings land on unmaintained code, and there are ~25 modules in trl.experimental that fit. This removes one surface. Followed to its conclusion, if report volume decides what stays, then whoever runs the most agents is choosing what TRL keeps. I'd rather we set the bar ourselves. That's also the answer to the process cost you describe: picking a version, landing a warning, remembering it, chasing the slip, re-arguing it in the removal PR is the cost of deciding case by case. Deciding once is how it goes away.

Opening the RFC now, with the demoted vs incubated split and triage surface as one of the axes.

@albertvillanova

Copy link
Copy Markdown
Member

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