Uh oh!
There was an error while loading. Please reload this page.
fix(ci): attach the uncapped-pool opt-out marker to the call it exempts - #2543
Open
LeSingh1 wants to merge 1 commit into
Open
fix(ci): attach the uncapped-pool opt-out marker to the call it exempts#2543LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
`_opted_out` accepted the opt-out marker anywhere on the line above the offending call: start = max(node.lineno - 2, 0) # -1 for 0-based, -1 more for a preceding comment end = getattr(node, "end_lineno", node.lineno) return any(OPT_OUT_MARKER in line for line in lines[start:end]) Nothing requires that line to be a comment, or to have anything to do with the call. So a trailing marker annotating one statement also exempts the statement on the next line: with pytest.raises(RuntimeError): DeviceMemoryResource(dev, DeviceMemoryResourceOptions(ipc_enabled=True)) # uncapped-pool-ok: raises first DeviceMemoryResource(dev, DeviceMemoryResourceOptions()) # <- silently exempt This is the shape a reviewer is least likely to catch, because both lines look correctly annotated. The marker text merely appearing in an unrelated string literal has the same effect: msg = "see uncapped-pool-ok in AGENTS.md" DeviceMemoryResource(dev, DeviceMemoryResourceOptions()) # <- silently exempt Bound the marker to the call's own statement instead. `_iter_calls` walks the tree carrying the chain of `ast.stmt` ancestors, and a marker counts when it is inside the call's statement (start of the statement through the end of the call), on the header of a compound statement containing the call, or on a dedicated comment line immediately above the statement. Every documented placement keeps working -- comment line above, inline on the call, and the `pytest.raises` block form from cuda_core/tests/AGENTS.md. Anchoring on the statement rather than the call also fixes a false positive the old window had, where a marker on the first line of a multi-line construction did not reach the inner options call: mr = DeviceMemoryResource( # uncapped-pool-ok: reason dev, DeviceMemoryResourceOptions(), # <- was reported anyway ) cuda_core/tests has no opt-out that relied on the loose behavior, so the tightened rule leaves the tree clean.
Contributor
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 freeto 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.
Problem
_opted_outaccepts the opt-out marker anywhere on the line above the offending call:Nothing requires that preceding line to be a comment, or to have anything to do with the call. So a trailing marker that annotates one statement also exempts the statement on the next line:
This is the shape a reviewer is least likely to catch: both lines look correctly annotated, and the second one reserves the full address-space window that this check exists to prevent.
The marker text merely appearing in an unrelated string has the same effect:
Both verified against
violations_in()onmain:[]in each case, while the identical call with no marker anywhere is correctly reported.Fix
Bind the marker to the call's own statement.
_iter_callswalks the tree carrying the chain ofast.stmtancestors, and a marker counts when it is:with,def,for, ...), so a marker on a block header still annotates the calls in that block;A marker anywhere else no longer counts.
Every documented placement keeps working, including the
pytest.raisesform fromcuda_core/tests/AGENTS.md. Anchoring on the statement rather than the call also fixes a false positive the old window had — a marker on the first line of a multi-line construction never reached the inner options call:Also documented the placement rule in
cuda_core/tests/AGENTS.md, which is what contributors read.Effect on the tree
None: no opt-out in
cuda_core/testsrelied on the loose behavior. The existingtest_the_live_test_suite_is_clean(which runsmain([])over the real tree) still passes.Tests
Two parametrized tests appended to
ci/tools/tests/test_check_mempool_hygiene.py:test_marker_that_does_not_annotate_the_call_does_not_suppress_it— trailing marker on the previous statement, marker inside an unrelated string, comment detached by a blank line.test_marker_attached_to_the_call_still_suppresses_it— comment above, indented comment above, inline on the call, continuation line of the same call, first line of a multi-line statement, containingwithheader, containingdefheader.Verification
Executed in full (pure AST analysis, no GPU):
(The third is the false positive described above — it fails on
mainfor the opposite reason.)ruff check/ruff format --checkclean on both changed Python files;python -m py_compileclean. Verified index-safely (cpaside,git show upstream/main:<path> >, run, restore) — no staged reverts.