Skip to content

ci(pins): make the python-classifier check advisory, not fatal - #9466

Open
lstein wants to merge 2 commits into
invoke-ai:mainfrom
lstein:ci/pins-classifier-advisory
Open

ci(pins): make the python-classifier check advisory, not fatal#9466
lstein wants to merge 2 commits into
invoke-ai:mainfrom
lstein:ci/pins-classifier-advisory

Conversation

@lstein

@lstein lstein commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to the review comment on #9351: the pin check treated project.classifiers as an authoritative allowlist, which is wrong — classifiers are optional in PEP 621 and purely informational on PyPI, so they cannot decide whether a pin is installable.

@JPPhoto's repro, on #9351 as it stands:

$ jq '.python = "3.11"' pins.json | sponge pins.json   # pyproject.toml untouched
$ python3 scripts/check_pins.py; echo "exit: $?"
pins.json is out of sync with pyproject.toml:
  - pins.json python is '3.11' but pyproject.toml's classifiers declare support only for 3.12; ...
exit: 1

requires-python = ">=3.11, <3.13" genuinely permits that pin. The metadata allows the install; only a piece of documentation disagreed.

After this PR:

$ python3 scripts/check_pins.py; echo "exit: $?"
warning: pins.json python is '3.11', which pyproject.toml's classifiers do not mention
(they list 3.12). That is allowed - classifiers are informational, and requires-python is
what gates installation - but consider adding 'Programming Language :: Python :: 3.11' so
the published metadata matches.
pins.json is consistent with pyproject.toml
exit: 0

What changed

  • requires-python is once again the only authority the script fails on for the python pin.
  • A pin the classifiers don't mention is a warning naming the classifier to add.
  • Absent, malformed, or project.dynamic classifiers are no longer a finding at all — classifiers is optional, so its absence says nothing about the pin.
  • The advisory still prints on a failing run: a wrong pin and an unmentioned pin are usually the same edit.

What this gives up, deliberately

A version that satisfies an open-ended requires-python but that no interpreter has — >=3.11 with a 3.99 pin — is no longer caught. The module docstring says so explicitly, and there's a test named after it.

Catching that case was the reason the classifier allowlist went in, but the price was rejecting legal pins, and a checker that blocks a correct change is worse than one that misses an implausible typo. The pin still has to be major.minor with no patch component, and it still has to satisfy requires-python — with the repo's current >=3.11, <3.13, there is no reachable gap.

Testing

116 tests in tests/test_check_pins.py, all green; ruff format/check clean; script verified under a bare python3 and parsed against the 3.11 grammar (CI runs it with whatever python3 the runner has).

New tests: a 3.11 pin passes with only an advisory; the advisory names the classifier to add; a pin the classifiers do mention says nothing; missing / malformed / dynamic classifiers are not findings; a 3.012 classifier does not count as declaring 3.12; and the advisory survives a failing run.

Mutation-tested the new logic — every mutant applied to the advisory path and the warning loop is caught, including the two that survived my first pass at this change.

@lstein
lstein requested a review from blessedcoolant as a code owner August 5, 2026 16:50
@github-actions github-actions Bot added CI-CD Continuous integration / Continuous delivery Root python-tests PRs that change python tests docs PRs that change docs python-deps PRs that change python dependencies labels Aug 5, 2026
lstein added 2 commits August 5, 2026 12:53
The pin check gated on `project.classifiers`, which is optional in PEP 621
and purely informational on PyPI. That made a legal pin fail: with
`requires-python = ">=3.11, <3.13"`, setting pins.json's python to 3.11
exited 1 solely because no `Programming Language :: Python :: 3.11`
classifier existed, even though the package metadata permits 3.11.

`requires-python` is what actually gates installation, so it goes back to
being the only authority the script fails on. A pin the classifiers don't
mention is now a warning naming the classifier to add, and their absence,
malformation or deferral to `project.dynamic` is not a finding at all.

This gives up one thing, and the docstring now says so: a version that
satisfies an open-ended `requires-python` but that no interpreter has
(">=3.11" with a "3.99" pin) is no longer caught. Catching it meant
gating on non-normative metadata, and a checker that rejects a legal pin
is worse than one that misses an implausible typo.
Found by attacking the previous commit:

- '... :: Python :: 3.012' counted as declaring 3.12, because the
  comparison normalized both sides through _parse_version. That is not a
  trove classifier, so PyPI shows no 3.12 support - silencing the
  advisory in exactly the case it exists to name. The classifier pattern
  now rejects leading zeros like the pin pattern does, which also makes
  the normalization redundant: with one spelling per version on either
  side, a string comparison is exact.
- The warning loop ran before the errors were printed, so a raise while
  computing advice would have discarded the error list - the failure mode
  check_python's "never raises" contract exists to prevent. It now runs
  after, and a test asserts the advisory still appears on a failing run.
- Three comments still described classifiers as authoritative or named
  check_python as the code that reads them.
@lstein
lstein force-pushed the ci/pins-classifier-advisory branch from e40919b to fb0b70e Compare August 5, 2026 16:53
@lstein

lstein commented Aug 5, 2026

Copy link
Copy Markdown
Collaborator Author

#9351 merged while this was in review, so I rebased — the diff is now just this PR's two commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI-CD Continuous integration / Continuous delivery docs PRs that change docs python-deps PRs that change python dependencies python-tests PRs that change python tests Root

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant