fix(presets): enforce that the served repo is a variant of the requested base - #4233
Open
vaibhav8a wants to merge 1 commit into
Open
fix(presets): enforce that the served repo is a variant of the requested base#4233vaibhav8a wants to merge 1 commit into
vaibhav8a wants to merge 1 commit into
Conversation
…ted base A preset declaring `base: X` was verified against two things that a substitution preserves: that the service advertises the requested name, and that the report echoes the requested base. Neither looks at the repo the agent actually served. So `vllm serve Qwen/Qwen3.5-27B-GPTQ-Int4 --served-model-name Qwen/Qwen3.8-27B` answered a request for Qwen3.8 with a different model generation, verified clean, and was reported successful. `base` is documented as "the base model for which the agent may select a compatible variant", so verification now checks compatibility: the served repo must be the base, or the base plus a suffix at a separator boundary. A different generation is not a variant however similar the name. The comparison is on the model name alone, ignoring the owner. A quantisation is routinely published by someone other than the model's author -- this repository's own fixtures pair a Qwen/Qwen3.5-27B base with a community/Qwen3.5-27B-GPTQ-Int4 repo -- so comparing owners would reject the ordinary case and cost `base` the freedom it exists to grant. It is also case-insensitive, since repo references are. Eight tests: four substitutions that must be rejected, including the reported one and a name that merely starts the same, and four genuine variants that must still pass, including the third-party quantisation and a lowercased reference.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #4197.
The gap
A preset declaring
base: Xwas verified against two things a substitution preserves:verify.py:99-100)verify.py:142-146)Neither looks at the repo the agent actually served. So
vllm serve Qwen/Qwen3.5-27B-GPTQ-Int4 --served-model-name Qwen/Qwen3.8-27Banswers a request for Qwen3.8 with a different model generation, verifies clean, and is reported successful — exactly as reported.The rule, and why this shape
baseis documented as "the base model for which the agent may select a compatible variant", so verification now checks compatibility: the served repo must be the base, or the base plus a suffix at a separator boundary (-,_,.).Two deliberate choices, both of which cut the other way from the obvious implementation:
The owner is ignored. Comparing full
org/namewould have been the simpler rule and it would be wrong — a quantisation is routinely published by someone other than the model's author. This repository's own fixtures already assume that, pairing aQwen/Qwen3.5-27Bbase with acommunity/Qwen3.5-27B-GPTQ-Int4repo. An owner check would reject the ordinary case and costbasethe freedom it exists to grant.The boundary is required.
Qwen3.5-27BacceptsQwen3.5-27B-AWQbut notQwen3.5-27Bx, so a barestartswithcannot let an unrelated longer name through.Comparison is case-insensitive, since repo references are.
community/Qwen3.5-27B-GPTQ-Int4Qwen/Qwen3.5-27BQwen/Qwen3.5-27B-AWQQwen/Qwen3.5-27Bqwen/qwen3.5-27b-gptq-int4Qwen/Qwen3.5-27BQwen/Qwen3.8-27B-GPTQ-Int4Qwen/Qwen3.5-27Bmeta-llama/Llama-3-8BQwen/Qwen3.5-27BQwen/Qwen3.5-27BxQwen/Qwen3.5-27BWhere I would welcome a steer: this is a naming-convention rule, so it is a heuristic about how HuggingFace repos are named rather than a fact about the models. It is deliberately conservative — it will reject a legitimate variant that renames rather than suffixes (say a
-v2republished under a different stem). If you would rather that case be allowed, the rule is one function (_is_variant_of) and easy to loosen. Erring toward rejection seemed right for a check whose absence let a silent substitution pass as verified.The error names both values, matching the style of the dataset and workload checks nearby — "not a variant" alone is not actionable.
Tests
Eight, parametrised into two groups:
-27Bxnear-miss. All four fail onmaster.baserather than fix it.Verified:
pytest src/tests/_internal/cli/— 484 passed, 18 skipped.ruff checkandruff format --checkclean on the touched packages using the pinnedruff==0.12.7frompyproject.toml.