Skip to content

fix(ci): treat max_size=0 as an uncapped pool in check_mempool_hygiene - #2538

Open
LeSingh1 wants to merge 1 commit into
NVIDIA:mainfrom
LeSingh1:ci-mempool-hygiene-max-size-zero
Open

fix(ci): treat max_size=0 as an uncapped pool in check_mempool_hygiene#2538
LeSingh1 wants to merge 1 commit into
NVIDIA:mainfrom
LeSingh1:ci-mempool-hygiene-max-size-zero

Conversation

@LeSingh1

Copy link
Copy Markdown
Contributor

Problem

ci/tools/check_mempool_hygiene.py enforces the rule in cuda_core/tests/AGENTS.md: a test-created pool without max_size reserves an address-space window sized from installed device memory rather than from what the test allocates, and the whole suite shares one process.

It decides "capped" by looking only for the presence of max_size, never its value:

def_is_capped(node: ast.Call) ->bool:
# ``**kwargs`` (arg is None) may carry max_size; do not guess.returnany(kw.argisNoneorkw.arg=="max_size"forkwinnode.keywords)

But max_size=0 is the default, and CUmemPoolProps.maxSize == 0 asks the driver for its system-dependent size. From DeviceMemoryResourceOptions (and identically PinnedMemoryResourceOptions):

max_size : int, optional — Maximum pool size. When set to 0, defaults to a system-dependent value. (Default to 0)

So these create exactly the uncapped pool the check exists to reject, and every one of them passes today:

DeviceMemoryResource(dev, DeviceMemoryResourceOptions(max_size=0))
DeviceMemoryResource(dev, {"max_size": 0})
PinnedMemoryResource(PinnedMemoryResourceOptions(ipc_enabled=True, max_size=0))

Verified against violations_in() on main: all three return [], while the equivalent DeviceMemoryResourceOptions() is correctly reported.

This is the worst shape for a hole in a lint: max_size=0 is the natural thing to reach for when quieting the hook, it looks like a cap in review, and it reserves the full window anyway.

Fix

Inspect the value and report a literal 0.

  • Non-literals are still accepted, unchanged. A named value is usually the suite-wide POOL_SIZE; the checker does not guess.
  • **kwargs / **other still short-circuit to "capped", unchanged — including {"max_size": 0, **opts}, where the unpacking may override the literal. Not statically decidable, so not reported.
  • The reported message names the actual problem (max_size=0, which is the uncapped default) instead of claiming max_size is missing. The omitted-max_size message is unchanged.

Also updated the corresponding rule in cuda_core/tests/AGENTS.md, which is what contributors read.

Effect on the tree

None: cuda_core/tests has no max_size=0 construction today, so the tightened rule leaves the suite clean. The existing test_the_live_test_suite_is_clean (which runs main([]) over the real tree) still passes.

Tests

ci/tools/tests/test_check_mempool_hygiene.py gains three UNCAPPED cases (kwarg, kwarg among other options, dict literal), two CAPPED regression guards (a non-zero literal, and {"max_size": 0, **opts}), and one assertion on the new message.

Verified with the index-safe swap (cp aside, git show upstream/main:<path> >, run, restore):

# with the fix
20 passed
# with ci/tools/check_mempool_hygiene.py restored from upstream/main
FAILED test_uncapped_pool_is_reported[zero-kwarg]
FAILED test_uncapped_pool_is_reported[zero-kwarg-among-others]
FAILED test_uncapped_pool_is_reported[zero-dict]
FAILED test_zero_cap_message_names_the_zero
4 failed, 16 passed

Full pytest ci/tools/tests: 62 passed. ruff check and ruff format --check clean on both changed files; python -m py_compile clean. No GPU is involved — this tool is pure AST analysis, so everything above was actually executed.

`check_mempool_hygiene.py` exists to stop tests from creating memory pools
whose address-space reservation is sized from installed device memory
instead of from what the test allocates. It decided a pool was capped by
looking only for the *presence* of `max_size`, never at its value.
`max_size=0` is the default and, per `CUmemPoolProps.maxSize`, asks the
driver for its system-dependent size -- "When set to 0, defaults to a
system-dependent value" in both `DeviceMemoryResourceOptions` and
`PinnedMemoryResourceOptions`. So
DeviceMemoryResource(dev, DeviceMemoryResourceOptions(max_size=0))
DeviceMemoryResource(dev, {"max_size": 0})
create exactly the uncapped pool the check is meant to reject, and both
passed the check. The spelling is an easy one to reach for when silencing
the hook, which is the worst case: the annotation looks like a cap and
reserves the full window anyway.
Inspect the value for the literal `0` and report it, with a message that
names the value rather than claiming max_size is missing. Non-literals are
still accepted unchanged -- a named constant is usually the suite-wide
POOL_SIZE, and this checker does not guess -- and neither is `**kwargs` /
`**other`, where the value is not statically decidable.
`cuda_core/tests` has no `max_size=0` construction today, so the tightened
rule leaves the tree clean (`test_the_live_test_suite_is_clean` still
passes).
@copy-pr-bot

Copy link
Copy Markdown
Contributor

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CI/CDCI/CD infrastructurecuda.coreEverything related to the cuda.core module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant

@LeSingh1