Repository navigation
fix(tensilelite): stop coverage ratchet update from lowering floors - #10596
Conversation
write_baseline() rewrote every entry from the current coverage.json with no comparison against the committed floors, so an update run made to move one file also reset all the others to that run's numbers, lowering some. The gate's own failure message suggested exactly that command, which made the tool's remediation path the fastest way to weaken the tool. tox.ini, the module docstring and the characterization README all already claimed floors are never lowered to go green. update now only strengthens the baseline: a floor rises to the current number, a new file is pinned, and a floor that would fall is held. Lowering one requires naming it with --allow-lower, and the remediation text emits one such flag per offending file so a red CI log cannot be pasted into a blanket reset. ISSUE ID : #10591 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. |
There was a problem hiding this comment.
Pull request overview
This PR fixes TensileLite’s per-file coverage “ratchet” so that coverage_ratchet.py update can no longer silently lower unrelated per-file floors when run to adjust a single file. It makes update strictly one-way by default (raise/pin only), and requires an explicit per-file opt-in (--allow-lower PATH, repeatable) to lower any floor, with updated remediation output and documentation to match.
Changes:
- Implement
ratchet_floors()to compute updated floors while refusing unintended decreases unless explicitly allowed via--allow-lower. - Replace the constant remediation string with a
remediation(paths)helper that prints a copy/paste command listing only the regressed files. - Add unit tests covering the new
updatesemantics and update the characterization README instructions.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| projects/hipblaslt/tensilelite/Tensile/Tests/unit/characterization/tools/coverage_ratchet.py | Implements “update never lowers floors unless named” behavior, adds --allow-lower, and updates remediation output. |
| projects/hipblaslt/tensilelite/Tensile/Tests/unit/characterization/tools/test_coverage_ratchet.py | Adds unit tests validating refusal behavior, per-file lowering opt-in, and remediation text output. |
| projects/hipblaslt/tensilelite/Tensile/Tests/unit/characterization/README.md | Updates documentation to reflect refusal-by-default and explicit --allow-lower workflow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #10596 +/- ##
===========================================
- Coverage 69.36% 68.98% -0.39%
===========================================
Files 2778 2767 -11
Lines 458114 455972 -2142
Branches 67566 67227 -339
===========================================
- Hits 317756 314509 -3247
- Misses 117218 118543 +1325
+ Partials 23140 22920 -220
*This pull request uses carry forward flags. Click here to find out more. 🚀 New features to boost your workflow:
|
|
If I understand correctly, this PR's main change is to make it a requirement for anyone running the ratchet script to explicitly list all files whose new floors will go down, and not up? That seems sensible. Indeed, without the asymmetry introduced here it is not a ratchet (a distinctly asymmetrical tool!). I do have one concern, although it is not introduced in this PR. I see 2 orthogonal things being mixed up in this tooling:
With the current flow, a developer who wants to accept an intentional regression (2) is forced to ratchet up the other floors (1). I think the ratcheting up and the accepting a lowering of floor are separate concerns. I might be missing something though. I will get an agent review here shortly, but overall this PR looks good to me! |
|
Agent review: https://github.com/newling/agent-public-reviews/blob/main/rocm-libraries/review_pr10596.md |
…10595) ISSUE ID : #10591 ## Motivation `TensileLite coverage / Linux` has failed on every tensilelite PR since #9410 landed, on a file none of those PRs touch. The baseline holds `Tensile/Components/Subtile/SubtileGREmit.py` at 73.39%, develop measures it at 70.83%, and the 2.56 pp gap is past the 1 pp tolerance. Issue #10591 has the full trace, including the four unrelated PRs showing a byte-identical failure. This is not something a re-run can clear: the drift is on develop, and a re-run replays the same merge SHA. The lane is advisory rather than required, so nothing is blocked by it, but it is red on every affected PR and that is worth clearing rather than teaching people to scroll past. Please read this as what it is: approving a 2.56 pp coverage reduction on `SubtileGREmit.py`. Whether that drop should instead be bought back with tests is a component call rather than a CI one, and @WorldofKerry has a follow-up planned in #10591. When those tests land the floor ratchets back up on its own, which is the mechanism working as intended. ## Technical Details One entry changed, `SubtileGREmit.py` 73.39 -> 70.83. Nothing else in the baseline moves. The number is develop's own measurement, taken from the `TensileLite coverage` job on #9410's merge commit: https://github.com/ROCm/rocm-libraries/actions/runs/31121977279/job/92714351091. It came from the CI log rather than a local run because the lane needs Linux plus a built `rocisa`. This PR's own coverage run is the check on it: if 70.83 is wrong, the gate says so. The entry was edited directly rather than by running `coverage_ratchet.py update`, which is what the gate's failure message currently suggests. That command rewrites every entry from whatever `coverage.json` is on disk, so it would also reset unrelated files to that run's numbers and quietly lower some of them. #10596 fixes the tool so `update` can no longer do that; this PR deliberately touches only the one number so the diff is the whole review. ## Test Plan - The `TensileLite coverage / Linux` lane on this PR. It re-measures `SubtileGREmit.py` from source and compares against this baseline, so a green run is the direct check that 70.83 is the right number. - No other test level applies. This changes one datum in a CI baseline file: no product code, no kernel generation, no device or architecture impact, so the CPU-only coverage lane is both the appropriate and the only relevant coverage. ## Test Result Pending the PR's own coverage run. ## Submission Checklist - [ ] Look over the contributing guidelines at https://github.com/ROCm/ROCm/blob/develop/CONTRIBUTING.md#pull-requests. ## Risk level None to low (1). A single number in a CI baseline file. It cannot affect built artifacts or runtime behavior; the only thing it changes is where one file's coverage gate sits. The risk it does carry is the one named above: it accepts a real coverage reduction rather than covering it with tests, which is why the drop is called out explicitly rather than buried in a diff. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
…aths Address the two actionable items from the agent review on this PR: ratchet_floors() now takes the same tolerance check uses, so an in-tolerance dip is held silently instead of forcing a second --allow-lower the printed remediation command never named. remediation() now shell-quotes each path and uses --allow-lower=<value>, so a path with a space, a leading hyphen, or shell metacharacters can't corrupt or hijack the copy-pasted command. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for the review, both the design note and the agent review's two actionable items are solid catches. Pushed a fix for both:
Added unit tests for both (in-tolerance dip held without a refusal, a real regression still refused, an end-to-end check→update round trip, and quoting cases for space/hyphen/metacharacter paths). All 43 tests pass. On your separate design point about ratcheting-up vs. accepting-an-intentional-regression being different concerns: agreed that's real, but I'd like to treat it as its own follow-up rather than grow this PR further, similar to the floor-deletion-on-partial-report gap already called out in the description. |
makes sense to me |
newling
left a comment
newling
left a comment
There was a problem hiding this comment.
LGTM, but I suggest letting @davidd-amd have a look at this too as he's most familiar with it
ISSUE ID : #10591
JIRA ID : AIHPBLAS-3878
Motivation
coverage_ratchet.py updateis not a ratchet.write_baseline()rewrites every entry from whatevercoverage.jsonis on disk, with no comparison against the floors already committed, so anupdaterun made to move one file also resets every other file to that run's numbers. Some of those move down. The gate's own failure message tells you to run exactly that command, which makes the tool's remediation path the fastest way to weaken the tool.That contradicts what the ratchet is documented to do in three places:
tox.inisays the floors "are never lowered to go green", and the module docstring and the characterization README both describe a one-way mechanism. This makes the code match the contract.It came up while clearing the stale
SubtileGREmit.pyfloor in #10591, where the documented fix would have silently lowered unrelated floors as a side effect. It matters more the momentTensileLite coveragebecomes a required check, because being blocked is what makes someone reach for the remediation command at speed. Today the lane is advisory, so this is a fix rather than a scramble.Technical Details
updatecan now only strengthen the baseline. Newratchet_floors()takes the committed floors plus the current report and returns the new floors and any it refused to move. A floor rises to the current number, a file with no floor yet is pinned at its current number, and a floor that would fall is held. Percentages are rounded to the two decimals the baseline is written at before being compared, so a difference the file cannot represent is not treated as a fall.Lowering stays possible, but it has to be named.
--allow-lower PATH(repeatable) lowers exactly the files listed and nothing else. Without it,cmd_updatewrites nothing, exits 1, and prints the offending files with their floor, current value and delta.The remediation text can no longer be pasted into a blanket reset.
REMEDIATIONwas a constant suggesting a bareupdate --current coverage.json; it is nowremediation(paths), emitting one--allow-lowerline per offending file. Bothcheckandupdateprint it, so the command a developer copies out of a red CI log can only touch the files that actually regressed. An--allow-lowernaming a path that is not in the report warns instead of passing silently, since a typo there would otherwise look like the drop had been reviewed.Docs move with the tool. The README's "Raising the floors" steps described the old blanket behavior and now describe the refusal and the named-file opt-in.
No CI job invokes
update;coverage-gaterunscheckonly, and the Math CItensilelite-unit-codecovjob runscoverage-unitwithout the gate. So no automation changes behavior.One related gap is deliberately left alone:
updatestill drops the floor for a file absent from the report, so running it against a partial report would delete those floors. Same family, but it needs its own opt-out and would grow this diff past one logical change.Test Plan
test_coverage_ratchet.py: a floor rises to current, a floor is held when current is lower, a new file is pinned, a named file is lowered, sub-precision dips are not falls, refusals sort biggest-drop-first, and the remediation text names every offender and never emits a bareupdate.test_lowering_one_file_does_not_lower_another: with one file named in--allow-lowerand a second also measuring low, the second file's floor must not move.cmd_updateandmain: a refusal writes nothing and returns 1,--allow-lowerlowers only the named file, repeated flags parse, and an unknown path warns.Test Result
Reproduction against the pre-change tool, identical inputs both times. Starting floors
{named.py: 90.0, bystander.py: 88.0}, coverage on disknamed.py=70.0, bystander.py=60.0, loweringnamed.pyon purpose:After the change the run refuses, writes nothing, and prints the command that lowers only
bystander.py.Unit suite, 36 tests (24 pre-existing, 12 new), all passing:
Run locally on Windows with
--noconftest, because the suite's conftest importsrocisa, which this machine cannot build. CI runs the file normally in the-m unitlane, so that caveat is local only.Note on this PR's CI:
TensileLite coverage / Linuxwill stay red until the sibling baseline PR #10595 lands, because develop'sSubtileGREmit.pyfloor is still stale. That failure is pre-existing, is the subject of #10591, and is unrelated to this diff. The unit tests above run in the same lane and are the signal to read here.Submission Checklist
Risk level
Low (2). Test infrastructure only, with a blast radius of one CI lane and one manual maintenance command. The change makes that command stricter, so the failure mode is a refusal that names its own fix rather than a silently weakened gate. The one behavior change a user will notice is that an
updatewhich used to succeed and quietly lower floors now exits 1 and asks which file you meant.