Drop trl skills install and move the skill source out of the package - #6793
Conversation
|
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1edd105ea
ℹ️ 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".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mkdir -p skills-repo/skills/trl | ||
| cp trl/.agents/skills/trl/SKILL.md skills-repo/skills/trl/ |
There was a problem hiding this comment.
Remove the old skill directory during synchronization
When this workflow runs against huggingface/skills after earlier releases populated skills/trl-training, these commands only add the renamed skills/trl directory and never delete the existing tracked directory. The generated PR will therefore publish both the current skill and a stale trl-training copy rather than completing the rename; remove skills/trl-training as part of this synchronization step.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a1edd10. Configure here.
|
I'll elaborate more on this, but I don’t think this is really a "convention". Rather, the FastAPI org made this choice and aligned its projects accordingly. In the HF ecosystem, our OSS projects are already aligned around Additionally, I think imposing this specific repo path for skills can be misleading, as it conflates two distinct audiences and use cases: skills for developing and contributing to the project versus skills for using the project/library. |
|
A few data points that shaped this PR (details in #6743): On "just a FastAPI org choice": On On conflating audiences: I'd argue the opposite. A skill inside the package ships in the wheel and reaches library users, version-matched to their install. A contributing skill lives at the repo root, outside the package, and never reaches a wheel (that is how gradio splits it). The path separates the two audiences cleanly I think. |
b945195 to
d864c74
Compare
|
Thanks for the detailed reply — and to be clear about what I'm not disputing: the diagnosis is right and the instrumentation is valuable. The skill we ship today is undiscoverable, and 0 invocations over ~3,300 tool calls is a real finding worth acting on. I also agree with dropping My objection is narrower: the destination path. I don't think 1.
|
| Location | What repo-scope means there |
|---|---|
A consumer's repo .agents/skills/trl/ |
"how to use TRL, in this application" — correct, and exactly where hf skills add installs |
TRL's own repo .agents/skills/ |
"how to contribute to TRL" — the same path, a different meaning |
hf skills add is right because it materializes the artifact in the consumer's tree. This PR takes a consumer-facing artifact and stores it in TRL's tree, at a path that in TRL's tree means something else. The wheel is a side effect of that placement, not a justification for it.
Concretely, on library-skills as the bridge: its default target is the workspace-root .agents/skills, as relative symlinks, and its docs note projects "can commit them to Git." So for any contributor with an editable TRL install who runs it, TRL's library-user skill lands as a committable symlink inside TRL's contributor skill surface. That's the conflation, mechanically.
On "gradio splits it that way" — I checked, and it doesn't. gradio/.agents/skills/ at the repo root currently holds gradio, gradio-themes, hf-gradio, and frontend-unit-testing. The first is user-facing by its own frontmatter ("Build Gradio web UIs and demos in Python. Use when creating, modifying, debugging..."); the last is contributor-shaped. So gradio's repo root mixes both audiences — it's the clearest available example of the problem, not of the split. (It's also not in-wheel, so it isn't evidence for this PR's path either way.)
2. There is no standard here to align with
The Agent Skills spec specifies the format — SKILL.md, frontmatter fields, scripts/, references/, assets/, progressive disclosure. It defines zero discovery locations and no distribution mechanism. You noted this yourself in #6743, and I think it's the decisive point: there is no standard being followed here, only a format being shared.
library-skills is a single individual's repository (tiangolo/library-skills), created 2026-04-26 — roughly four months old — at ~776 stars. That's a promising idea from a very credible author. It is not a cross-ecosystem convention, and I don't think a core HF library should ratify a third-party filesystem layout on that basis.
The adopter list also reads stronger than it is: fastapi and typer are both in the fastapi org, i.e. the tool author's own projects. streamlit is the one genuinely independent in-wheel adopter (lib/streamlit/.agents/skills/developing-with-streamlit — which, incidentally, doesn't follow library-skills' own <libname>/ prefix recommendation). gradio and trackio are repo-root, not in-wheel. So the honest count for the path this PR adopts is: the author's two projects, plus Streamlit.
Meanwhile the spec's own discovery work (agentskills#255, spec in #254, building on Cloudflare's RFC) is heading toward .well-known HTTP discovery, explicitly "no registry, no package manager." Baking the artifact into the wheel is a bet against the direction the format's own maintainers are exploring.
3. The proposed mechanism is weaker than what TRL already ships
This is the part I'd most like to settle on facts. hf skills is not a marketplace-only flow — check hf skills add --help:
Default location is in the current directory (
.agents/skills) or user-level (~/.agents/skills). If--claudeis specified, the skill is also symlinked into Claude's legacy skills directory.
So:
in-wheel + uvx library-skills |
hf skills add trl-training |
|
|---|---|---|
| Destination | .agents/skills (or .claude/skills if .claude/ exists) |
same — .agents/skills, ~/.agents/skills with -g, or --dest |
| Update path | re-run after upgrading the lib | hf skills update (first-class) |
| Claude Code | not covered — library-skills' docs: "Claude Code doesn't support the standard .agents directory" |
--claude symlinks into ~/.claude/skills |
| Extra tool to know about | third-party, must be discovered | hf ships transitively with every TRL install (via transformers/datasets/accelerate) |
| Time to fix a bad skill | next TRL release | same day, for every existing install |
The destination isn't in dispute — both write to .agents/skills. Only the source of truth is. And on that, the "copies freeze at install day" argument actually inverts: a skill frozen in the wheel can only be corrected by cutting a release, whereas the Hub-hosted skill can be fixed immediately for users who already installed. Given that the concrete complaint in #6743 is that the current skill's content is wrong (CLI-shaped, covers 1 of ~9 load-bearing fields), "fixable without a release" seems like the property we actually want.
huggingface/skills is also not aspirational — it currently carries 25 skills including trl-training, huggingface-gradio, huggingface-trackio, huggingface-vision-trainer, train-sentence-transformers, and transformers-js. The HF ecosystem has already converged, and TRL is already wired in via sync-huggingface-skills.yml.
4. The experiment doesn't discriminate between the options
0 invocations over two runs establishes that a skill sitting at a path nothing scans is never loaded. It doesn't tell us which scanned path to use — any of the three delivery mechanisms would move that number off zero.
The planned rerun has the same issue, plus a confound: if it ships the moved path and the rewritten content (step 3), we can't attribute the delta. If we want a number that settles this, I'd suggest holding content fixed and varying only delivery, with three arms: (a) nothing installed, (b) hf skills add trl-training, (c) in-wheel + library-skills. My prediction is (b) ≈ (c), because both end at the same .agents/skills path — which would make the path question a pure maintenance-surface decision rather than an empirical one.
5. What I'd support
Splitting this PR:
- Land now: remove
trl skills installandtrl/skills/as a CLI surface. Agreed, no objection. - Land now, highest value: Make TRL's skill discoverable from an install, and drop
trl skills install#6743 step 3 — rewrite the skill around the Python API (the ~9 load-bearing fields, the GRPO reward-fn signature, dataset formats). This is the part that most plausibly explains non-invocation and it's entirely independent of where the file lives. - Keep user-facing delivery on
huggingface/skills+hf skills add/update, and fix the sync workflow (it also currently leaves a staleskills/trl-trainingbehind on rename, per the bot review). - Repurpose a repo-root
.agents/skills/for what that surface is for — contributor skills. TRL has unusually good candidates, becauseAGENTS.mdalready documents knowledge an agent cannot infer from the source:check-trainer-consistency(the deliberate-duplication rule — same variable names, same branch order, same comments across trainers, propagate a fix to every copy),add-trainer,update-paper-index,run-slow-tests,prepare-release. That's repo-specific, task-shaped, costly-to-get-wrong, and maintained in the same PR as the workflow it describes. - Escalate the in-wheel scanner (Make TRL's skill discoverable from an install, and drop
trl skills install#6743 step 4) tohuggingface_hubas its own discussion. If HF decides in-package discovery is right, it should be an ecosystem decision with a path we control — and one that can resolve the installed distribution's version against a Hub-hosted skill, which gets the version-matching benefit without freezing bytes in the wheel.
Happy to open the contributor-skills PR for (4) so the split is concrete rather than hypothetical.
3d21ef4 to
562087e
Compare
|
thanks for the review, I think it makes sense. First, the numbers: the rerun (same task, skill made discoverable via library-skills) invoked the skill once in 255 tool calls, vs 0 before. And it has the confound you predicted, I shipped the moved path and the rewritten content together, so the delta doesn't attribute. Not enough to defend the in-wheel bet. So:
On version matching, there's something to steal from hf itself: the hf-cli skill is generated from the installed hf version, and the CLI warns when the installed copy is stale. A version marker in the synced skill plus hf skills update gives version matching without freezing bytes in a wheel. |
.agents/skills convention and drop trl skills installtrl skills install and move the skill source out of the package
dc435cd to
6f41070
Compare
6f41070 to
c73c246
Compare
|
merging without review, feel free to share thought after though |
|
"Without review"? Two rounds on the destination path, and the PR changed shape because of them. What was actually missing is the approval click; I should have come back and approved once you trimmed it. Flagging it because it's a measurement gap worth fixing: if the board scores "reviewed" by approval state, the PRs where review did the most work are the ones that look neglected. |
|
Yes "approval review"* |

Steps 1–2 of #6743.
trl/.agents/skills/trl/SKILL.md, the convention consumed by library-skills and already shipped by Typer, FastAPI, and Streamlit. Verified the wheel ships it at the new path.trl skills installand thetrl/skills/package.Note
Medium Risk
Removes a public CLI surface and Python API (
trl.skills); users who relied ontrl skills installmust adopt another skill distribution path. Core training CLI is unchanged.Overview
Removes the
trl skillssubcommand and the entiretrl/skillsPython package (install/list/uninstall APIs, agent path resolution, and ~800 lines of tests). Skill content is no longer bundled or installed via the TRL CLI; it lives at repo-rootskills/trl-training/SKILL.mdinstead of inside the wheel.Packaging and lint config drop references to packaged skill markdown (
MANIFEST.in, Ruff per-file ignore). The huggingface/skills sync workflow copies fromskills/trl-training/, ensures the target directory exists, and usesgit status --porcelain(notgit diff) to decide whether to open a PR.Reviewed by Cursor Bugbot for commit c73c246. Bugbot is set up for automated code reviews on this repo. Configure here.