Repository navigation
Conversation
The `requirements-test.txt` pulls in `hip-python==7.2.2.562.43` which is not desired.
✅ 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. |
| numpy>=1.26.0 | ||
| filelock>=3.0.0 | ||
| syrupy==5.3.2 | ||
| hip-python; sys_platform != "win32" |
There was a problem hiding this comment.
PR #5529 added
hip-pythonas a test dependencies, but following #5529 (comment) it can be dropped.
I'm not sure about that comment, see #5529 (comment)
There was a problem hiding this comment.
Yes, @marbre since the submodule bumps (rocm-libraries) started running the gfx950 tests recently, removing hip-python will silently skip 12 GPU tests.
Example tests that will be skipped but required on gfx950, passing now: https://github.com/ROCm/TheRock/actions/runs/29815512767/job/88684173821#step:14:4809
Do you have guidance on the right mechanism to add hip-python in TheRock workflow?
There was a problem hiding this comment.
No, I have not. There isn't a hip-python release for the current ROCm 7.14 production release. Maybe @domcharrier or @psamfass can give guidance.
There was a problem hiding this comment.
What APIs are called? If this subset is stable between ROCm 7.2.2 and ROCm LATEST then it is very likely safe to use hip-python~=7.2.2. All libraries and function symbols are loaded lazily. There would only be an issue if you use newly added HIP APIs or a signature had changed (type/order/number of args/return values).
There was a problem hiding this comment.
The TensileLite gfx950 GPU subtile tests use hip-python for HIP runtime calls to assemble and execute generated subtile kernels — hipModuleLoad, hipModuleLaunchKernel, hipMalloc, hipMemcpy, hipDeviceSynchronize, etc.
hip-python 7.2.2 uses dlopen("libamdhip64.so") at runtime — so it calls into whichever ROCm is installed (10.x in TheRock). It works today because the APIs we use haven't changed, but newer ROCm versions could add/change struct fields that the 7.2 wrapper might not know about.
There was a problem hiding this comment.
I think we should move this requirement to be local to only the tests that need it.
The unconditional download of 64MB hip-python packages is actually contributing pretty significantly to test time on some gfx942 runners with slow network, and pip/uv caching isn't enabled (? see also #3674).
For example: https://github.com/ROCm/TheRock/actions/runs/32165005832/job/96055002747?pr=7449#step:8:160
Tue, 18 Aug 2026 18:57:10 GMT Resolved 49 packages in 45.50s
Tue, 18 Aug 2026 18:57:10 GMT Building perfetto==0.7.0
Tue, 18 Aug 2026 18:57:10 GMT Built perfetto==0.7.0
Tue, 18 Aug 2026 18:57:11 GMT Downloading pygments (1.2MiB)
Tue, 18 Aug 2026 18:57:11 GMT Downloading setuptools (1.0MiB)
Tue, 18 Aug 2026 18:57:11 GMT Downloading numpy (15.9MiB)
Tue, 18 Aug 2026 18:57:11 GMT Downloading sccache (14.0MiB)
Tue, 18 Aug 2026 18:57:11 GMT Downloading botocore (13.3MiB)
+Tue, 18 Aug 2026 18:57:11 GMT Downloading hip-python (63.8MiB)
Tue, 18 Aug 2026 18:57:53 GMT Downloaded setuptools
Tue, 18 Aug 2026 18:57:56 GMT Downloaded pygments
Tue, 18 Aug 2026 19:00:28 GMT Downloaded botocore
Tue, 18 Aug 2026 19:00:35 GMT Downloaded sccache
Tue, 18 Aug 2026 19:00:51 GMT Downloaded numpy
+Tue, 18 Aug 2026 19:06:07 GMT Downloaded hip-python
+Tue, 18 Aug 2026 19:06:07 GMT Prepared 49 packages in 8m 56sAggregate statistics from that run show all 51 test jobs downloaded the 64MB wheel but none of them used it as TensileLite is the only user with relevant test imports and the "quick" category ran with not gpu and not skip-gfx94X. Timing was pretty variable due to differing network speeds:
| Metric | Duration |
|---|---|
| Aggregate runner time | 50m 39.8s |
| Mean | 59.6s |
| Median | 16.5s |
| p90 | 2m 12.9s |
| p95 | 7m 55.6s |
| Maximum | 8m 55.8s |
| Under 30s | 44/51 jobs |
| Over 2 minutes | 6/51 jobs |
Out of curiosity, why is hip-python so large, at 64MB compressed? Looking at https://pypi.org/project/hip-python/7.2.2.562.43/#files, I see:
| file name | uncompressed size |
|---|---|
hipblas.cpython-314-x86_64-linux-gnu.so |
101MB |
hip.cpython-314-x86_64-linux-gnu.so |
81MB |
hipsparse.cpython-314-x86_64-linux-gnu.so |
47MB |
hipsolver.cpython-314-x86_64-linux-gnu.so |
41MB |
rccl.cpython-314-x86_64-linux-gnu.so |
6MB |
chipblas.cpython-314-x86_64-linux-gnu.so |
5MB |
hiprand.cpython-314-x86_64-linux-gnu.so |
3MB |
There was a problem hiding this comment.
none of them used it as TensileLite is the only user with relevant test imports and the "quick" category ran with
not gpu and not skip-gfx94X.
correction: it might be used at "quick", but only for tensilelite and no other components
There was a problem hiding this comment.
While we're at it, we can move at least sccache and numpy to component-specific test requirements too:
Lines 16 to 29 in 04a2474
botocore from boto3 is what we use to download artifacts, so I don't think we can easily slice that out
There was a problem hiding this comment.
For " # TODO: Keep in sync with rocm-libraries/projects/hipblaslt/tensilelite/requirements.txt ", what if we copy that requirements.txt into the test artifact for tensilelite?
Motivation
The
requirements-test.txtpulls inhip-python==7.2.2.562.43which is not desired.Technical Details
PR #5529 added
hip-pythonas a test dependencies, but following #5529 (comment) it can be dropped.Submission Checklist