ci: keep pins.json in sync with pyproject.toml - #9351
Conversation
pins.json is consumed only by the Invoke Launcher (fetched at the release tag) to pick the torch wheel index for legacy installs. Nothing in-repo references it, so it silently drifted from pyproject.toml: its Linux ROCm index stayed at rocm6.3 after the torch-rocm index moved to rocm7.1, causing the launcher to install ROCm 6.3 wheels (invoke-ai#9328, launcher#131). - Add scripts/check_pins.py, which fails if any torchIndexUrl entry in pins.json differs from the matching [[tool.uv.index]] URL in pyproject.toml. - Run it from the uv-lock-checks workflow, triggered by changes to any of pyproject.toml, uv.lock, pins.json, or the check script. - Bump pins.json rocm index to rocm7.1 (matches invoke-ai#9337) so the check passes. - Bump the workflow's uv from 0.6.10 to 0.11.28: uv.lock is already lock format revision 3, which 0.6.x cannot parse. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
torch 2.12.x+rocm7.1 breaks generation (invoke-ai#9328; 2.11 works, and 2.10 is what the rocm extra pins). The cap only matters for legacy (pre-6.14) launcher installs, which resolve the base range live against the pytorch wheel indexes instead of using uv.lock — with the range open to <3.0, AMD users could land on the broken 2.12.x. Lockfile-based installs are unaffected: uv.lock stays at 2.7.1 / 2.10.0+rocm7.1 (no locked versions changed, only the recorded specifier). Remove the cap once the 2.12 ROCm incompatibility is diagnosed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
JPPhoto
left a comment
There was a problem hiding this comment.
invokeai/scripts/check_pins.py:32-43: The consistency check only validates entries that already exist inpins.json; it does not require every launcher-supported platform/backend entry to be present. RemovingtorchIndexUrl.linux.rocmstill printspins.json is consistent with pyproject.tomland exits 0. The launcher's schema (mainover in the launcher repo) permits backend keys to be absent, and its legacy install path only adds--indexwhen the selected entry exists. An omitted ROCm entry therefore silently falls back from the ROCm wheel index, causing AMD installations to resolve an unsuitable build or fail. This recreates the exact installation failure the new guardrail is intended to prevent. To expose this issue, add a test that removes each required CPU, CUDA, or ROCm entry in turn and asserts that the checker exits nonzero; the implementation should validate an explicit required platform/backend matrix, not merely iterate existing entries.
Resolved uv.lock conflict by regenerating from main's lock with `uv lock` rather than merging hunks. The only substantive change is the recorded `torch` specifier (>=2.7.0,<2.12); no package versions moved.
Addresses JPPhoto's review. The checker only iterated the entries that already existed in pins.json, so deleting torchIndexUrl.linux.rocm still exited 0 — recreating the exact silent-fallback failure the guardrail was meant to prevent (the launcher only passes --index when the entry exists, and its schema makes every backend key optional). - Validate pins.json against REQUIRED_BACKENDS, an explicit matrix of the platform/backend combinations the launcher can select and Invoke supports. Missing entries, missing platform sections, unknown platforms and unsupported backends are now all errors, alongside the existing URL check. - Split the logic into check_pins(), with main() taking a repo root, so the exit code is directly testable. - Add tests/test_check_pins.py: removes each required entry in turn and asserts a nonzero exit, plus stale-URL, unknown-platform and unsupported-backend cases. - pins.json: add the win32.cpu entry the matrix exposes as missing. Without it, a Windows "no dedicated GPU" legacy install gets no --index and pulls the CUDA-bundled PyPI torch wheel instead of the +cpu build. Linux already had this entry. - python-tests: trigger on scripts/** and pins.json so these tests actually run when the checker or the pins change. - Revert the uv 0.6.10 -> 0.11.28 bump in uv-lock-checks: 0.6.10 parses the revision-3 lock fine (verified), so the stated justification was wrong and the bump left this workflow out of step with python-tests.
|
Good catch — you're right, and the failure mode was exactly as you described: Explicit required matrix. REQUIRED_BACKENDS: dict[str, set[str]] = {
"win32": {"cpu", "cuda"},
"linux": {"cpu", "cuda", "rocm"},
"darwin": set(),
}It's an exact match in both directions, so four classes of drift now fail:
I derived the matrix from the launcher rather than guessing: Tests. New Also added One consequence worth flagging: the new matrix immediately caught a second missing cell, Unrelated to your review, two other changes in this push:
|
JPPhoto
left a comment
There was a problem hiding this comment.
One more thing found:
invokeai/pyproject.toml:52-54: The reported incompatibility is specificallytorch 2.12.x+rocm7.1, but the newsys_platform != 'darwin'upper bound is emitted into package metadata for every Linux and Windows installation, including CUDA, CPU-only, and Linux ARM64 environments that do not use ROCm. Direct/manual installs in those environments can therefore reject an otherwise requiredtorch>=2.12, downgrade an existing Torch installation, or fail dependency resolution; this also contradicts the PR's claim that the cap only affects legacy launcher installs. Test: resolve and smoke-test InvokeAI with Torch 2.12 on representative Windows CUDA, Windows CPU, Linux CUDA, Linux CPU, and Linux ARM64 environments, and either scope the cap to the affected legacy ROCm resolution path or document evidence that Torch 2.12 is incompatible on every capped platform.
… docs Addresses JPPhoto's second review. The cap was applied to the base `sys_platform != 'darwin'` requirement, so it landed in published package metadata for every Linux and Windows install — CUDA, CPU-only and Linux ARM64 included — despite invoke-ai#9328 only ever reporting a ROCm 7.1 failure. My original justification ("only affects legacy pre-6.14 launcher installs") was wrong in both directions: - It never protected legacy launcher installs. Those install an already-released version, resolving against that release's frozen PyPI metadata, not this pyproject. - It never protected bootstrap (6.14+) launcher installs either. Those pass `invokeai[cpu|cuda|rocm]`, and every one of those extras pins torch exactly, so the base range is never consulted. - What it did affect is manual installs, which per docs/start-here/manual use `--torch-backend=<x>` with no extra and therefore resolve the base range directly — on every platform, not just ROCm. There is no environment marker for "resolving against the rocm7.1 index", so the cap cannot be scoped in metadata. Constrain it where it can actually be targeted instead: the `rocm` extra already pins torch==2.10.0+rocm7.1, and the manual install docs now carry a ROCm-specific caution with the `"torch<2.12"` workaround for users following the `--torch-backend=rocm7.1` path. pyproject.toml returns to `torch>=2.7.0,<3.0`, so uv.lock is untouched by this PR again.
|
You're right, and it's worse than a too-broad marker — the cap wasn't protecting anything. Removed at 3a072fe. I went and checked the launcher source rather than defending my own description, and the justification I wrote ("the cap only affects legacy pre-6.14 launcher installs") was wrong in both directions:
So the cap imposed the cost you identified and bought nothing in return. On scoping rather than removing: there's no environment marker that expresses "resolving against the rocm7.1 index", so the constraint genuinely can't be scoped in package metadata. I put it where it can be targeted — a caution in the manual install docs, on the I didn't run the torch 2.12 smoke-test matrix you suggested — with the cap gone there's nothing left to justify, since we're back to The PR is now purely the |
JPPhoto
left a comment
There was a problem hiding this comment.
A few housekeeping things:
-
docs/src/content/docs/start-here/manual.mdx:135-140: The caution says to retain the workaround "until that is resolved" but links to issue#9328, which is already closed as completed and primarily tracked the stale ROCm 6.3 launcher index. The separate Torch 2.12 generation failure therefore has no open tracking or removal condition, so readers may interpret the closed issue as evidence that the workaround is obsolete. Test: follow the rendered documentation link and verify it targets an open issue dedicated to the Torch 2.12 plus ROCm 7.1 failure, with reproduction and resolution criteria; otherwise create that issue and update the link. -
docs/src/content/docs/start-here/manual.mdx:142: The statement that every other backend "work[s] fine with torch 2.12" is stronger than the available evidence. The linked report establishes only a ROCm failure, and the PR discussion explicitly says the proposed CUDA, CPU, Windows, and ARM64 smoke-test matrix was not run. This leaves a user-facing cross-platform compatibility guarantee unsupported. Test: perform generation smoke tests with Torch 2.12 on Windows CUDA, Windows CPU, Linux CUDA, Linux CPU, Linux ARM64, and macOS MPS, or narrow the wording to say only that the documented workaround applies to the reported ROCm case.
invoke-ai#9328 was closed as completed once pins.json was corrected to the rocm7.1 index; the separate torch 2.12.x generation failure reported in that thread had no open tracking issue, so the caveat had no removal condition. Filed invoke-ai#9410 for it and link there instead. Also narrow the cross-platform claim: we have no torch 2.12 smoke tests on CUDA/CPU/Windows/ARM64/MPS, so say only that the workaround applies to the reported ROCm case rather than asserting other backends are unaffected.
|
Both housekeeping items addressed in 1520cd1. 1. Stale/closed issue link. You're right — #9328 was closed as completed once 2. Overreaching compatibility claim. Agreed, and I didn't run that matrix. The line now reads:
That states the workaround's scope without asserting anything about torch 2.12's behaviour on backends we haven't tested. |
|
Could we just… not have pins.json anymore? All it contains is PyPI index URLs, but pyproject.toml's |
JPPhoto
left a comment
There was a problem hiding this comment.
-
invokeai/scripts/check_pins.py:50-95: Checker ignorespins.json's launcher-criticalpythonfield. Changingproject.requires-pythonto>=3.13,<3.14while keeping"python": "3.12"returns no errors. Launcher creates Python 3.12 before installing incompatible package metadata, causing install failure despite green consistency check. Test: removepython, then set it outsideproject.requires-python; both cases must return nonzero. -
docs/src/content/docs/start-here/manual.mdx:142-143: The claim that it "isn't needed" for other backends isn't verified, so I'd leave that clause off. Supporting evidence: linked issue#9410explicitly says CUDA, CPU, Windows, ARM64, and MPS were not tested and makes no claim beyond ROCm. Of course, you could always test on every platform combination out there...
The launcher's zod schema declares `python: z.string()` (required) and passes
pins.python to `uv venv --python` on both the bootstrap and legacy install
paths, before anything is installed. Bumping project.requires-python without
bumping pins.json therefore builds a venv the package metadata then rejects -
and the check passed, because it only looked at torchIndexUrl.
check_pins.py now also checks that pins.json's python satisfies
project.requires-python. It stays stdlib-only (CI runs it with a bare python3),
so the specifier handling is hand-rolled and deliberately narrow: ==, !=, >=,
<=, > and < against dotted numeric versions, zero-padded so 3.12 == 3.12.0.
Anything it cannot evaluate is reported rather than guessed at. Differentially
tested against packaging over 1014 version/operator/bound combinations with no
divergences.
Hardening found while attacking the above:
- check_python never raises. requires-python is optional in PEP 621 and can be
mistyped as a bare TOML float; either used to throw out of check_pins(),
which runs before the torchIndexUrl loop and so suppressed the ROCm drift
check this script exists for.
- A present-but-null platform section ("linux": null) passed silently, because
.get() cannot tell it from an absent one.
- Version components are bounded, so a pin like 3.12.99999999 - which satisfies
every specifier but which uv cannot resolve to an interpreter - is rejected.
- A trailing comma in requires-python is no longer read as an empty clause.
Also drop the unverified 'isn't needed for the other torch backends' clause
from the ROCm caveat, per review: nothing on CUDA/CPU/Windows/ARM64/MPS was
smoke-tested with torch 2.12.
|
Both addressed in 402e4ba. 1.
The script stays stdlib-only, since CI runs it with a bare Things I found while attacking my own patch, all fixed in the same commit — worth calling out since two of them were regressions the new check introduced:
New tests cover: missing 2. Docs clause. Dropped. The sentence is now just:
And no, I'm not going to test every platform combination out there. 🙂 |
|
@keturn I looked at this while working out what the checker needed to cover, and the short answer is "not yet, and not unilaterally from this side." Two things stand in the way:
There's also a sequencing constraint. So I'd say yes, it should eventually go — but it's a launcher-side change first, and until then this guardrail is guarding something that's still load-bearing. Happy to file that as a follow-up if it's worth tracking. |
|
I was questioning whether we were at the point where updating launcher's approach would be a better investment than adding this layer to keep the moving parts in sync. Now I see that launcher has been making steps in this direction, which is encouraging. Frankly, I wasn't prepared for this part:
Installing something off of PyPI is entirely blind to not only the pyproject.toml's |
JPPhoto
left a comment
There was a problem hiding this comment.
-
invokeai/scripts/check_pins.py:57-64,140-145: Accepts bogus versions such as3.12.9999; checker passes, but launcher cannot create that interpreter and installation fails. Test: setpins.jsonpython to3.12.9999;python3 scripts/check_pins.pyreturns 0 whileUV_CACHE_DIR=/tmp/uvcheck uv python find 3.12.9999fails. -
invokeai/scripts/check_pins.py:24: Claimspython scripts/check_pins.pyworks from anywhere, but the relative path only works from repository root. Test: runcd /tmp && python scripts/check_pins.py; Python reports the file does not exist. Either update the claim or make this work from anywhere.
The python pin was validated with a bounded regex, which made a bogus
version merely unlikely rather than impossible: "3.12.9999" passed, and
`uv python find 3.12.9999` finds no interpreter, so CI stayed green
while the launcher's install broke at the last step.
Replace the heuristic with two checks that make the bad values
unrepresentable:
- The pin must be exactly major.minor. `uv venv --python 3.12` already
resolves to the newest 3.12.x, `--python 3.12.7` demands one exact
build that can leave uv's index, and the launcher's reinstall check
compares only major()/minor() of this field anyway. That removes the
whole 3.12.<anything> family by shape.
- Its major.minor must appear in project.classifiers. requires-python
says which versions the metadata allows; the classifiers say which
ones we ship for. Without it, an unreal version inside an open-ended
requires-python (">=3.11" and a "3.99" pin) satisfies every clause and
no digit bound catches it.
Also fix the module docstring: only the path handed to python is
location-independent, not the working directory it is typed in.
Hardening found by attacking the above:
- Bound the classifier pattern's digits like its two siblings. int()
refuses a string of more than 4300 digits, so an unbounded pattern let
a classifier raise straight out of check_python - which runs first and
would have taken the torchIndexUrl checks with it.
- Guard the pins.json top level. A non-object raised AttributeError from
pins.get(), with the same suppressing effect (pre-existing).
- Reject leading zeros, so the pin has exactly one spelling.
- Report dynamic classifiers as dynamic rather than as absent.
- De-duplicate classifier versions so a repeat is not listed twice.
Tests assert the reason a patch pin is rejected, not just the exit code:
the classifier check rejects those too, so without that a loosened
version pattern would leave the suite green while advising the user to
add a classifier for "3.12.7".
|
Both addressed in bf690d1. 1. Bogus versions. Confirmed —
Your repro now: and with One consequence worth flagging since it's a policy call, not just a bug fix: bumping the pin now requires the matching classifier. Today Missing classifiers are an error rather than a skip, for the same reason a missing 2. Docstring. You're right, only the
and there's a test that runs the script as a subprocess from a New tests: Things I found attacking the above, all in the same commit:
Mutation-tested: 12 mutants across the new checks, all killed. |
JPPhoto
left a comment
There was a problem hiding this comment.
This is approved!
For a follow-up PR:
invokeai/scripts/check_pins.py:200-220: Treats optional, non-normative Python classifiers as an authoritative allowlist. A validrequires-python = ">=3.11, <3.13"withpins.jsonset to3.11fails unless a classifier is also added, even though package metadata permits 3.11. Test: setpins.jsonpython to3.11without changingpyproject.toml; checker exits 1 solely because classifier3.11is absent.
…heck # Conflicts: # .github/workflows/uv-lock-checks.yml
|
Thanks! The conflict in Follow-up opened as #9466. You're right, and the framing helped: I was using a documentation field to answer an installability question. Your exact repro now exits 0: That gives up one thing, and the docstring says so rather than leaving it implicit: a version satisfying an open-ended Two things I found attacking that change, both fixed in #9466: #9466 is stacked on this branch (it needs |
Summary
The launcher installs InvokeAI's dependencies from two sources that nothing keeps in sync:
pins.json(fetched at the release tag, used for the torch index URL in legacy installs) andpyproject.toml/uv.lock.pins.jsondrifted — its Linux ROCm index stayed atrocm6.3afterpyproject.tomlmoved torocm7.1— so launcher installs on AMD got ROCm 6.3 wheels, which cannot work at all on RDNA4 cards (#9328). #9337 fixed the immediate URL; this PR adds the guardrail so it can't silently drift again.Changes
scripts/check_pins.py: validatespins.jsonagainst an explicit required platform/backend matrix (linux: cpu/cuda/rocm,win32: cpu/cuda,darwin: none — MPS has no separate torch index). Missing entries, missing platform sections, unknown platforms, unsupported backends, and URLs that disagree with the matchingtorch-<backend>[[tool.uv.index]]inpyproject.tomlall fail the check. A missing entry matters as much as a stale one: the launcher's schema makes every backend key optional and it only passes--indexwhen the entry exists, so an absent entry silently falls back to the default PyPI index.tests/test_check_pins.py: removes each required entry in turn (and each platform section) and asserts a nonzero exit, plus stale-URL, unknown-platform and unsupported-backend cases. The parametrized cases are generated from the matrix itself so coverage can't silently shrink.pins.json: adds thewin32.cpuentry the new matrix exposes as missing. Without it a Windows "No dedicated GPU" legacy install gets no--indexand pulls the CUDA-bundled PyPI torch wheel rather than the+cpubuild;linux.cpuhas always been there.uv-lock-checksworkflow: runs the new check, triggered by changes topyproject.toml,uv.lock,pins.json, or the check script.python-testsworkflow: path filter extended toscripts/**andpins.json, so editing the checker runs its own tests.--torch-backend=rocm7.1tab, with the"torch<2.12"workaround for [bug]: Release 6.13.5rc1 not installing ROCm 7.1 as expected #9328.On the torch cap (removed)
Earlier revisions of this PR capped the base requirement at
torch<2.12. That has been dropped — it was applied tosys_platform != 'darwin', so it would have landed in published metadata for every Linux and Windows install (CUDA, CPU-only, Linux ARM64) on the strength of a ROCm-only report, and it protected none of the paths I claimed:pyproject.toml.invokeai[cpu|cuda|rocm], and every one of those extras pins torch exactly, so the base range is never consulted.--torch-backend=<x>, no extra) are what actually resolve the base range — on all platforms, not just ROCm.There is no environment marker for "resolving against the rocm7.1 index", so the constraint can't be scoped in metadata; it now lives in the manual install docs instead.
pyproject.toml's specifier is unchanged frommain, souv.lockis not touched by this PR.Caveat
Because the launcher fetches
pins.jsonat the release tag, none of this retroactively fixes already-tagged releases (e.g.v6.13.5.rc1carriesrocm6.3forever). It takes effect from the next tag onward.QA
python3 scripts/check_pins.pypasses on the fixed files.pytest tests/test_check_pins.py— 19 passed.uv lock --lockedpasses;uv.lockis byte-identical tomain.Related: #9328, #9337, invoke-ai/launcher#131
🤖 Generated with Claude Code