Repository navigation
fix(tensilelite): find msgpack master libraries in the LibraryClient step - #12569
Merged
tony-davis merged 4 commits intoSep 25, 2026
Merged
tony-davis merged 4 commits into
tony-davis merged 4 commits into
Conversation
github-actions
Bot
added
project: hipblaslt
project: hipsparselt
ci:hipsparselt-fast
labels
tony-davis
marked this pull request as ready for review
September 24, 2026 20:34
✅ 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. |
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Msgpack handling must support .dat.zlib outputs, and gfx942 integration validation is still pending.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
This pull request fixes TensileLite LibraryClient discovery for msgpack libraries.
Changes:
- Adds format-aware library discovery across architectures.
- Adds unit tests for library selection and multi-architecture results.
| File | Summary |
|---|---|
projects/hipblaslt/tensilelite/Tensile/Tests/unit/test_ClientWriter_library_files.py |
Tests library discovery behavior. |
projects/hipblaslt/tensilelite/Tensile/ClientWriter.py |
Updates client library discovery. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
gfx1250v0 output paths are not handled, leaving a client-library lookup failure unresolved.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
Resolved since last review (1)
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #12569 +/- ##
========================================
Coverage 69.94% 69.95%
========================================
Files 2933 2933
Lines 482179 482187 +8
Branches 69511 69512 +1
========================================
+ Hits 337251 337269 +18
+ Misses 120719 120711 -8
+ Partials 24209 24207 -2
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
1 task done
…step ClientWriter.main collected the client library with a *.yaml glob no matter what LibraryFormat was, so with LibraryFormat=msgpack it found nothing and crashed on yamlList[0] with IndexError. Any config with a LibraryClient section hit this, e.g. Tests/common/gemm/xfp32.yaml. Move the lookup into clientLibraryFiles(), which matches the master library by name and by the configured format's extension, so the always-msgpack TensileLiteLibrary_*_Mapping.dat side file is never picked. YAML behavior is unchanged. Co-authored-by: Cursor <cursoragent@cursor.com>
Msgpack libraries are written as <name>.dat.zlib, so the TensileLibrary*.dat glob still found nothing, and TensileLibrary* also matches lazy-loading shards. Build the master name from the arch and LazyLibraryLoading, accept it when either .dat or .dat.zlib exists, and pass the client the logical .dat name, which it resolves to .zlib itself. Write the unit-test fixtures with LibraryIO.write so their names match real TensileCreateLibrary output. Co-authored-by: Cursor <cursoragent@cursor.com>
…raries A gfx1250v0 client build is written to library/gfx1250v0/ (the child TensileCreateLibrary is invoked with --architecture=gfx1250v0), but ClientWriter looked in library/gfx1250/ and found nothing. Map each ISA-derived arch through computeOutputArchNames, as TensileCreateLibrary does, while keeping the ISA suffix in the master's filename. Co-authored-by: Cursor <cursoragent@cursor.com>
Only the first ISA's client library is rebuilt, so other archs can have lazy shards but no master. Cover the branch where clientLibraryFiles finds neither .dat nor .dat.zlib and adds nothing for that arch. Co-authored-by: Cursor <cursoragent@cursor.com>
tony-davis
force-pushed
the
users/tony-davis/tensilelite-clientwriter-library-format
branch
from
September 25, 2026 03:11
7ce84ac to
4db5b0b
Compare
davidd-amd
approved these changes
Sep 25, 2026
tony-davis
deleted the
users/tony-davis/tensilelite-clientwriter-library-format
branch
September 25, 2026 14:42
tony-davis
added a commit
to geotseng-amd/rocm-libraries
that referenced
this pull request
Sep 25, 2026
Brings the branch up to date with develop and ports what landed there under the gfx1250v0 name onto gfx1250-strict. Tuning logic. ROCm#12336 and ROCm#12271 replaced 4 gfx1250v0 logic files and added 7 (including a new Origami pair). The 4 are dropped here too. The 7 move to gfx1250-strict/, with ScheduleName and ArchitectureName rewritten the same way d05bf20 did for the rest. Every file in the tree now matches its develop counterpart apart from those two names. Test marks. ROCm#12282 skipped three streamk configs on gfx1250v0, and it and ROCm#12355 dropped xfail-gfx1250 from those and spmm_gate now that they pass. They keep skip-gfx1250-strict and lose the xfail. fused_a2a gained skip-gfx1250 on develop, so it also gets skip-gfx1250-strict, per this branch's convention. findAvailableArchs keeps this branch's version. The develop version made gfx1250v0 inherit base gfx1250 marks. That is the opposite of the mirrored marks here, where a strict config says skip-gfx1250. The four gfx1250v0 tests in test_config_helpers.py are ported to pin the strict behaviour instead, including that a base skip-gfx1250 does not fire on a strict target. ClientWriter. ROCm#12569 added clientLibraryFiles (msgpack master lookup) and routed a stepping through computeOutputArchNames, which this branch removes. It auto-merged into TensileCreateLibrary/__init__.py as an import of a function that no longer exists, so Tensile failed to import. The export is dropped. clientLibraryFiles keeps the msgpack fix but loses the remap, since a stepping is now its own subdir and master suffix. Its stepping test now checks that gfx1250-strict is found by name and that the gfx1250 files next to it are not picked up. Not changed: .github/workflows/gfx1250_hipblaslt_tensilelite_test.yml (ROCm#12164, manual dispatch) still passes --gpu-targets gfx1250v0 and selects with -m "gfx1250 and not skip-gfx1250". It needs porting to gfx1250-strict as a follow-up. Co-authored-by: Cursor <cursoragent@cursor.com>
tony-davis
added a commit
that referenced
this pull request
Sep 25, 2026
… tests (#12492) ISSUE ID : #12491 ## Motivation `Tensile/Tests/common` (real codegen, build, and execution against a CPU reference) only runs in Math CI's `preliminary` job today. It doesn't run anywhere in TheRock Multi-Arch CI or nightly. TESTING.md already lists this as a known gap (Improvement Roadmap item 5). This PR adds the category TheRock needs to run it. The TheRock side is a companion PR, linked below. ## Technical Details - Adds a `hw-common` category to `projects/hipblaslt/tensilelite/test_categories.yaml`. It runs `Tensile/Tests/common` with `-m common` and the prebuilt `tensilelite-client` from the artifact. - Arch selection is opt-out, the same way `preliminary` does it. `configMarks()` tags every config `common` and turns each config's `skip-gfxNNNN` marks into skips for whatever arch `rocm_agent_enumerator` reports. So this category deliberately doesn't use `{AMDGPU_TARGETS}` (that would match almost nothing on real hardware), and it doesn't pass `--gpu-targets`. Hardware detection also sets `PyTestBuildArchNames`, which is what lets a gfx120X runner build for whichever of gfx1200/gfx1201 it actually has. - `parallel_workers: 4`, which is what Math CI runs. I didn't copy the 16 from the FFM categories. `assign_gpu_to_worker()` maps workers with `worker_num % num_gpus` and hands out per-worker locks, so on TheRock's 1-GPU runners every worker shares GPU 0. - Per-test timeout is 5400s, as a hang guard only. Math CI runs this suite with no per-test timeout, and the slowest config I found in recent `preliminary` logs (`streamk/sk_I8gemm_quick.yaml`) took 3173s. - Updates `tensilelite/TESTING.md`: the CI-gates section, the TheRock row in the lanes table (it still pointed at the retired `test_tensilelite.py`), two Known Risks rows, and roadmap item 5 (now the deferred sharding follow-up). - Worker-to-GPU assignment on TheRock's 1-GPU runners is fixed by #11898 (merged separately), which this PR now picks up from develop. The first TheRock run of `hw-common` hit exactly that bug: all 76 failures were on xdist workers gw1-gw3 with `no ROCm-capable device is detected`. With a worker-assignment fix in place, a later run passed 135 of 136. The one failure was `gemm/xfp32.yaml`, which #12569 fixes. Nothing selects `hw-common` until the TheRock PR lands, so this is inert on its own. ## Test Plan - Loaded the YAML and ran TheRock's `pytest_runner.build_marker_expression()` against `hw-common` for each target family, to confirm the `-m` expression it builds. - Sized the timeouts from four recent green `preliminary` builds (PR-12472 #2, PR-12469 #1, PR-12462 #1, PR-12461 #1). - Real collection and execution need a HIP runtime, so that happens on the TheRock PR's GPU run. ## Test Result - Marker expressions come out as `(common) and not skip-<arch>` for gfx90a, gfx94X, gfx950, and gfx120X. - Measured `-m common` wall clock in `preliminary` at 4 workers: gfx950 1:43 to 2:03 (182 tests), gfx942 49 to 64 min (136), gfx90a 16 to 24 min (43), gfx12 13 to 25 min (38). - Ran `hw-common` on gfx942 through TheRock Multi-Arch CI ([run 35915539444](https://github.com/ROCm/rocm-libraries/actions/runs/35915539444)): 136 tests selected, matching Math CI's `preliminary`. Every test on gw0 passed (46/46). All 76 failures were on gw1-gw3 with `no ROCm-capable device is detected`, which is what the conftest fix addresses. The suite ran in 35 minutes. - With #12569 (the msgpack LibraryClient fix) below it in the stack and #12526's TheRock pins on top, `tensilelite-common` passed all 136 tests on gfx942 in 46 minutes on the stack's testing PR, #12573 ([job](https://github.com/ROCm/rocm-libraries/actions/runs/36062518748/job/107875479911)). That includes `gemm/xfp32.yaml`, the one test that failed before #12569. ## Submission Checklist - [x] Look over the contributing guidelines at https://github.com/ROCm/TheRock/blob/main/GOVERNANCE.md#pull-requests. ## Risk level Low. It's a new category plus docs, and nothing selects it until the TheRock PR merges. Related: ROCm/TheRock#8459 (companion), ROCm/TheRock#5183, ROCm/TheRock#3288 Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
TheRock Submodule Bump ActivityNewest first
|
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
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.
ISSUE ID : #12491
Motivation
TheRock's new
hw-commoncategory runs TensileLite's common GEMM suite withLibraryFormat='msgpack'. Every config with aLibraryClientsection fails there. On gfx942 that'sgemm/xfp32.yaml, which failed in two of two TheRock runs with anIndexErrorinClientWriter.main. On gfx950 about 10subtile_mxfp8_*configs will hit the same thing. Math CI uses the default YAML format, so it never sees this.Technical Details
ClientWriter.maincollected the client library withglob(archDir, "*.yaml")regardless ofLibraryFormat. Msgpack libraries are written as.dat.zlib, so the list came back empty andyamlList[0]raisedIndexError.clientLibraryFiles(). It builds the exact master name (TensileLibrary_lazy_<arch>with lazy loading,TensileLibrary_<arch>without) and accepts it if either.dator.dat.zlibexists. It passes the client the logical.datname, because the loader probes for.zlibitself and expects a single extension.TensileLibrary_prefix, andTensileLiteLibrary_lazy_<arch>_Mappingis always written as msgpack. A glob could pick either one.library/gfx1250v0/while the master keeps thegfx1250suffix. The helper now maps each arch throughcomputeOutputArchNames, the same routing TensileCreateLibrary uses. This was already broken ondevelop, in either format.Test Plan
Tensile/Tests/unit/test_ClientWriter_library_files.py. It covers msgpack and YAML, with and without lazy loading, with a shard and the mapping file present, the logical.datname, combining two archs, and a gfx1250v0 stepping subdir. Files are written withLibraryIO.write, so their names match real output.TensileLibrary*.datglob fails 5 of the first 6 tests, dropping the.zlibcheck fails 4, ignoringLazyLibraryLoadingfails 2, and ignoring the stepping routing fails 1.hw-commonon gfx942 in TheRock once this is in.Test Result
Submission Checklist
Risk level
Low. It's a one-function change to the client-library lookup. The msgpack path goes from always crashing to working. The YAML path now picks the master by exact name instead of the first
*.yamlmatch.