Repository navigation
build(musa-image): align public-package pins with the pinned upstream, cap huggingface_hub - #262
Conversation
yeahdongcn
left a comment
There was a problem hiding this comment.
Merge verdict: no blocking findings — ready to merge.
Reviewed at head c6982cfb4b4c955f660ddb50358a3af958016d27 (unchanged from the dispatched SHA; mergeable=MERGEABLE, mergeStateStatus=CLEAN, 3 files, +124/−2). The change does what it claims: the vendored upstream file's unbounded huggingface_hub >= 1.27.0 line, installed with --no-deps, is now intersected with >=1.27.0,<2.0, which resolves to 1.33.0 — the newest release below 2.0 — and 1.33.0 satisfies both transformers 5.5.3 (huggingface-hub<2.0,>=1.5.0) and tokenizers 0.22.2 (<2.0,>=0.16.4). This was auto-review round 1 of 3 and, with a clean verdict, the final round for this pull request: no blocking findings remain and a human can merge, weighing the pending build log noted in P2-3.
P0: none. P1: none.
P2 (nits, non-blocking)
P2-1 — shipped comments carry another PR's image tag, measurement logs and test-authoring history — requirements/constraints.txt:14, tests/test_inplace_porting_contract.py:120, :144, :156-157
requirements/constraints.txt:14 # Measured in the built image (vllm-musa:pr250-final-8b4cbc997):
tests/test_inplace_porting_contract.py:120 Measured in vllm-musa:pr250-final-8b4cbc997; the same image with the cap
tests/test_inplace_porting_contract.py:144 Measured in vllm-musa:pr250-final-8b4cbc997. requirements/constraints.txt
tests/test_inplace_porting_contract.py:156 # flag dropped from either one of them (that is how the first version of this
tests/test_inplace_porting_contract.py:157 # test survived its own mutation).
comment-style.md rules 2–4 (no tracker/PR cross-references, no history narration in shipped artifacts) put an image tag naming PR #250 and a before/after measurement table in the ticket, not in a shipped requirements file or a test docstring. Rule 1 also applies to volume: the same ~25-line rationale appears in three places — requirements/constraints.txt:3-30, docker/musa.Dockerfile:296-300, and the docstrings at tests/test_inplace_porting_contract.py:110-145.
Suggested change: keep one present-tense paragraph in constraints.txt (which distributions declare the <2.0 bound, why --no-deps ignores it, keep the constraint on every such install) and move the pr250-final-8b4cbc997 measurements and the mutation/authoring history to tickets/wip/MUSA-100051-huggingface-hub-uncapped-in-image.md or the PR description.
P2-2 — the cap covers one of several unbounded floors installed by the same --no-deps line — requirements/constraints.txt:31
huggingface_hub>=1.27.0,<2.0
At the pinned upstream revision (third_party/PINS → VLLM_COMMIT=2cf0a6915ce544dc493a0990f2ea38d81601128a), the file this line installs also carries unbounded floors transformers >= 5.5.3 (line 10) and tokenizers >= 0.21.1 (line 12). Today's newest releases are transformers 5.17.0 and tokenizers 0.23.2 — the latter outside the tokenizers<=0.23.0,>=0.22.0 range transformers 5.5.3 declares — while the fork pins only transformers==5.5.3 (requirements/common.txt:6). The precondition that produced this bug is a lagging installed version, and the real build log generated/MUSA-100050/image-build-final-06413f2bc.log shows it in step #20: Collecting huggingface_hub>=1.27.0 (from -r third_party/vllm/requirements/common.txt (line 11)) → Downloading huggingface_hub-2.0.0 → Uninstalling huggingface-hub-1.16.1 → Successfully installed … huggingface_hub-2.0.0. transformers 5.5.3 and tokenizers 0.22.2 escaped the same jump only because they already met their floors (Requirement already satisfied: huggingface-hub<2.0,>=1.5.0 … (1.16.1) at #19 21.09). If a future base or pin leaves either below its floor, that line installs 5.17.0 / 0.23.2 and the self-check will not notice: neither name is in the expected version tuple (docker/musa.Dockerfile:333-355), only importability is asserted (:370-372).
This is not a defect at this head — the --constraint addition does not change how those two lines currently resolve — which is why it is P2.
Suggested change: add fork-compatible bounds for the same two names to requirements/constraints.txt (e.g. tokenizers>=0.22.0,<=0.23.0, and transformers==5.5.3 if the vendored line is meant as a floor only), or state in one line why hf_hub is the only name in scope; optionally add transformers to expected so the image gates the version it imports.
P2-3 — the from-scratch build of this commit has not landed — docker/musa.Dockerfile:301-309
The PR body is explicit: "A from-scratch rebuild with this change has not landed yet … Until it does, the end-to-end claim is 'the Dockerfile's own install lines + self-check pass inside the image', not 'an image was built from this commit'." I checked the workspace evidence myself: generated/MUSA-100050/hfhub-cap-validation.log is the in-image rerun (unconstrained line → hf_hub 2.0.0, with the cap → 1.33.0, widened self-check → PASS import transformers version=5.5.3, PASS import huggingface_hub version=1.33.0); the only other recent image log, generated/MUSA-100050/image-build-final-06413f2bc.log, is a build of the unfixed Dockerfile whose step #21 still runs for module_name in ("vllm", "vllm_musa"):. generated/MUSA-100051/ contains no build log, and there is no CI anywhere (gh pr checks → no checks reported, and .github does not exist at this head).
Suggested change: post the build result the body promises. The cheapest complete check is docker build --target vllm_musa_installed, which contains both constrained installs and the widened self-check (it still builds the extension, so it is slower than the documented --target vllm_musa_deps shortcut).
Not blocking, because every failure mode of this specific change is loud — a missing or unsatisfiable constraint file fails the pip step, and a still-broken image fails the new import transformers line — and the fixed point is forced by metadata I verified directly.
What I verified
- Prior rounds and checks:
gh pr view 262 --json reviews→reviews: [], so there is no earlier P0/P1 to re-check; head is stillc6982cfb4…;gh pr checks 262→no checks reported on the 'dsh/musa-100051-hfhub-cap' branch, with no.github/in the repo at this head (no in-repo CI to read). - The cap's fixed point, from live PyPI metadata:
transformers 5.5.3requireshuggingface-hub<2.0,>=1.5.0;tokenizers 0.22.2requireshuggingface-hub<2.0,>=0.16.4;huggingface-hub2.0.0 is the newest release and 1.33.0 is the newest<2.0; the vendored floor is>= 1.27.0(fetched at the pinned commit). The intersection is exactly 1.33.0 — the version in the PR's table — and an already-installed 2.0.0 violates the merged specifier, so the same line also repairs an image that already carries it. - Build context for the new flag:
COPY requirements/ /vllm-workspace/requirements/(docker/musa.Dockerfile:246),WORKDIR /vllm-workspace(:247),COPY . /vllm-workspace(:291);.dockerignoredoes not excluderequirements/, and the same relative form is already used on those exact lines, so--constraint requirements/constraints.txtresolves. - No other consumer of the new file:
setup.py:541isinstall_requires=_read_requirements("musa.txt"), which reads explicit filenames plus-rincludes and never globsrequirements/*.txt, so package metadata is unchanged; noPIP_CONSTRAINTis set anywhere, so the cap is opt-in only. - Nothing can re-raise hf_hub after the two constrained installs: the only later pip step installs
mooncake-transfer-engine-musa==0.3.13, whose published dependencies are aiohttp/requests/msgpack/typing_extensions, and thefinal/vllm-openaistages run no pip. - The cap cannot make the transitive resolve unsatisfiable: none of the 26 names in
requirements/vllm_runtime_transitive.txtdeclareshuggingface_hub, and a two-level closure walk (285 distributions, PyPI JSON) found nohuggingfacerequirement at all. - The new guard test behaves as its docstring claims: replaying
_dockerfile_run_commandsplus the per-invocation scan against the head Dockerfile finds 9 pip invocations, exactly one per target, both carrying the flag; dropping either flag reddens the assertion, so the "per pip invocation, not per RUN" claim holds.dockerfile.index("importlib.import_module(module_name)") < dockerfile.index("installed = version(dist_name)")now holds, so the reorder atdocker/musa.Dockerfile:359-360satisfies the pre-existing assertion attests/test_inplace_porting_contract.py:205-208— the same failure the PR body attributes to the pristine base. test_musa_image_caps_huggingface_hub(dockerfile=None)(tests/test_inplace_porting_contract.py:107) is not a missing-fixture bug: pytest excludes parameters that have defaults (p.default is Parameter.emptyingetfuncargnames), checked in the installed pytest 9.0.1 and in the_pytest/compat.pysources of 6.2.5, 7.4.4 and 8.2.0, so the parameter is never resolved as a fixture.- The deferred residuals are correctly out of scope here:
huggingface_hub2.0.0 and 1.33.0 both declareclick<9.0.0,>=8.4.2against the image'sclick==8.2.0(requirements/common.txt:9), so no hf_hub version removes thatpip checkline — a separate pin decision, asrequirements/constraints.txt:24-27says.
What I could not verify
- I ran no build, install or GPU work (read-only session), so I did not reproduce the
ImportErroror thepip checkoutput myself; those are quoted from the two workspace logs named above, which I read in full. My package-metadata checks usedpypi.org; I did not query the internalPYPI_INDEX_URLmirror directly, though the author's in-image log resolving 1.33.0 is consistent with it serving the same releases. - The base-image build log ends mid-
cargooutput, so it demonstrates step#20's pip resolve, not a fully green build of the base commit. - I did not exercise the built image; the claim that the widened self-check aborts an unfixed build rests on the author's log (the same script run against the 2.0.0 image raising the
ImportError), which I read but did not re-run.
auto-review round 1/3 · verdict: clean
Rebuild landed: the fix is now measured end to endThe body's "not yet done" caveat is resolved. Two from-scratch builds with
The second image, pushed as So: the cap resolves Evidence: Still open (unchanged, out of scope here): the |
c6982cf to
cfa9441
Compare
Rewritten after review: the fix is now one line, no new fileBranch force-pushed to RUN python -m pip install \
--no-cache-dir \
--no-deps \
- -r third_party/vllm/requirements/common.txt && \
+ -r third_party/vllm/requirements/common.txt \
+ 'huggingface_hub>=1.27.0,<2.0' && \Why: Re-measured with the new shape, from-scratch The earlier comment's build refers to the superseded revision; the resolved version is the same Attribution, for the record: the unbounded requirement is upstream's line (vendored |
…nstall The pinned upstream file (third_party/vllm/requirements/common.txt) asks for `huggingface_hub >= 1.27.0` with no upper bound. docker/musa.Dockerfile installs that file with --no-deps, so without a constraint that one line resolves to the newest release (2.0.0), while this image pins `transformers==5.5.3`, whose metadata requires `huggingface-hub<2.0` (tokenizers 0.22.2 requires the same). The image then cannot import transformers at all: ImportError: huggingface-hub>=1.5.0,<2.0 is required for a normal functioning of this module, but found huggingface-hub==2.0.0. Measured in vllm-musa:pr250-final-8b4cbc997 and reproduced in a from-scratch build of 06413f2bc; `pip check` in that image lists both violations, and no distribution in it requires huggingface_hub >= 2. Upstream's own install path resolves, so it never sees this; our deliberate --no-deps install (to protect the MUSA torch stack) is what removes the dependency context that would have kept the pair consistent. Fix it where the versions already live: `huggingface_hub>=1.27.0,<2.0` joins transformers==5.5.3 in requirements/common.txt, and the Dockerfile applies that file to the vendored install with --constraint, so the public-package versions stay in requirements/ and the Dockerfile only states the mechanism. pip accepts the constraint and changes nothing else in that file (pre-flight: 'Would install huggingface_hub-1.33.0', every other requirement already satisfied). The image self-check now imports transformers and huggingface_hub as well as vllm/vllm_musa, and imports before it reads the distribution version, so a future mismatch fails the build loudly instead of shipping a broken image (the ordering assertion already existed and failed on pristine 2293791).
cfa9441 to
b090260
Compare
Placement changed again: the version now lives in
|
Restores the shape verified by a from-scratch build and a container run, after the constraint variant (pin in requirements/common.txt, applied with --constraint) turned out not to build: `click==8.2.0` in the same file cannot coexist with the pin, because every `huggingface-hub >= 1.27` requires `click<9.0.0,>=8.4.2` and that conflict is visible to the resolver in the deps stage (pip: 'Cannot install -r requirements/common.txt (line 10) and click==8.2.0' -> ResolutionImpossible). Aligning the fork's public-package pins with the pinned upstream's own resolved set is a separate decision; until then the cap stays on the --no-deps install that resolves the uncapped line, where nothing else moves. third_party/vllm/requirements/common.txt (the pinned upstream file) asks for `huggingface_hub >= 1.27.0` with no upper bound. docker/musa.Dockerfile installs that file with --no-deps, so that one line resolves to the newest release (2.0.0) while this image pins `transformers==5.5.3`, whose metadata requires `huggingface-hub<2.0`: ImportError: huggingface-hub>=1.5.0,<2.0 is required for a normal functioning of this module, but found huggingface-hub==2.0.0. Measured in vllm-musa:pr250-final-8b4cbc997 and reproduced in a from-scratch build of 06413f2bc; `pip check` there lists both violations (transformers and tokenizers), and no distribution in the image requires >= 2. The image self-check now imports transformers and huggingface_hub as well as vllm/vllm_musa, and imports before it reads the distribution version, so a mismatch like this fails the build loudly (that ordering assertion already existed and failed on pristine 2293791).
eff5248 to
334926f
Compare
The
|
| package | fork pin today | upstream v0.28.0's own resolved env | note |
|---|---|---|---|
| click | ==8.2.0 |
8.4.2 |
blocks this cap |
| transformers | ==5.5.3 |
5.15.0 |
makes the GLM-5.3-Flash smoke fail ("model type glm5_next ... update Transformers") |
| numpy | ==1.26 |
2.2.6 |
intentional MUSA divergence (torch_musa) - keep |
| ray | >=2.47.1,<=2.48.0 |
2.48.0 |
aligned |
| openai | >=2.24.0 |
2.44.0 |
compatible |
So the clean home for this cap is what this review asked for - but it needs the click pin
aligned first. That work (plus the transformers question, which needs its own model-smoke
verification) is filed as MUSA-100058; once click is at the value upstream itself resolves,
huggingface_hub>=1.27.0,<2.0 can move into requirements/common.txt and this Dockerfile hunk
becomes a single --constraint flag.
…, cap huggingface_hub
The image's own pin file (requirements/common.txt) had drifted from what the pinned upstream
vLLM v0.28.0 resolves, and both drifts hurt:
- `click==8.2.0` versus upstream's resolved 8.4.2 could not coexist with the
`huggingface_hub >= 1.27.0` that upstream itself requires (every huggingface-hub >= 1.27
needs `click<9.0.0,>=8.4.2`), so the image carried a silent inconsistency, visible only as
`pip check: huggingface-hub ... has requirement click<9.0.0,>=8.4.2, but you have click
8.2.0`.
- `transformers==5.5.3` versus upstream's resolved 5.15.0 made the image unable to load
some checkpoints at all: the GLM-5.3-Flash smoke fails with "model type 'glm5_next' but
Transformers does not recognize this architecture ... update Transformers".
Align both with the versions upstream's own environment resolves
(third_party/vllm/requirements/test/{cpu,cuda,rocm}.txt agree on click 8.4.2 and transformers
5.15.0), pin tokenizers to the same 0.22.2 (upstream only floors it, so a cold resolve takes
0.23.2), and add the missing `huggingface_hub>=1.27.0,<2.0`: the vendored upstream file asks
for `>= 1.27.0` with no upper bound while transformers (5.15.0 as well as 5.5.3) requires
`huggingface-hub<2.0`, and docker/musa.Dockerfile installs that file with --no-deps, so
without a pin the line resolves to 2.0.0 and transformers refuses to import:
ImportError: huggingface-hub>=1.5.0,<2.0 is required for a normal functioning of
this module, but found huggingface-hub==2.0.0.
Because these pins now live in requirements/common.txt, the Dockerfile carries no version for
them: it applies that file to the vendored install with `--constraint requirements/common.txt`.
numpy stays at 1.26 as the one deliberate divergence (torch_musa needs numpy 1.x), and
localises to transformers/huggingface_hub/click/tokenizers otherwise untouched.
The image self-check imports transformers and huggingface_hub as well as vllm/vllm_musa and
imports before reading the distribution version, so a future mismatch fails the build loudly
instead of shipping a broken image (that ordering assertion already failed on pristine
2293791).
Rebuilt end to end with the aligned pins, and verified out of the boxThe full change is now: From-scratch Container from that image, no install step of any kind: The Pushed to |
Smoke matrix on the rebuilt image: same verdict set, no regression
The difference from the earlier round is that this run needed no dependency remediation: the The
|
…and the pin file The rationale belongs in the pull request and the ticket, not in the files: requirements/common.txt returns to its one-line header and plain list, and docker/musa.Dockerfile goes back to having no comment above the vendored install. Comment-only change - verified that the requirement lines pip sees and the Dockerfile instructions are byte-identical to 96448dc, the revision that was built and smoke-tested.
What breaks
The image could not import
transformersat all, and two of its own pins were the reason.docker/musa.Dockerfileinstalls the pinned upstream vLLM's requirements with--no-deps.That file asks for
huggingface_hub >= 1.27.0with no upper bound, so pip resolves thatone line on its own and takes the newest release - 2.0.0 - while
transformers(5.5.3 then,5.15.0 now) requires
huggingface-hub<2.0, as doestokenizers:Measured in
vllm-musa:pr250-final-8b4cbc997and reproduced in a from-scratch build of06413f2bc. Nothing in the image requireshuggingface_hub >= 2; the newer version was puredrift from the missing pin.
One more pin had drifted, and it was load-bearing:
click==8.2.0versus the8.4.2upstreamresolves. Adding the
huggingface_hub>=1.27.0,<2.0pin to the same file makes the deps stagefail outright, because every
huggingface-hub >= 1.27needsclick>=8.4.2:The fix
Align the fork's public-package pins with the versions the pinned upstream vLLM itself
resolves (
third_party/vllm/requirements/test/{cpu,cuda,rocm}.txtagree), keep numpy as theone deliberate divergence, and give the vendored install its missing constraint:
The versions live in the pin file - the Dockerfile states only the mechanism, so our pins stay
authoritative wherever they overlap upstream's requirements, and any future upstream floor that
contradicts one of them fails this install loudly instead of shipping a broken image. The image
self-check now also imports
transformersandhuggingface_hub, and imports before readingthe distribution version (that ordering assertion already failed on pristine
22937910b).tests/test_inplace_porting_contract.py::test_musa_image_caps_huggingface_hubasserts the pinis in
requirements/common.txt, that no such version appears in the Dockerfile, and that thepip invocation consuming the vendored file carries the constraint.
Why
<2.0when upstream itself declares no upper boundCorrect, and worth stating plainly: upstream v0.28.0's line is a floor only.
third_party/vllm/requirements/common.txt:11is exactlyhuggingface_hub >= 1.27.0. Nothing elsein that tree bounds it -
pyproject.tomlhas no huggingface entry at all,setup.pyonly readscommon.txt, and there is no constraints file - and it never has: every historical revision of that
line is lower-bound-only. The four
huggingface-hub==1.27.0pins inrequirements/test/{cpu,cuda,rocm,xpu}.txtare pip-compile lock files for its frozen testenvironments, not a range.
The cap in this PR exists because of how this image installs, not because of a policy of ours.
docker/musa.Dockerfileinstalls the vendored file with--no-deps, so pip evaluateshuggingface_hub >= 1.27.0on its own and takes the newest release - 2.0.0 - whichtransformers==5.15.0(huggingface-hub<2.0,>=1.5.0) andtokenizers==0.22.2(
huggingface-hub>=0.16.4,<2.0) both forbid. The<2.0mirrors their metadata cap; adependency-resolving install would not need it, and without it the image cannot import
transformers - the bug this PR fixes.
For reference, upstream's locked test environments carry the same versions this PR aligns to -
transformers==5.15.0,tokenizers==0.22.2,click==8.4.2,safetensors==0.8.0- withhuggingface-hub==1.27.0. Measured in this image:1.27.0satisfies every declaration there is(nothing requires >= 1.28), and
1.33.0is the newest release under the cap, i.e. what the imageinstalls.
Evidence
From-scratch
docker/build_image.shat headf12eec5be(base22937910b+ this change):exit 0, image
sha256:10351a7d6ef5, 20.9 GB, 171 patches applied, loggenerated/MUSA-100051/image-build-f12eec5be.log. The imageself-check (it imports
vllm/vllm_musa/transformers/huggingface_hub, and reads the versionafter importing) prints:
Container from that image, no install step of any kind:
out-of-box: hub 1.33.0 transformers 5.15.0 vllm 0.28.0 vllm_musa 0.1.24, andpip checkreports only the two pre-existing residuals(
mcp 2.2.0 requires mcp-types, which is not installed.andopencv-python-headless 5.0.0.93 has requirement numpy>=2against the documentednumpy==1.26).The
huggingface-hub/clickviolations this image used to carry are gone.pytest tests/test_inplace_porting_contract.py-> 18 passed.GPU smoke matrix on the rebuilt image, 8 legs, matrix script byte-identical to the previous round
(sha256
cacd7c7a0cef...):pass=5 fail=2 skip=1- the same verdict set as that round - and withno in-container dependency remediation, where earlier rounds had to
pip install 'huggingface-hub>=1.27.0,<2.0'inside the container first. Run loggenerated/MUSA-100058/gpu-rerun/pr258-rerun-gpu-run.log. That run used the registry imageregistry.mthreads.com/mcconline/inference/vllm:pr250-hfhub-96448dc10; the final head differsfrom
96448dc10only in removed comments, and the requirement lines pip sees plus the Dockerfileinstructions with comments stripped are byte-identical between the two revisions, so the build and
smoke results carry over.
One hypothesis tested and disproven (so it is not repeated)
An earlier revision of this PR suggested the stale
transformerspin explained theglm53-mhasmoke failure ("model typeglm5_nextbut Transformers does not recognize thisarchitecture"). The alignment does not fix that leg:
glm5_nextis still not a knownconfig type in transformers 5.15.0 (the version upstream itself resolves) - verified directly:
The alignment stands on the
clickconflict and on upstream consistency; GLM-5.3-Flash supportis a separate gap that no released transformers fixes.
Why not a separate constraints file
This repo replaces that kind of machinery with pins in the requirement files: on 2026-07-30
(
dd670c9e86, "address Docker image review feedback") a dynamically generated/tmp/vllm-musa-constraints.txtwas removed fromdocker/vllm-omni.Dockerfileand replacedwith
-r requirements/test.txt, and the contract test now asserts"vllm-musa-constraints" not in dockerfile. A tracked pin file used as--constraintis notthat pattern.
Out of scope (unchanged, pre-existing)
mcp 2.2.0 requires mcp-types,opencv-python-headless 5.0.0.93 requires numpy>=2vsnumpy==1.26(this one is the numpy divergence, documented above).