fix(ci): Install pytorch pinned requirements in core UT workflow#6384
fix(ci): Install pytorch pinned requirements in core UT workflow#6384zjliu-amd wants to merge 2 commits into
Conversation
✅ 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. |
zjliu-amd
changed the title
zjliu-amd
changed the title
|
similar is done in #6400 |
|
@HereThereBeDragons I'm in favour of this PR since it pulls the other missing test deps (not just |
|
I am in favor of this move. |
|
yeah i think this seems more elegant. but we probably then want some comment in requirements-test.txt that for the ci testing we also use the pytorch requirements.txt and some versions may be overwritten? |
|
This change, however, could lead to a dependency installation failure on high python versions (such as 3.14). An example is https://github.com/ROCm/TheRock/actions/runs/28899244667/job/85731743101 Update: Fixed by bumping dependency version in pytorch |
|
Rather than pin an old numpy in TheRock long-term, we're going to fix this at the source and let TheRock inherit upstream's pins. Two upstream approaches are staged: First, cap numpy in upstream test CI: Cap numpy <2.5 in requirements-ci.txt to avoid strict-cast regression by ethanwee1 · Pull Request #1… adds a <2.5 upper bound to the numpy entries in .ci/docker/requirements-ci.txt (keeping the existing floors), so CI resolves to the 2.4 line (currently 2.4.6), the last release before numpy tightened negative-int to uint8 scalar casting. Least invasive, but since the numpy change was deliberate it's only a stopgap. Second, fix the reference itself: Fix nn.functional.threshold OpInfo reference for strict NumPy uint8 casting by ethanwee1 · Pull Requ… makes the nn.functional.threshold OpInfo test dtype-safe (non-negative fill for uint8, -9 kept for signed/float), so it survives any numpy. An upstream reviewer (rgommers) already suggested this here: nn.functional.threshold OpInfo reference raises OverflowError on unsigned dtypes with NumPy >= 2.5 ·… Once the reference fix (option 2) is validated upstream, we'll drop TheRock's external-builds/pytorch/requirements-test.txt numpy pins entirely so we inherit upstream's requirements-ci.txt versions. |
| - name: Install test requirements | ||
| run: | | ||
| python -m pip install -r external-builds/pytorch/requirements-test.txt | ||
| python -m pip install -r external-builds/pytorch/pytorch/.ci/docker/requirements-ci.txt |
There was a problem hiding this comment.
Note that this external-builds/pytorch/pytorch directory is the default but it is not the path we use when building on Windows. Looks like we do use it here for testing consistently on both platforms though.
| python -m pip install -r external-builds/pytorch/requirements-test.txt | ||
| python -m pip install -r external-builds/pytorch/pytorch/.ci/docker/requirements-ci.txt |
There was a problem hiding this comment.
Looks like this will bump the requirements install time from ~5 seconds (example: https://github.com/ROCm/TheRock/actions/runs/29119539622/job/86451121264#step:11:1) to ~1m07s (example: https://github.com/ROCm/TheRock/actions/runs/29264970035/job/86867806066#step:11:1)
If pytorch needs all of the extra packages, that's fine.
To optimize, we can look into tuning a persistent pip cache on our GPU test runners and/or switching from pip to uv.
There was a problem hiding this comment.
This looks fine to me, as long as we get more stable and comprehensive dependency installation
|
@ScottTodd There's only one thing I want to ask for your opinion: currently some of the pinned dependencies don't have prebuilt wheels for python 3.14, which is in our test matrix. If we run |
|
We need to be very careful with installing system-wide
I'd prefer to use requirements that include prebuilt wheels, and we can even host such wheels ourselves in our python package mirror repositories like https://rocm.nightlies.amd.com/whl-multi-arch/ |
|
That makes sense. I'll update the requirements file in pytorch, then we can merge this PR |
5 participants
Motivation
PyTorch Core UT workflow only installs the requirements specified in TheRock, but not the more detailed pytorch requirements, which cause numpy version not pinned and therefore leading to UT failures. Besides, this is also causing a lot of UTs missing dependencies such as scipy. This requirements.txt file IS installed in test_pytorch_wheels_full, but not in this workflow.