Repository navigation
[tensilelite] Add requirements-test.txt to use on CI - #11396
Conversation
github-actions
Bot
added
project: hipblaslt
project: hipsparselt
ci:hipsparselt-fast
labels
✅ 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. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #11396 +/- ##
===========================================
- Coverage 70.78% 70.76% -0.02%
===========================================
Files 2842 2801 -41
Lines 465020 460117 -4903
Branches 68499 67751 -748
===========================================
- Hits 329152 325593 -3559
+ Misses 112174 111042 -1132
+ Partials 23694 23482 -212
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
## Motivation * Progress on ROCm/TheRock#7491 * See also ROCm/libhipcxx#29 * See also #11396 We've been using a common https://github.com/ROCm/TheRock/blob/main/requirements-test.txt file across all ROCm subproject tests. libhipcxx and hipthreads are currently the only subprojects using `lit` during tests, so we can move the requirement into project-local files that are only installed as part of testing those subprojects. ## Test Plan I'm testing this together with some changes in TheRock that will start picking up this requirements file from the test artifact. See https://github.com/ROCm/TheRock/actions/runs/33191445520/job/98940712134. It should be safe to add the requirements here and then start using them from TheRock later. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests.
|
Ping? 3+ weeks turnaround time for a 20 LOC change isn't great... and I have a stack of work building on this that is stalled. |
## Description * Progress on ROCm/TheRock#7491 * See also ROCm/rocm-libraries#11396 * See also ROCm/TheRock#2018 We've been using a common https://github.com/ROCm/TheRock/blob/main/requirements-test.txt file across all ROCm subproject tests and these packages are relatively large to download for all subprojects on runners with slow network (sccache is 8-15MB), so I want each project to declare which test requirements it has directly and leave only _CI system_ and _test runner_ requirements in the common file. The full libhipcxx source is already included in the 'test' artifact in TheRock via https://github.com/ROCm/TheRock/blob/950330ecc2554996ff6a424780a81bf1aff64e32/math-libs/artifact-libhipcxx.toml#L10-L18, so this won't need extra work to start distributing. --------- Co-authored-by: Marius Brehler <marius.brehler@gmail.com>
tony-davis
left a comment
tony-davis
left a comment
There was a problem hiding this comment.
Sorry for the slow turnaround on this one, I know it's been sitting for a few weeks.
This looks good to me. The CMake install block follows the same pattern used just above it for AMaxGenerator.py/LayerNormGenerator.py, and the pinned versions in requirements-test.txt match TheRock's own tensilelite/rocisa block 1:1. It also mirrors the hipthreads sibling PR (#11438) that already merged with the same shape, so there's good precedent here.
I just hit "Update branch" to rebase onto develop and kick off a fresh CI run. The failures you were seeing (Math CI Summary, TensileLite coverage, etc.) trace back to a known sibling-DeviceNames YAML issue (#11397) that was already fixed on develop by #11442, so I'd expect this to go green now that we're picking that up.
Approving. Thanks for the patience, and thanks for the clear explanation earlier on requirements.txt vs requirements-test.txt, that made sense to me.
|
Thanks! I'll see if CI can go through and this can get landed + picked up by a submodule update while the dependent PR ROCm/TheRock#7728 is still in review. I switched it to use a local |
There was a problem hiding this comment.
This is too much duplications. It's not good practice to e.g. specify pytest versions, msgpack versions, in all of these places. A break somewhere is almost guaranteed. I would propose adding what you need to requirements-dev for now, or I will likely have a better proposal later today.
> [!IMPORTANT] > This relates to the following PRs: > * ROCm/libhipcxx#29 > * ROCm/rocm-libraries#11396 > * ROCm/rocm-libraries#11438 ## Motivation * Fixes #7491 We've been using a common https://github.com/ROCm/TheRock/blob/main/requirements-test.txt file across all ROCm subproject tests that seemed convenient to extend at the time, but it has a few drawbacks: * #5529 added 50MB+ of dependencies only used for tensilelite tests but downloaded for all subprojects, sometimes contributing up to 50 minutes of job time _per CI run_ just downloading packages on runners with slow network and no pip cache * #6106 required sending a pull request to TheRock in order to run "TensileLite characterization (snapshot) tests", when such changes could be made purely here in rocm-libraries if we keep the responsibility for maintaining requirements lists next to the test code itself ## Technical Details > [!IMPORTANT] > Now each project that installs additional requirements should provide its own `requirements.txt` as part of its test artifact and register that with the CI test system via the `additional_requirements_files` field in `build_tools/github_actions/fetch_test_configurations.py`. Comparing https://github.com/ROCm/TheRock/actions/runs/33191445520 (this branch) vs a baseline https://github.com/ROCm/TheRock/actions/runs/33159247177, in aggregate across up to 50 subproject test jobs, this results in: * Most test jobs now download ~15MB instead of 110MB of python packages --> save ~4.5GB of download bandwidth per full CI run * Installing requirements median time dropped from 39s to 2.5s --> save ~24 runner-minutes per full CI run ## Test Plan * Tested via `workflow_dispatch`: https://github.com/ROCm/TheRock/actions/runs/33191445520 and checked that jobs complete successfully after installing the expected set of packages * Note the 'quick' test levels, while projects run additional tests that other test levels that _may_ have additional requirements ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md.
|
ROCm/TheRock#7728 is now merged.
So each subproject can now maintain its own requirements directly. If you want to manage the requirements from this rocm-libraries repository, a requirements.txt like will need to be packaged in the test artifact like what this PR does. Without that, the requirements will still be maintained separately in TheRock. |
I believe I understand what you are saying. The goal would be that these use cases:
To all have the same understanding of "what pytest version is used in running the tests?" We also have concrete user stories that will tie us further to the uv ecosystem, which have so far been banned on TheRock infra. So finding a way to specify in 1 place e.g. what version of |
|
Yep. You can also do things like have requirements file include each other, e.g. Just note that the requirements-test.txt would need to function standalone as CI and shipped packages do not do a full git clone of the source repository, they just include files that get packaged into artifacts. # requirements-test.txt
# Dependencies needed to run TensileLite and rocisa tests.
# Includes Python runtime dependencies exercised by the tests.
pytest
pytest-timeout
pytest-xdist
syrupy
packaging
PyYAML
msgpack
joblib
numpy
filelock
hip-python; sys_platform != "win32"# requirements-dev.txt:
-r requirements-test.txt
# Build rocisa from source.
./rocisa
# Additional development tools.
tox
invoke
memory_profiler
pre-commitYou could also update https://github.com/ROCm/rocm-libraries/blob/develop/projects/hipblaslt/tensilelite/requirements.txt, moving dependencies from there into the test file: --- a/projects/hipblaslt/tensilelite/requirements.txt
+++ b/projects/hipblaslt/tensilelite/requirements.txt
@@
-packaging
-pyyaml
-msgpack
-joblib>=1.4.0
+# Source development includes the artifact test dependencies.
+-r requirements-test.txt
+
+# Additional source-build and development dependencies.
simplejson
ujson
orjson
yappi
tox
invoke
-filelock
-numpy
-hip-python
# nanobind compatibility comment retained here.
nanobind>=2.0,!=3.0.0
scikit-build-core>=0.10
--- /dev/null
+++ b/projects/hipblaslt/tensilelite/requirements-test.txt
@@
+# Standalone dependencies for testing prebuilt TensileLite and rocisa artifacts.
+# Ship this file alongside the tests; compiled components come from the artifact.
+pytest==9.0.3
+pytest-timeout==2.4.0
+pytest-xdist>=1.32.0
+syrupy==5.3.2
+packaging==25.0
+PyYAML==6.0.2
+msgpack>=1.0.0
+joblib>=1.4.0
+numpy>=1.26.0
+filelock>=3.0.0
+hip-python; sys_platform != "win32"
--- a/projects/hipblaslt/tensilelite/requirements-dev.txt
+++ b/projects/hipblaslt/tensilelite/requirements-dev.txt
@@
+# Includes source-build tooling and, transitively, artifact test dependencies.
-r ./requirements.txt
./rocisa
memory_profilerI don't have too much of an opinion on pinned |
|
Thanks for moving this out of TheRock — the cross-repo situation it replaces is worse, and the I've put a small patch in #12319 against this branch, so it can land without another round-trip.
|
|
That sounds good to me, thanks. |
…nd pyproject.toml (#12319) JIRA ID : N/A Targets #11396 rather than `develop` — merge into that branch to pick these up. ## Motivation #11396 adds `tensilelite/requirements-test.txt`. Moving the list out of TheRock and next to the tests is a clear improvement, and the CMake install rule is correct as written (including the absence of `OPTIONAL`, which is what makes a bad path fail loudly rather than be skipped). The concern is only the version limits. `requirements-test.txt` is the **fifth** place under `tensilelite/` that declares Python dependencies: | File | Read by | |---|---| | `requirements.txt` | `install.sh:47`, `tox.ini:20,124,165`, `MANIFEST.in` | | `requirements-dev.txt` | `utilities/geko/README.md:268` setup step | | `pyproject.toml` `[project].dependencies` | `uv.lock`, and tox's sdist install (no `skipsdist`) | | `pyproject.toml` `[dependency-groups].dev` | `uv sync` | | `tox.ini:16-26` inline `deps` | every tox env | They agree today. Nothing checks that they keep agreeing. `requirements-test.txt` is the only one of the five that hard-pins, so it is the one that drifts first — and nothing in CI would report it. ## Technical Details Relax the four hard pins to the limits already stated elsewhere in the tree: | Package | Before | After | Source of the limit | |---|---|---|---| | `pytest` | `==9.0.3` | `>=5.4.1` | `tox.ini:21`, `pyproject.toml:105` | | `PyYAML` | `==6.0.2` | *(none)* | `requirements.txt` | | `packaging` | `==25.0` | *(none)* | `requirements.txt`, `pyproject.toml:77` | | `syrupy` | `==5.3.2` | *(none)* | `tox.ini:24`, `pyproject.toml:108` | ### On `pytest` To be precise about what the problem is and is not: the newest pytest is 9.1.1, and `>=5.4.1` is a floor rather than a pin, so `tox -e unit` already resolves to pytest 9 today. This is *not* a case of two lanes running two different majors. The problem is that `==9.0.3` freezes one lane while every other lane floats. They start together and separate over time. The `>=5.4.1` floor is itself stale — it traces to the pytest 5.3.3 defect note at `tox.ini:12-14`, which is years old. It does need raising. The right place to raise it is `pyproject.toml`, using PEP 735 `include-group` nesting so a single declaration propagates, rather than a new version arriving by hand in a fifth file. ### On the other three `PyYAML`, `packaging` and `syrupy` float everywhere else in the repo, so the least damaging thing is to let them float here too until this is settled through `pyproject.toml`. This is consistency rather than risk: `packaging` has exactly one use in the package (`Tensile/Common/Parallel.py:38`), and PyYAML 6.x is interface-stable. `syrupy` deserves a footnote. There are 101 `.ambr` snapshot files under `Tensile/Tests`, so the syrupy version and those files are coupled and a pin has a real purpose. But a pin in one file of three does not help — it makes the artifact lane and the tox lane read the same snapshots with different engines. One value in all three files is the actual fix, and that is larger than this PR. ### Kept unchanged * `pytest-timeout==2.4.0` — `tox.ini:23` already states the same value. * `msgpack>=1.0.0`, `joblib>=1.4.0`, `numpy>=1.26.0`, `filelock>=3.0.0` — a floor does not contradict a bare requirement, so these add information without creating a second source of truth. * `projects/hipblaslt/CMakeLists.txt` — no change. * Line order — unchanged, to keep the diff readable. ### `requirements-dev.txt` One line added: `-r ./requirements-test.txt`. This links the new file into the set instead of leaving it as a fifth parallel list, and it closes a gap that exists today: `requirements-dev.txt` ships no test runner at all, so anyone following the geko README setup step (`utilities/geko/README.md:268`) ends up without pytest. `-r` paths resolve against the directory of the file that contains them, so this line is safe from any working directory. `requirements-test.txt` itself gains no `-r` line, so it remains standalone for CI and for shipped artifacts, which do not clone the source repo. Note this does not yet remove the overlap between `requirements.txt` and `requirements-test.txt` — seven packages are still declared in both. See the follow-ups. ## Test Plan No behavioural change to any code path. Nothing reads `requirements-test.txt` yet — TheRock currently uses its own `build_tools/github_actions/test_executable_scripts/requirements-test-tensilelite.txt` (ROCm/TheRock#7728) — so this only affects the contents of the installed test artifact. Checks: the existing #11396 CI, which exercises the `install(FILES ...)` rule via `HIPBLASLT_INSTALL_TENSILELITE_TEST_ARTIFACTS` (`ROCm/TheRock:math-libs/BLAS/CMakeLists.txt:172` sets it from `THEROCK_BUILD_TESTING`). ## Test Result Pending CI on this branch. Worth knowing when reading the green checks: the contents of the file are unvalidated by CI on either branch until TheRock installs from it. The `Test tensilelite` job uses the runner's ambient pytest. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests. ## Risk level None. Four version limits relaxed to values already declared elsewhere in the tree, plus one `-r` line. No code, no build logic, no CMake changes. ## Follow-ups (not in this PR) 1. Remove the remaining overlap between `requirements.txt` and `requirements-test.txt` (seven packages), keeping `requirements-test.txt` standalone. 2. Remove `./rocisa` from `requirements-dev.txt`. It is a CWD-relative path (see the workaround comment at `utilities/geko/tox.ini:47`) and it produces a different install than `invoke rocisa` — no `ROCM_PATH`, no `ROCISA_INCLUDE_BUILD_INFO=ON`, non-editable, with build isolation. Needs a matching geko README update. 3. Make `pyproject.toml` the single source of truth for versions, with PEP 735 nesting so one declaration propagates, and a check that reports drift. This is what the `TODO` in the file refers to. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude <noreply@anthropic.com>
|
Looks like CI workflows include some flaky test jobs, might need some help merging (or I can override myself). |
TheRock Submodule Bump ActivityNewest first
|
|
I'm preparing a PR to TheRock that will switch to using this now. Once that lands and rocm-libraries updates its "TheRock ref" to include it, you'll be able to change which packages are installed for tests under "TheRock CI" with only edits in this repository. |
) ## Motivation Following up on: * #7491 * #7728 * ROCm/rocm-libraries#11396 Now that `projects/hipblaslt/tensilelite/requirements-test.txt` in rocm-libraries exists and is included in packages we can start using it directly and drop the local copy in TheRock's `build_tools/github_actions/test_executable_scripts/requirements-test-tensilelite.txt`. ## Test Plan Check that artifacts include the new requirements file, the `_get_artifact_path` syntax works, and the tests execute correctly after installing the packages listed. ## Test Results tensilelite: https://github.com/ROCm/TheRock/actions/runs/36042000912/job/107777294331 * Setup additional requirements: ``` python ./build_tools/install_additional_requirements.py \ --requirements-files "${REQUIREMENTS_FILES}" env: VENV_DIR: /home/runner/_work/TheRock/TheRock/.venv ARTIFACT_RUN_ID: 36008887516 OUTPUT_ARTIFACTS_DIR: ./build THEROCK_BIN_DIR: ./build/bin AMDGPU_FAMILIES: gfx94X-dcgpu AMDGPU_TARGETS: gfx942 ARTIFACT_GROUP: multi-arch-release BUILD_VARIANT: release VIRTUAL_ENV: /__w/TheRock/TheRock/.venv REQUIREMENTS_FILES: build/share/hipblaslt/tensilelite/requirements-test.txt Resolved 18 packages in 178ms Downloading numpy (15.9MiB) Downloading rocm-bindings-hip (3.0MiB) Downloaded rocm-bindings-hip Downloaded numpy Prepared 11 packages in 173ms warning: Failed to hardlink files; falling back to full copy. This may lead to degraded performance. If the cache and target directories are on different filesystems, hardlinking may not be supported. If this is intentional, set `export UV_LINK_MODE=copy` or use `--link-mode=copy` to suppress this warning. Installed 11 packages in 148ms + cloudpickle==3.1.2 + execnet==2.1.2 + filelock==4.0.3 + hip-python==10.0.0.0.1.1 + joblib==1.6.0 + msgpack==1.2.2 + numpy==2.5.3 + pytest-xdist==3.8.0 + rocm-bindings-core==10.0.0.0.1.1 + rocm-bindings-hip==10.0.0.0.1.1 + syrupy==6.1.1 ``` * Test: ``` collecting ... collected 8548 items / 143 deselected / 8405 selected ... 100% tests passed, 0 tests failed out of 1 ``` tensilelite-common: https://github.com/ROCm/TheRock/actions/runs/36042000912/job/107777294399 ``` Component: tensilelite-common (/__w/TheRock/TheRock/build/share/hipblaslt/tensilelite) Test category: hw-common Loaded test categories from /__w/TheRock/TheRock/build/share/hipblaslt/tensilelite/test_categories.yaml Test category 'hw-common' is not defined by this component yet (available: ['comprehensive', 'emu-fast', 'emu-full', 'ffm-quick', 'full', 'quick', 'standard']); skipping. Warning: Test category 'hw-common' is not defined by this component yet (available: ['comprehensive', 'emu-fast', 'emu-full', 'ffm-quick', 'full', 'quick', 'standard']); skipping. ``` ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md.
## Summary The rocjitsu sidecar relied on therock's common test environment to provide tensilelite's Python dependencies. After those dependencies were split out by project, `joblib` was no longer installed and the tensilelite workload failed during import before running under rocjitsu. Install the tensilelite test requirements explicitly after unpacking the test artifacts, using the requirements file shipped with the artifact by #11396. ## Testing - Pre-commit and `actionlint` - Validated the artifact requirements path against the installed artifact layout JIRA ID: AIHPBLAS-3605
## Summary The rocjitsu sidecar relied on therock's common test environment to provide tensilelite's Python dependencies. After those dependencies were split out by project, `joblib` was no longer installed and the tensilelite workload failed during import before running under rocjitsu. Install the tensilelite test requirements explicitly after unpacking the test artifacts, using the requirements file shipped with the artifact by #11396. ## Testing - Pre-commit and `actionlint` - Validated the artifact requirements path against the installed artifact layout JIRA ID: AIHPBLAS-3605 (cherry picked from commit 973351c)
Motivation
We've been using a common https://github.com/ROCm/TheRock/blob/main/requirements-test.txt file across all ROCm subproject tests that seemed convenient to extend at the time, but it has a few drawbacks:
Including
requirements-test.txtin the test artifact will also allow users installing the test artifacts/tarballs to know which packages are needed too.Technical Details
The contents of this
projects/hipblaslt/tensilelite/requirements-test.txtfile were lifted from both https://github.com/ROCm/TheRock/blob/main/requirements-test.txt and an analysis of tensilelite/rocisa test entry points. I'm not sure if this is the exact list that these projects want, but once the project is in direct control of the requirements further edits can be made more easily.Test Plan
I'm testing this together with some changes in TheRock that will start picking up this requirements file from the test artifact. See https://github.com/ROCm/TheRock/actions/runs/33191445520/job/98940712172. It should be safe to add the requirements here and then start using them from TheRock later.
Submission Checklist