Repository navigation
feat(hipblaslt): Introduce Solution Unique ID generation foundation - #12185
Merged
Merged
Conversation
Keep the cross-process uniqueness check at a winner-scale batch so the test is less likely to flake on same-millisecond 24-bit collisions.
✅ 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 #12185 +/- ##
===========================================
- Coverage 72.12% 71.70% -0.42%
===========================================
Files 2868 2824 -44
Lines 473269 467535 -5734
Branches 69803 69199 -604
===========================================
- Hits 341336 335217 -6119
- Misses 108234 108824 +590
+ Partials 23699 23494 -205
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
Exercise missing-file, non-dict YAML, and epoch overflow branches so TensileGenerateUID and SolutionIdGen unit coverage is complete.
The shipped logic YAML lives in the hipBLASLt product tree, so it is missing when this test runs from installed tensilelite artifacts. Guard it with the same skipif other corpus tests use instead of failing.
Alex-Vasile
requested changes
Oct 1, 2026
Alex-Vasile
left a comment
Alex-Vasile
left a comment
Contributor
There was a problem hiding this comment.
test_generate_solution_id_is_unique_across_sequential_calls is the one I tested for flakiness, but others may have a similar issue.
This also needs to be considered when we're going to be generating new kernels as they will likely run into this same issue depending on how numbering will be implemented.
Redraw a repeated ID inside the concurrency test so one process does not fail on a same-millisecond collision.
Alex-Vasile
reviewed
Oct 5, 2026
Alex-Vasile
approved these changes
Oct 6, 2026
Alex-Vasile
left a comment
Alex-Vasile
left a comment
Contributor
There was a problem hiding this comment.
Remaining concern: guaranteeing that the generated ID is globally unique during the course of regular generation of kernels without requiring a manually activate verification and fixup pass or relying on CI to catch the issue.
Discussed offline and that concern will be addressed in one of the follow-up PRs that fully integrates an enables this feature. Approving with the understanding that this required change is within the scope of one of those follow-up PRs.
Contributor
|
Cancelling the remaining gfx950 jobs on this run so the TheRock submodule bump PRs can be prioritized on the gfx950 queue. This change does not target gfx950, and gfx950 was the only work still outstanding. Jobs that already finished are unchanged. Re-run failed jobs if you need gfx950 results before merge. |
newling
reviewed
Oct 8, 2026
newling
left a comment
newling
left a comment
Contributor
There was a problem hiding this comment.
Stable solution indices will be very nice, thanks for working on this! An agent review (https://github.com/newling/agent-public-reviews/blob/main/rocm-libraries/review_pr12185.md) has a few suggestions and fixes, please take a look.
- Use PyYAML instead of raw string parsing so the uniqueness check handles reordered fields, inline mappings, and comments correctly.
- Preserve the intended YAML field order when regenerating IDs by passing sort_keys=False to the writer.
- Forward the UID checker’s environment variables through tox so strict mode, custom logic directories, and worker counts take effect.
- Clarify the docs: UID decoding is explicit, and the later migration step makes IDs mandatory.
Contributor
TheRock Submodule Bump ActivityNewest first
|
ScottTodd
reviewed
Oct 14, 2026
Comment on lines
+92
to
+99
| library-uniqueness: | ||
| name: "hipBLASLt library uniqueness" | ||
| needs: changes | ||
| if: needs.changes.outputs.library-uniqueness == 'true' | ||
| uses: ./.github/workflows/component-ci-library-uniqueness.yml | ||
| with: | ||
| docker_image: ${{ needs.changes.outputs.docker-image }} | ||
| linux_runner: ${{ needs.changes.outputs.linux-runner }} |
Member
There was a problem hiding this comment.
Please use smaller runners for these tests (or move them into an existing workflow instead of adding a new workflow). 96 core machines are expensive and in short supply.
https://github.com/ROCm/rocm-libraries/actions/runs/37805607146/job/113409341093 took 3 minutes total for just 7 seconds of tests. 2m53 was just overhead checking out the repository.
tony-davis
added a commit
that referenced
this pull request
Oct 19, 2026
- R2: require caller edits only for breaking interface changes; call out external callers. - R3: keep review read-only; run pre-commit in a throwaway checkout when reviewing. - R4: say the base helper does not apply the high-coupling list. - R8: shorter description, full links to the base skill, note on extends/argument-hint. - M4/R7: add a completed retrospective review of #12185 as a worked example. Resolves #13727 Co-authored-by: Cursor <cursoragent@cursor.com>
tony-davis
added a commit
that referenced
this pull request
Oct 19, 2026
- R2: require caller edits only for breaking interface changes; call out external callers. - R3: keep review read-only; run pre-commit in a throwaway checkout when reviewing. - R4: say the base helper does not apply the high-coupling list. - R8: shorter description, full links to the base skill, note on extends/argument-hint. - M4/R7: add a completed retrospective review of #12185 as a worked example. Resolves #13727 Co-authored-by: Cursor <cursoragent@cursor.com>
tony-davis
added a commit
that referenced
this pull request
Oct 20, 2026
…d scripts (#13729) Resolves #13727 ## Summary Adds a `github-ci-pr-quality` overlay on the `rocm-pr-quality` base skill for changes under `.github/`. It tells authors and reviewers where CI tests belong (extend an existing lane, not a new workflow per test), how to size runners and checkout, how to treat shared state in `ci-env`, and to read each affected project's TESTING.md before changing a lane or gate. It also covers what a CI job checks out (the PR merge ref, `refs/pull/<N>/merge`, so every lane tests the same tree) and requires CI references to be updated when files move or are deleted. A matching Copilot review instructions file loads it for `.github/**`. This is the second of two PRs; it is stacked on the root AGENTS.md PR (#13728), so its base is that branch until #13728 merges. ## Risk Assessment 1/5. Guidance for reviewers and AI assistants only. No workflow, script, or build changes, and the guidance is advisory. One behavior change: Copilot code review will start applying this guidance to PRs that touch `.github/**`. ## Related - Work tracking: #13727 - Depends on: #13728 (the root AGENTS.md PR, tracked in #13726) - Cases it is designed to catch: #12185, #13623 (reverted in #13698), and the stale paths left by #12150 (fixed in #13399) - Review input: a skill-forge review of the PR-quality skills (findings R2, R3, R4, R8, M4/R7 applied here; the helper findings M1, M2, M3, and R5 are fixed in ROCm/TheRock#6653; the rest of the base-skill findings are still open) - Base skill: `skills/rocm-pr-quality/` in ROCm/TheRock ## Testing Summary - `pre-commit` on the changed files passed. Every workflow, script, action value, and path the skill cites was checked against `origin/develop`. ## Testing Checklist - [x] pre-commit on the changed files - Status: Passed - [x] Cited paths and facts checked against `develop` - Status: Passed - [ ] PR CI - GitHub PR checks - Status: Pending ## Flags / Guardrails None. The guidance is advisory. ## Adjacent Tests Considered None apply; no code changes. ## Technical Changes - New `skills/github-ci-pr-quality/SKILL.md` with the TESTING.md cross-read, five CI change classes, and test-placement, runner-sizing, trigger-scope, tested-tree, and shared-state rules. Trigger scope includes checks that path filters match real files and that moves and deletes update CI in the same PR. It tightens stale-base and approvals, adds a review checklist, three worked examples, and a completed retrospective review of #12185 as a model of the required output. - Review contract: the overlay keeps review read-only (pre-commit runs in a throwaway checkout), and requires caller edits only for breaking workflow interface changes. - New `.github/instructions/github-ci-pr-quality.instructions.md` (`applyTo: ".github/**"`). - `AGENTS.md`: the planned catalog row now links to the skill, and the new instructions file is listed. ## Risk Acceptance / Waivers `W-DOC`: guidance only; no product or CI behavior changes. --------- Co-authored-by: Cursor <cursoragent@cursor.com>
dsaffars
added a commit
that referenced
this pull request
Oct 23, 2026
JIRA ID: AIHPBLAS-5178 ## Summary - Move the hipBLASLt SolutionUID uniqueness check from the prod 96-core build runner to a standard GitHub-hosted CPU runner (`ubuntu-24.04`), with no container. - Rename the check from `Component CI / hipBLASLt library uniqueness / Linux` to `Component CI / hipBLASLt library UID uniqueness / Linux`. ## Motivation `tox -e library-uniqueness` only scans the logic YAML and checks that every `SolutionUID` is unique. It does not build hipBLASLt, does not use a GPU, and does not need ROCm. Component CI was still scheduling it on `aws-linux-scale-rocm-prod` inside the TheRock build container, the same pool used for rocISA, Geko, and TensileLite coverage. Those 96-core machines are expensive and in short supply, and the job spent far longer on checkout than on the test itself. This is a stopgap for the review feedback on #12185 (#12185 (comment)). It fixes the runner cost. It does not change where the test lives. ## Technical Details `.github/workflows/component-ci-library-uniqueness.yml` now runs on `ubuntu-24.04` with Python 3.12 (using the same `actions/setup-python` pin as `pre-commit.yml`) and no container. It still runs `python -m tox -e library-uniqueness`. The workflow no longer takes `docker_image` or `linux_runner` inputs, and the caller in `.github/workflows/component-ci.yml` no longer passes them. The job name is now `hipBLASLt library UID uniqueness`. The nested job is still `Linux`, so a pull request check is `Component CI / hipBLASLt library UID uniqueness / Linux (pull_request)`. The only required checks on `develop` are the summary jobs, so the rename does not affect branch protection. The test itself is unchanged. It still uses 8 worker processes (`HIPBLASLT_UID_TEST_PROCESSES`, default 8) on the runner's 4 vCPUs. ## Test Plan The workflow file is in the component's trigger list, so this PR exercises the change on its own CI. ## Test Result The job passed on the new runner: https://github.com/ROCm/rocm-libraries/actions/runs/38761106727/job/116512738350. Total time was about 5 minutes, with about 4 minutes of that in checkout and about 46 seconds in the gate (including tox environment setup). That is a little slower in wall-clock time than before, but it is not on the critical path of Component CI and it no longer uses the prod runner pool. ## Related - Follow-up to #12185 (#12185), which introduced the check.
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.
JIRA ID: AIHPBLAS-4733
Motivation
hipBLASLt still identifies kernels by a build-order global solution index. That index moves when logic YAML files are added, removed, or reordered, so bench logs, geko, and CI cannot tell whether a kernel itself changed or only the library layout did.
This PR is step 1 of the SolutionUID migration. It introduces a stable 64-bit
SolutionUIDformat, YAML field placement, regeneration tooling, and a uniqueness gate. It does not backfill shipped logic YAML, does not mint UIDs duringTensileCreateLibrary, and does not change runtime or bench selection.Technical Details
Identifier. Internally
SolutionUIDis auint64:(ms_since_epoch << 24) | secrets.randbits(24):EPOCH_MS = 1767225600000, ~35 years of headroom)Generated in
tensilelite/Tensile/Common/SolutionIdGen.py. Identity, comparison, uniqueness, and mapping use that integer. YAML and CLI print an unpadded base62 string:0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0u; readers also accept0U0u02^64 − 1are rejected (same class of error as an out-of-rangesolution_index)200is0u3EIn dict-format logic YAML the optional, unquoted field sits immediately after the local
SolutionIndex:encode_solution_uid/decode_solution_uidlive next to generation.read_solution_uiddecodes the string immediately;ensure_solution_uid/regenerate_solution_uidstore the canonical0ustring.YAML I/O only.
LibraryIO.reorderSolutionsParamspreserves that key order when a UID is present. Existing solutions without the field are unchanged.Regenerate one UID (no full-repo scan):
cd projects/hipblaslt/tensilelite python3 -m Tensile.TensileGenerateUID path/to/logic.yaml --index 5 --inplace--indexis the localSolutionIndex. Without--inplacethe new canonical0uUID is printed only.Uniqueness CI. New component
hipblaslt_library_uniquenessin.github/scripts/component_ci.pyrunstox -e library-uniqueness(CPU-only, no ROCm). It scans all*.yamlunderLogic/asm_full, decodes0u/0Utext, and fails on duplicate or malformed UIDs (including stored0u0and leftover decimal). MissingSolutionUIDis ignored unlessHIPBLASLT_REQUIRE_SOLUTION_UIDis set; the tox env does not set it, so current YAML without UIDs still passes. The job triggers on Logic YAML, the uniqueness test,tox.ini, andcomponent-ci-library-uniqueness.yml.Out of scope (later PRs): repo-wide YAML backfill to
0u…, LibraryLogic winner assignment,UidMapping/ MatchTable, and bench/API--algo_method uid.Test Plan
ensure_solution_uid/read_solution_uid, and the regenerate CLI (test_tensile_generate_uid.py).0u0,0Uprefix, reject leading zeros / bad characters / uint64 overflow (test_solution_uid_base62.py).test_solution_uid_concurrency.py), plus sequential generation and a 10-winner batch.LibraryIO/TensileMergeLibrarytests that conversion keepsSolutionUIDnext toSolutionIndexwith0ufixtures.asm_full*.yaml(not only*_UserArgs.yaml); fails on duplicates and non-canonical text; does not fail on missing UIDs by default.python -m tox -e library-uniquenessfromprojects/hipblaslt/tensilelite.Test Result
Local uniqueness tox (
pytest --noconfteston the uniqueness test) passed with no shippedSolutionUIDfields (missing UIDs are not a failure). Generator, CLI, and base62 unit tests cover the 40+24 layout, canonical0uencode/decode, and regenerate-in-place path. The 8×8 concurrent test is sized so same-millisecond 24-bit birthday risk stays ~0.01%, not the ~0.78% of the old 64×8 batch. GitHub Component CI uniqueness will run when Logic YAML, the uniqueness test,tox.ini, or the uniqueness workflow change.