Skip to content

feat(recipes): add the qwen3.5-9b-grpo recipe (#277) - #448

Merged
MakazhanAlpamys merged 2 commits into
MakazhanAlpamys:mainfrom
harshitthek:recipe/277/qwen3.5-9b-grpo
Aug 19, 2026
Merged

feat(recipes): add the qwen3.5-9b-grpo recipe (#277)#448
MakazhanAlpamys merged 2 commits into
MakazhanAlpamys:mainfrom
harshitthek:recipe/277/qwen3.5-9b-grpo

Conversation

@harshitthek

@harshitthek harshitthek commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Closes #277

  • Add ready-made Qwen 3.5 9B GRPO reasoning recipe to catalog
  • Add TestIssue277Qwen35GrpoRecipe regression tests (load, show, use)
  • Update total recipe count from 144 to 145 across catalog, docs, and test invariants
  • Update CHANGELOG.md under [Unreleased] Added

What does this PR do?

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Documentation
  • Tests

Checklist

  • ruff check src/soup_cli/ tests/ passes
  • pytest tests/ -v passes
  • Updated relevant docs (README.md and the matching page under docs/) if needed

Copilot AI lite review requested due to automatic review settings August 18, 2026 17:34

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@MakazhanAlpamys

Copy link
Copy Markdown
Owner

Reviewed at 153c3a6d in an isolated worktree with PYTHONPATH pinned to it. I did this one by hand. Two one-line items and it merges — the recipe itself is correct and, more importantly, correctly guarded.

The part I check hardest on recipe PRs, and you got it right

Two earlier recipe PRs here established the standard: a test that pins the literal repo id. The reason is on the record — an id that is wrong consistently in both RecipeMeta.model and the inline base: passes any test that compares the two against each other, and that defect actually shipped once (glm-5 pointing at the wrong org). So I ran that mutation specifically:

mutation result
id changed in RecipeMeta.model only (inconsistent) KILLEDtest_recipe_loads_with_expected_grpo_shape
id changed consistently in both places — the glm-5 shape KILLEDtest_show_and_use_recipe (2 failed)
task: grpo -> task: sft KILLED — 2 failed

The middle row is the one that matters. A test comparing the two fields to each other would have passed it.

End to end, not just loaded:

$ soup recipes use qwen3.5-9b-grpo
✓ Recipe qwen3.5-9b-grpo written to soup.yaml
$ load_config('soup.yaml')
task: grpo | base: Qwen/Qwen3.5-9B | reward_fn: accuracy | num_gen: 4

It parses as a real SoupConfig with a reward function set — a GRPO recipe that renders but fails the GRPO cross-validators would be the interesting failure and this is not it.

Repo id checked against its siblings, since I cannot hit the network: Qwen/Qwen3.5-9B is byte-identical to the id qwen3.5-9b-sft already uses, and the shape matches all ten Qwen3.5 entries in the catalog. Count is really 145 (len(RECIPES)), tests/test_recipes.py 35 passed, and the removed test lines are only the old == 144 assertions being updated — nothing weakened, no test deleted.

Licence-header check: clean. I ran the new repo-wide guard against your branch and it passes. Context, since it may bite you on rebase: a third-party AGPL header has landed here three times via contributor tooling, most recently in a PR I merged myself this morning, so tests/test_no_foreign_license_headers.py now fails the build on it. Yours is clean; you just need to know the check exists.

Two things before merge

1. CONTRIBUTING.md:116 still says 144 recipes. You updated the catalog, both docs pages and three test files — this is the fourth place, and count drift across files is a recurring problem here (I missed CONTRIBUTING.md myself on the v0.73.3 release two days' worth of commits ago, so I am not being clever about it). One number.

2. The CHANGELOG entry has no attribution suffix. It reads (#277); the house format is (#277 by @harshitthek in #448). I ask for this in the PR now rather than reconstructing it at release time, because reconstructing it is exactly when it goes wrong — four suffixes in v0.73.3 turned out wrong or incomplete when I checked them against gh.

Note you will need a rebase either way: I pushed to CONTRIBUTING.md a few minutes ago, so that file has moved under you.

One aside

You took #277 rather than either of the two I pointed you at, which is entirely your call — but do claim in the issue thread next time. #277's thread had zero comments when you opened this, so nothing went wrong here; the reason I keep saying it is that I promised the same issue to two different people earlier today by making offers in PR comments where nobody could see them. The claim convention exists because I am the one who broke it.

- Add ready-made Qwen 3.5 9B GRPO reasoning recipe to catalog
- Add TestIssue277Qwen35GrpoRecipe regression tests (load, show, use)
- Update total recipe count from 144 to 145 across catalog, docs, and test invariants
- Update CHANGELOG.md under [Unreleased] Added
@harshitthek
harshitthek force-pushed the recipe/277/qwen3.5-9b-grpo branch from 153c3a6 to 846f265 Compare August 18, 2026 18:28
@harshitthek

Copy link
Copy Markdown
Contributor Author

Updated both items!

  • Bumped CONTRIBUTING.md:116 to 145 recipes
  • Added attribution suffix (#277 by @harshitthek in #448) to CHANGELOG.md
  • Rebased cleanly onto current main (008da45)

Also checking out #434 as recommended — will drop a comment as soon i look carefully into it

@harshitthek

Copy link
Copy Markdown
Contributor Author

Updated both requested items:

Heads-up on the CI matrix failure:
It comes from upstream commit 008da45 on main. In tests/test_no_foreign_license_headers.py (lines 5–6), the example lines inside the module docstring start with '#' in the first 40 lines. Since the file is tracked in git, _scan_repo() flags its own docstring:

  • tests/test_no_foreign_license_headers.py:5 carries an SPDX identifier other than Apache-2.0
  • tests/test_no_foreign_license_headers.py:6 carries an all-rights-reserved notice

The recipe itself, validate-recipes, lint, type-check, and mlx-smoke are all green!

@MakazhanAlpamys

Copy link
Copy Markdown
Owner

Heads-up before you go chasing this: the red CI on your branch is mine, not yours.

FAILED tests/test_no_foreign_license_headers.py::TestNoForeignLicenseHeaders::test_no_tracked_file_declares_a_foreign_license

I added a repo-wide licence-header guard in 008da45 this evening, and it flagged its own module docstring — the docstring quotes the offending header verbatim as documentation, and the scanner read those indented lines as real comments. It passed locally only because an uncommitted file is absent from git ls-files, so the scan never read itself until it landed.

Fixed on main in 152d6ad (the rule now requires the # at column 0, which is where a real file header sits). Rebase onto current main and those cells should clear. Nothing in your diff caused it, and nothing in your diff needs to change for it.

Sorry for the noise — this is the second time this week I have turned contributor branches red with a guard of my own.

@MakazhanAlpamys

Copy link
Copy Markdown
Owner

I updated your branch with main myself rather than making you do it — the red was entirely my broken licence guard, so clearing it is my job, not yours. It is a merge of main into your branch, not a rebase, so none of your commits were rewritten.

CI is re-running now. Nothing in your diff changed.

@MakazhanAlpamys
MakazhanAlpamys merged commit aff90cf into MakazhanAlpamys:main Aug 19, 2026
13 checks passed
@MakazhanAlpamys

Copy link
Copy Markdown
Owner

Merged as aff90cf. Thank you — and your CONTRIBUTORS.md bullet carries the #449 diagnosis alongside it, as promised when I closed #450.

What I ran, on the branch as it actually merges (I re-ran the mutations after updating your branch with main, so the guard is verified against the merged state rather than the reviewed one):

mutation result
repo id wrong in RecipeMeta.model only KILLED
repo id wrong consistently in both places KILLED — 2 tests, 2 failed, 33 passed
task: grpo -> sft KILLED

The middle row is why recipe PRs get this specific attention here: a test that compares RecipeMeta.model against the inline base: passes a consistently-wrong id, and that is exactly how glm-5 once shipped pointing at the wrong org. Your literal pin catches it.

Both requested items are in: CONTRIBUTING.md:116 at 145, and the attribution suffix on the CHANGELOG entry. End to end, recipes use qwen3.5-9b-grpo writes a soup.yaml that loads as task=grpo with reward_fn=accuracy — a GRPO recipe that renders but fails the GRPO cross-validators would be the interesting failure, and this is not it.

On #449 and #450, since that is the honest footnote to this merge. You diagnosed a bug of mine in half an hour, including the part I had missed, and had a green fix up before I finished writing my own — which I then pushed without checking whether anyone had picked it up. The column-0 approach won on a design argument I can defend, not on quality or timing, and the credit for the find is yours in the bullet and in #449.

#452 (@Osheun) now rebases on top of you to 146. That is the order I said I would set, and it landed that way: you had the CHANGELOG entry and the method renames already, so you were closer.

One thing your PR made me file: #453. When two recipe PRs land in sequence, only the two test files conflict loudly — the five documentation count sites auto-merge and silently keep the old number, because nothing guards them. Your PR is not at fault; it is what let me see the gap.

@harshitthek

Copy link
Copy Markdown
Contributor Author

Thank you @MakazhanAlpamys! Really enjoyed working through the mutation matrix and synchronization on this one. Glad the literal pins and catalog invariants held up so well!
~Harshit ^^

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.

Add ready-made recipe: qwen3.5-9b-grpo (GRPO reasoning variant)

3 participants