Fix CI: pytest job fails on Python 3.11 (numpy 2.5.2 needs 3.12+) - #22
Merged
Conversation
The pytest job ran a 3.11/3.12 matrix, but requirements.txt pins exact versions (numpy==2.5.2 in particular) that were only ever verified against Python 3.12, the actual local dev environment. numpy 2.5.2 does not publish a 3.11 wheel (it requires Python >=3.12), so the 3.11 leg failed at pip install before a single test ran: ERROR: Could not find a version that satisfies the requirement numpy==2.5.2 ... Requires-Python >=3.12 This is the same class of mistake fixed earlier (pinned versions not matching the target Python version) -- I added the 3.11 leg to the CI matrix without ever actually verifying the pinned requirements install under 3.11. Rather than guess at a second set of pins for 3.11, narrowed the pytest job to Python 3.12 only, matching what has actually been verified and what the README already claims (Tested on Python 3.12). Also fixed a README line from the previous PR that claimed CI covered both Python versions. Verified locally: pytest and ruff both pass on 3.12, matching what CI will now run.
Deeper, pre-existing bug uncovered by the same CI run as the previous commit -- this one is not new, it predates all of this sessions work. Every test file imports its target module as from src.module_name import X (this was already the pattern in the original three test files, before any of my changes). That import requires the repo root to be on sys.path, so that src is resolvable as a namespace package. pytest.ini only set pythonpath = src, which puts src/ itself on sys.path -- that is what lets modules inside src/ import each other by bare name (e.g. taylor_maccoll_solver.py doing from isentropic_relations_solver import ...), but it does nothing for the repo root. Locally this went unnoticed because I always ran tests via python -m pytest, and -m module invocation adds the current working directory to sys.path as a side effect of how Python resolves -m. The bare pytest command -- which is what the README documents and what CI actually runs -- does not get that side effect, and failed immediately before collecting a single test. Reproduced locally via .venv/Scripts/pytest.exe -v (bypassing python -m) to confirm. Fixed by setting pythonpath = . src in pytest.ini, adding the repo root alongside src/ so both import styles resolve regardless of how pytest is invoked. Verified: .venv/Scripts/pytest.exe -v (bare entry point, matching CI exactly) now collects and passes all 24 tests. ruff check . still passes clean.
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
1 participant
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.
The 3.11/3.12 test matrix broke main right after PR #20 merged -- numpy==2.5.2 has no Python 3.11 wheel, so pip install failed before any test ran. I added that matrix without ever verifying the pinned deps actually install on 3.11. Narrowed CI to Python 3.12 only, matching what's actually been verified and what the README already claimed. Also fixed a README line that (incorrectly) said CI covered both versions.