fix(hipblaslt): key TensileLogic known-bugs on solution_name#9355
Open
tony-davis wants to merge 5 commits into
Open
fix(hipblaslt): key TensileLogic known-bugs on solution_name#9355tony-davis wants to merge 5 commits into
tony-davis wants to merge 5 commits into
Conversation
The TensileLogic --check-all known-bugs list keyed each documented skip on (path, solution_index). SolutionIndex is positional and shifts whenever the library logic is re-tuned or regenerated, so contributors had to hand-edit known_bugs.yaml on unrelated PRs. Key each skip on the solution's SolutionNameMin instead, a content-derived name that is stable across re-tuning and self-invalidates when the kernel actually changes. Also make the list self-cleaning: known-bug solutions are re-validated instead of blindly skipped, and one that starts passing raises a warning (with an optional --strict-known-bugs gate) so the fixing PR removes its entry. Adds scripts/migrate_known_bugs.py to resolve/verify entries against the on-disk logic, migrates the existing ROCM-7144 entries to names, and updates the unit and characterization tests with an ADR for the intended golden change. AIHPBLAS-4049 Relates: ROCM-7144
The index->name migration of known_bugs.yaml was a one-time change already landed in this branch, so the standalone helper script is no longer needed. Remove it and the doc/ADR references that pointed at it. AIHPBLAS-4049 Co-authored-by: Cursor <cursoragent@cursor.com>
✅ 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. |
tony-davis
changed the title
…own_bugs _runChecks now returns a 5-tuple (keep, total, known_bug_skips, chip_id_failures, stale_known_bugs) and main() unpacks all five, but the test_TensileLogic_Run.py mocks still returned 4-tuples, so the tensilelite-unit lane failed with "not enough values to unpack (expected 5, got 4)". Update the five ParallelMap2 mocks to 5-tuples and add two tests pinning the new stale-entry behavior: strict mode exits 1 when a known-bug now passes, and lenient mode (default) warns only. AIHPBLAS-4049 Co-authored-by: Cursor <cursoragent@cursor.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates TensileLite’s TensileLogic --check-all “known bugs” mechanism to key skip entries by stable SolutionNameMin (solution_name) instead of positional SolutionIndex, and adds detection (and optional strict failure) for “stale” known-bug entries that now pass validation.
Changes:
- Switch known-bugs keys from
(path, solution_index)to(path, solution_name)and reject legacysolution_indexentries. - Re-validate known-bugged solutions to detect stale entries; add
--strict-known-bugsto fail runs when stale entries are present. - Update unit/characterization tests, snapshots, and documentation/ADR to reflect the new behavior.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| projects/hipblaslt/tensilelite/Tensile/Tests/unit/test_TensileLogic_Run.py | Adjusts mocked ParallelMap2 tuple shape; adds tests for strict/lenient stale-known-bugs behavior. |
| projects/hipblaslt/tensilelite/Tensile/Tests/unit/test_KnownBugs.py | Updates known-bugs loader tests to require and use solution_name. |
| projects/hipblaslt/tensilelite/Tensile/Tests/unit/characterization/TensileLogic/test_knownbugs_char.py | Updates characterization tests to use (path, solution_name) keys. |
| projects/hipblaslt/tensilelite/Tensile/Tests/unit/characterization/TensileLogic/snapshots/test_knownbugs_char.ambr | Updates amber snapshots for the changed known-bugs key semantics. |
| projects/hipblaslt/tensilelite/Tensile/Tests/unit/characterization/DECISIONS.md | Records intended golden changes and rationale for the keying behavior update. |
| projects/hipblaslt/tensilelite/Tensile/Tests/unit/characterization/adr/0002-knownbugs-key-on-solution-name.md | New ADR documenting the decision and consequences of keying on solution_name. |
| projects/hipblaslt/tensilelite/Tensile/TensileLogic/Run.py | Implements name-based known-bugs checks, stale detection, aggregation, and strict exit behavior. |
| projects/hipblaslt/tensilelite/Tensile/TensileLogic/ParseArguments.py | Updates --known-bugs help text and adds --strict-known-bugs. |
| projects/hipblaslt/tensilelite/Tensile/TensileLogic/KnownBugs.py | Changes schema to (path, solution_name) and updates loader/matcher accordingly. |
| projects/hipblaslt/tensilelite/Tensile/TensileLogic/known_bugs.yaml | Migrates existing entries to solution_name keys. |
| projects/hipblaslt/scripts/README.md | Updates user-facing documentation for the new known-bugs behavior and strict mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… contract The Run._runChecks/main return contract grew a 5th value (stale_known_bugs), which was updated in the unit suite but not its characterization twin under Tensile/Tests/unit/characterization/TensileLogic/. Update test_run_char.py: the _runChecks unpack helper now takes the 5-tuple, the main-path ParallelMap2 mocks return 5-tuples, and the known-bug test keys on SolutionNameMin (the solution_name scheme) instead of a positional index. Regenerate the three test_parsearguments_char.ambr goldens for the new --strict-known-bugs option (adds StrictKnownBugs=False to the parsed namespace). AIHPBLAS-4049 Co-authored-by: Cursor <cursoragent@cursor.com>
Re-validating a documented known bug called _validateWorkGroupMappingXCC, which increments the module-global _xcc_failures_by_file dedup counter even under redirect_stdout. A still-failing known bug would bump that counter so a later *real* XCC failure in the same file printed only the "... (more solutions in this file)" line and lost its first detailed error message. Add a report=True flag to _validateWorkGroupMappingXCC; when False it neither prints nor touches the counter. The known-bug re-validation path in Run.py now passes report=False. Adds a characterization test pinning the side-effect-free behavior. Thanks to Copilot for catching this. AIHPBLAS-4049 Co-authored-by: Cursor <cursoragent@cursor.com>
Pre-commit check failed⛔ pre-commit failed Please run locally:
This repo uses |
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (76.84%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## develop #9355 +/- ##
===========================================
- Coverage 64.69% 64.60% -0.09%
===========================================
Files 2685 2686 +1
Lines 422661 423266 +605
Branches 62809 62933 +124
===========================================
+ Hits 273417 273431 +14
- Misses 128574 129166 +592
+ Partials 20670 20669 -1
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Summary
The TensileLogic
--check-allknown-bugs list (tensilelite/Tensile/TensileLogic/known_bugs.yaml) keyed each documented skip on(path, solution_index).SolutionIndexis positional, so it shifts whenever the library logic is re-tuned or regenerated, forcing contributors to hand-edit this file on unrelated PRs. This PR keys each skip on the solution'sSolutionNameMininstead (a content-derived name stable across re-tuning), and makes the list self-cleaning so a fixed kernel is detected instead of skipped forever.Risk Assessment
Risk 3. Host-only TensileLite Python touching the pre-build validation gate and its unit/characterization tests; no kernel/codegen or device behavior changes. Residual risk is that the
.ambrcharacterization goldens were hand-edited to match syrupy's format and must be confirmed byte-identical by a-m unitrun with the compiled rocisa module (not available where this was authored).Related
JIRA ID : AIHPBLAS-4049
Related: ROCM-7144 (the gfx950 validation drift these entries document)
Device / Architecture Coverage
Host-only Python change with no device/codegen impact; passing PR CI (the
-m unitlane and the TensileLogic pre-build gate) is sufficient. No device sweep required.Testing Summary
test_KnownBugs.py): name-based keys, schema validation (legacysolution_indexnow rejected). Logic verified locally via a rocisa-free importlib harness.test_knownbugs_char.py+.ambr): updated for the new key scheme; ADR 0002 records the intended golden change.known_bugs.yaml: the 14 ROCM-7144 entries were migrated tosolution_nameand each was confirmed to resolve against the in-repo gfx950 library logic.python -m py_compileon all changed modules.Testing Checklist
-m unitlane incl. characterization goldens (needs rocisa build) - Status: PendingFlags / Guardrails
--strict-known-bugs(TensileLogic, off by default): exits non-zero when a known-bug now passes. Not auto-passed byrun_tensile_logic_check.py, so normal/CMake pre-build runs stay lenient.Adjacent Tests Considered
The characterization
.ambrgoldens undercharacterization/TensileLogic/are the adjacent surface; updated surgically for the two affected nodes only. No client/kernel tests touch this path.Technical Changes
KnownBugs.py,Run.py,ParseArguments.py:(path, solution_name)keys; re-validate known bugs;stale_known_bugscount +--strict-known-bugs.known_bugs.yaml: migrated 14 ROCM-7144 entries tosolution_name.