ci(pytorch): pin numpy per Python version to mirror upstream test CI#6400
ci(pytorch): pin numpy per Python version to mirror upstream test CI#6400lucbruni-amd wants to merge 2 commits into
Conversation
numpy was unpinned in external-builds/pytorch/requirements-test.txt (bare numpy since the file was created in #903), so CI installed the newest release. A numpy release after 2.4.6 tightened negative-int -> uint8 casting to raise OverflowError, breaking upstream PyTorch's nn.functional.threshold OpInfo reference (np.where(...).astype(uint8) with value=-9). This surfaced as 4 failing test_reference_numerics_*_nn_functional_threshold_cuda_uint8 tests across gfx90a/gfx94X/gfx950/gfx1151/gfx110X/gfx120X, py3.12-3.14, torch 2.9-2.11. Upstream PyTorch never hits this because its test CI pins numpy per Python version. These entries are copied verbatim from pytorch/pytorch .ci/docker/requirements-ci.txt, so we test against the exact numpy versions upstream validates against. Test-environment only; does not touch shipped wheel metadata. Fixes #6027 Fixes #6174
✅ All Checks Passed — Ready for Review
📖 Need help? See the Policy FAQ for details on every check and how to fix failures. |
|
🎉 All checks passed! This PR is ready for review. |
|
can we have a bit more discussion here @ethanwee1 @jithunnair-amd @pragupta if this is the correct way to do it? @lucbruni-amd can you please confirm that this is only for torch versions <= 2.11 and in newer versions it is fixed? for me this would look more like that upstream should fix the tests to be able to handle the new behavior of numpy correctly. |
There was a problem hiding this comment.
Pull request overview
Pins NumPy versions in the PyTorch test-only requirements to mirror upstream PyTorch CI, preventing recent NumPy behavior changes from breaking PyTorch reference-numerics tests (notably uint8 casting in nn.functional.threshold references).
Changes:
- Replace the unpinned
numpydependency with Python-version-specific NumPy pins for the PyTorch test environment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You are right, it's not bound to <= 2.11: Upstream ROCm (The links were taking a while to open on my browser due to the size of these files) Updated the PR description to reflect that.
That is true - this PR only touches the test-env side of things, which mirrors what upstream is doing. I'm happy to discuss further and open a PR upstream instead if that is preferred. |
|
Filed pytorch/pytorch#189267 to get the ball rolling on your suggestion, @HereThereBeDragons. |
|
I agree with @HereThereBeDragons These pins are copied into a TheRock-owned file (external-builds/pytorch/requirements-test.txt) rather than upstream's requirements-ci.txt, so there's no mechanism keeping them in sync so this will silently drift as upstream bumps versions. Could you add a short comment above the block noting the upstream source path (pytorch/pytorch .ci/docker/requirements-ci.txt) and the commit/branch it was copied from? That gives a future maintainer a clear pointer for when to re-sync or drop the pin (e.g. once the OpInfo reference is fixed upstream via pytorch/pytorch#189267). |
Add a provenance comment above the numpy block pointing to pytorch/pytorch .ci/docker/requirements-ci.txt @ 6a13e44 and pytorch/pytorch#189267, per PR review.
Done. This is now a matter of whether we want to pin (and if so, how to handle the drift - I wonder if we can derive such pins from the |
|
i think we could pin it for now, but the idea should be not to pin it in the future. just needs too much maintenance - and also a bit unrealistic to just test a single numpy version that does not have your breaking changes in it. @ethanwee1 is getting this upstream something your team can take care of it? i would feel more confident that it is not forgotten if we have someone assign to it. |
|
Thanks @HereThereBeDragons - yes, my team can take the upstream fix. I'm taking it up. I've filed a draft PR upstream that fixes the OpInfo reference itself (pre-casting the fill value into the input dtype so Plan: land the upstream fix, then drop the interim numpy pin here once it propagates. Tracking this on my board so it doesn't get lost. I think this PR is fine to merge as the interim unblock in the meantime. |
|
Opened a draft #6423 which implements the alternative solution of deriving the pins automatically, preventing drift. Would we prefer something like that instead of merging this PR? EDIT: This PR and the draft are possibly superseded by the mentioned PR below. |
|
#6384 is trying to solve the same issue (and some more) by installing pytorch's requirements-ci.txt file. This step is also done in the |
Motivation
Fixes #6027
Fixes #6174
numpywas unpinned inexternal-builds/pytorch/requirements-test.txt(barenumpysince the file was created in #903), so CI installed the newest release.A numpy release after 2.4.6 tightened negative-int →
uint8casting to raiseOverflowError, breaking upstream PyTorch'snn.functional.thresholdOpInforeference (
np.where(...).astype(uint8)withvalue=-9). This surfaced as 4failing
test_reference_numerics_*_nn_functional_threshold_cuda_uint8testsacross gfx90a / gfx94X / gfx950 / gfx1151 / gfx110X / gfx120X, py3.12–3.14.
Torch version binding: the failure was observed on the branches in the
failing matrix (2.9–2.11), but it is not specific to those versions and is
not fixed in newer torch. The same OpInfo (
value: -9cast touint8) isstill present unchanged on both the upstream
nightlyandrelease/2.12(pinned to current HEAD):pytorch/pytorch@566bb97:common_methods_invocations.py#L18461-L18475ROCm/pytorch@7a68a17(release/2.12):common_methods_invocations.py#L17628-L17642Any torch version hits it given a new-enough numpy.
Technical Details
Upstream PyTorch never hits this because its test CI pins numpy per Python
version. Pin numpy the same way, copied verbatim from
pytorch/pytorch.ci/docker/requirements-ci.txt(still pinned on trunk), so we test against theexact numpy versions upstream validates against. Test-environment only — does not
touch shipped wheel metadata (the wheel's
pyprojectleaves numpy unpinned, soend users still get the latest).
Test Plan
The 4
test_reference_numerics_*_nn_functional_threshold_cuda_uint8tests shouldpass in the full PyTorch test workflow across the affected arches / Python
versions.
Test Result
Pending CI.
Submission Checklist