Skip to content

Add CI: check PR-changed server.yml against vllm-project/recipes - #107

Open
wjhrdy wants to merge 6 commits into
mainfrom
ci/recipes-check
Open

wjhrdy wants to merge 6 commits into
mainfrom
ci/recipes-check

Conversation

@wjhrdy

@wjhrdy wjhrdy commented Aug 20, 2026 •

Copy link
Copy Markdown
Contributor

SUMMARY:
Adds this repo's first GitHub Actions workflow (.github/workflows/recipes-check.yml + .github/scripts/check_recipes.py). On every PR touching a server.yml, it diffs only the changed model(s) against upstream vLLM community serving guidance (vllm-project/recipes, via the hosted recipes.vllm.ai API) and posts findings as a job summary + sticky PR comment, linking each matched model to its recipe page as evidence. A high-confidence correctness conflict (reasoning-parser/tool-call-parser/tokenizer-mode/config-format/load-format) fails the check and blocks merge (--fail-on correctness); everything else (missing/extra, performance notes, fuzzy matches) is advisory-only and never fails the build.

This follows a manual, repo-wide review done this session that found a real bug (wrong reasoning-parser on Qwen/Qwen3.6-35B-A3B, fixed in #106/#104) but also produced two false alarms once checked against actual vLLM/harness behavior. The check is deliberately tuned to avoid reproducing that noise on every PR:

  • tool-call-parser: a hardcoded wrong value is the same class of bug as a wrong reasoning-parser (both mis-select an output-parsing implementation), so an explicit conflict renders top-tier and blocks merge, same as reasoning-parser/tokenizer-mode/config-format/load-format. "Missing"/"extra" is still never flagged for it — nm-cicd's OCP harness independently injects it at deploy time, and this repo can't see that harness to know if it applies.
  • enable-auto-tool-choice (a boolean toggle, not a parser selection): same missing/extra suppression, but conflicts stay advisory-only, non-blocking.
  • kv-cache-dtype: always a performance note, never correctness — vLLM's own docs frame the default as the higher-fidelity choice for standard attention backends.
  • Fuzzy/ambiguous recipe matches are shown separately and never drive the main findings or the block-merge gate.
  • "Expected divergence" flags (max-model-len, tensor-parallel-size, chat-template, trust-remote-code, ...) are filtered out entirely.
  • Network failures and "no upstream recipe for this model" are treated as normal, non-error outcomes, and never fail the check.

TEST PLAN:

  • Local dry runs against 6 scenarios: real bug present (flags reasoning-parser diff prominently), bug fixed (no top finding), common/-only change (graceful skip message), unrelated file change (graceful no-op), unmatched model (graceful "no recipe found"), vendored argv_to_config unit-sanity check against upstream's documented example — all pass.
  • This PR doesn't itself touch a server.yml, so the pull_request path filter doesn't fire automatically here. Confirmed end-to-end on a real pull_request event (not just workflow_dispatch) via throwaway PR [DEMO, do not merge] Evidence: tool-call-parser top-tier + recipe links #109: deliberately reintroduced reasoning-parser: deepseek_r1 + tool-call-parser: hermes on Qwen/Qwen3.6-35B-A3B, confirmed both rendered top-tier with a working recipe-page link in the posted PR comment (evidence).
  • Confirmed --fail-on correctness actually blocks merge: reintroducing the reasoning-parser bug on [DEMO, do not merge] Evidence: tool-call-parser top-tier + recipe links #109 made the check-recipes status fail (run); reverting to the correct value made it pass (run). Note: this only actually blocks a merge button if check-recipes is added as a required status check under branch protection — that's a separate, repo-wide setting not touched by this PR.
  • [DEMO, do not merge] Evidence: tool-call-parser top-tier + recipe links #109 stays open only as a running evidence log (its base predates the real fix, so it must never merge).

wjhrdy added 3 commits August 20, 2026 16:38
Adds this repo's first GitHub Actions workflow. On every PR touching a
server.yml, diffs only the changed model(s) against upstream vLLM
community serving guidance (vllm-project/recipes, via the hosted
recipes.vllm.ai API) and posts findings as a job summary + sticky PR
comment. Advisory only -- always exits 0.

Deliberately conservative, based on lessons from a manual full-repo
review done this session (see the reasoning-parser fix in #106):

- tool-call-parser/enable-auto-tool-choice are never flagged as
  missing/extra -- nm-cicd's OCP harness independently injects these
  at deploy time regardless of server.yml, and this repo can't see
  that harness to know if it applies. Only an explicit value conflict
  is surfaced, and only as a secondary note.
- kv-cache-dtype is always a performance note, never correctness --
  vLLM's own docs frame the default as the higher-fidelity choice for
  standard attention backends.
- A reasoning-parser conflict (both sides set, disagreeing) is the
  most prominent finding -- nothing downstream overrides it, and it's
  exactly the bug class this check exists to catch.
- Fuzzy/ambiguous recipe matches are shown separately and never drive
  the main findings.
- Expected-divergence flags (max-model-len, tensor-parallel-size,
  chat-template, trust-remote-code, ...) are filtered out entirely.
- Network failures and "no upstream recipe" are treated as normal,
  non-error outcomes.
…g-parser

A hardcoded wrong tool-call-parser is the same class of bug as a wrong
reasoning-parser -- both mis-select a model-specific output-parsing
implementation, and neither is compensated for once the value is
explicitly (wrongly) set in server.yml. Missing/extra is still
suppressed (the OCP harness may legitimately inject it), but an
explicit value conflict now renders top-tier instead of buried in the
advisory details.
A reasoning-parser/tool-call-parser/tokenizer-mode/config-format/
load-format conflict is now a blocking failure, not just an advisory
comment -- this is exactly the class of bug the check exists to catch,
and the whole point of adding it was to prevent a repeat of the
Qwen3.6-35B-A3B reasoning-parser bug from merging silently again.
Everything else (missing/extra, performance notes, fuzzy matches)
still never fails the build.
wjhrdy added a commit that referenced this pull request Aug 20, 2026
wjhrdy added a commit that referenced this pull request Aug 20, 2026
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.

1 participant