Repository navigation
test: consolidate memory-pool test helpers and lint bindings pools - #3298
Merged
Merged
Conversation
Follow-up to NVIDIA#3290. The same memory-pool helpers were copied across the cuda_bindings_12, cuda_bindings_13, and cuda_core test suites. Move them into one shared module and delete the copies. No test outcome changes: the conditions behind every skipif and skip are the same. New cuda_python_test_helpers/memory.py: * POOL_SIZE (9 copies, including the cuda_core constant, which now re-exports it). * supports_memory_pools() and supports_managed_memory() (13 copies, including the two Cython test modules). * xfail_graph_mempool_oom() (3 copies of the xfail_on_graph_mempool_oom fixture). Each call site keeps its own out-of-memory check and cleanup. cuda_bindings_12/tests/test_cuda.py also uses the shared driver_version_less_than() instead of its own copy, which behaves the same. cuda_bindings_12/tests/test_cudart.py keeps its local driverVersionLessThan: it does not call cuInit, and the shared one would turn collection-time skips into errors on machines with a driver but no device. cuda_core/tests/helpers/memory.py gains skip_if_mempool_unsupported(), skip_if_managed_mempool_unsupported(), and is_pool_resource(), which replace the per-module _skip_if_no_mempool (6 copies), _skip_without_mempools, _gpu_with_mempool_or_skip, _skip_if_no_managed_mempool (2 copies), and _is_pool (3 copies). Two skip reason strings change wording; their conditions do not.
cuda_bindings_12/tests/test_cuda.py::test_cuda_graphMem_attr creates its own context with cuCtxCreate. When cuGraphAddMemAllocNode fails with CUDA_ERROR_OUT_OF_MEMORY, the test destroys the graph and stream and then xfails, but never destroyed the context, so the context stayed current for the rest of the session. Destroy the context before xfailing. The test still xfails on this path; later tests no longer inherit the leaked context, which can change their outcomes. The bindings 13 v2 test and legacy_api/test_legacy_cuda.py use the device fixture and create no context of their own, so they are not affected.
check_mempool_hygiene.py only covered cuda_core/tests, so the uncapped pools fixed in NVIDIA#3140 and NVIDIA#3290 in the bindings tests could have been reintroduced unnoticed. * DEFAULT_TREES now covers cuda_core/tests, cuda_bindings_12/tests, and cuda_bindings_13/tests. * A new pool-properties rule, with its own error message, reports a MemPoolProps_v1, CUmemPoolProps, CUmemPoolProps_v1, CUmemPoolProps_st, or cudaMemPoolProps assigned to a name when the same function (or module, for module-level code) never assigns <name>.max_size or <name>.maxSize. The existing cuda_core rule and message are unchanged. The uncapped-pool-ok marker opts a construction out of either rule. * Graph alloc-node params.poolProps fields are not constructor calls and are not checked; that pool cannot be capped anyway. * .pyx tests are out of scope because ast cannot parse Cython; the module docstring and cuda_bindings_13/AGENTS.md say so. * The check-mempool-hygiene pre-commit hook now also runs on the bindings tests. All current .py tests pass the new rule.
juenglin
added
test
Contributor
Fold the pool-props check into one recursive _pool_props_violations that walks each scope once: it collects the caps and the constructions in the same pass and recurses into nested scopes, replacing the separate _capped_variables and _assigned_pool_props helpers and the double walk. Add a _report helper so the two rule outputs no longer duplicate the print/bullet/hint block. Same rules, same messages; all lint tests pass.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Shared memory test helpers cuda_python_test_helpers/cuda_python_test_helpers/memory.py, cuda_core/tests/helpers/memory.py, cuda_core/tests/helpers/constants.py, cuda_core/tests/AGENTS.md, cuda_bindings_13/AGENTS.md |
Shared modules now provide pool size, capability checks, graph-OOM handling, skip helpers, and pool-resource classification. CUDA Core re-exports the shared pool-size constant. Test guidance describes helper use and pool caps. |
Pool-hygiene enforcement .pre-commit-config.yaml, ci/tools/check_mempool_hygiene.py, ci/tools/tests/test_check_mempool_hygiene.py |
The hook scans Python tests in CUDA Core and bindings trees. The checker reports uncapped resource constructors and named pool-properties objects as separate rules. Tests cover cap assignment, scope, opt-out, and diagnostics. |
CUDA Core test migration cuda_core/tests/graph/*, cuda_core/tests/helpers/*, cuda_core/tests/memory/*, cuda_core/tests/memory_ipc/*, cuda_core/tests/test_object_protocols.py, cuda_core/tests/test_rlcompleter_patch.py |
Graph and memory tests replace local capability checks and resource predicates with shared helpers. |
CUDA bindings test migration cuda_bindings_12/tests/*, cuda_bindings_13/tests/* |
Bindings tests replace local pool constants and capability checks with shared helpers. Graph OOM handling uses the shared helper. CUDA 12 adds null-handle failure tests for cuKernelGetName and cuFuncGetName. |
Suggested reviewers: andy-jost
Priority:
Change: Other
Merge Risk: 
1c7ec
-
The previously identified reassignment gap is fixed, and the expanded check covers Python tests in both bindings trees. No identified issue remains to block merging after normal checks.
Comment @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
-
-
- Source mode: Auto
- Repository: NVIDIA/cuda-python
- Branch: main
- File: .coderabbit.yaml
- Commit: 03b4153
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Enterprise
- Run ID:
a960756c-2a3e-4c6a-aea8-205c8c749bf8
-
.pre-commit-config.yamlci/tools/check_mempool_hygiene.pyci/tools/tests/test_check_mempool_hygiene.pycuda_bindings_12/tests/cython/test_interoperability_cython.pyxcuda_bindings_12/tests/test_cuda.pycuda_bindings_12/tests/test_cudart.pycuda_bindings_12/tests/test_interoperability.pycuda_bindings_13/AGENTS.mdcuda_bindings_13/tests/cython/test_interoperability_cython.pyxcuda_bindings_13/tests/legacy_api/test_legacy_cuda.pycuda_bindings_13/tests/legacy_api/test_legacy_interoperability.pycuda_bindings_13/tests/test_cuda.pycuda_bindings_13/tests/test_cudart.pycuda_bindings_13/tests/test_interoperability.pycuda_core/tests/AGENTS.mdcuda_core/tests/graph/test_graph_definition.pycuda_core/tests/graph/test_graph_definition_errors.pycuda_core/tests/graph/test_graph_definition_integration.pycuda_core/tests/graph/test_graph_definition_lifetime.pycuda_core/tests/helpers/constants.pycuda_core/tests/helpers/memory.pycuda_core/tests/memory/test_resource_live_count.pycuda_core/tests/memory_ipc/test_errors.pycuda_core/tests/memory_ipc/test_memory_ipc.pycuda_core/tests/memory_ipc/test_serialize.pycuda_core/tests/test_object_protocols.pycuda_core/tests/test_rlcompleter_patch.pycuda_python_test_helpers/cuda_python_test_helpers/memory.py
Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.
-
Finding 1 (minor): a type-annotated construction ``props: cuda.CUmemPoolProps = cuda.CUmemPoolProps()`` produces an ast.AnnAssign, not ast.Assign, so the rule never saw the constructor. Extend the construction detection to cover AnnAssign. Finding 2 (major): when the same variable is assigned twice, ``props = CUmemPoolProps()`` # capped ``props = CUmemPoolProps()`` # uncapped, but lint was silent the old cap remained in the shared `capped` set and excused the second construction. Fix: process nodes in source order and track the latest uncapped construction per variable name. When the variable is reassigned, the old cap is reset; the new construction must have its own cap. Any construction still pending at end-of-scope is reported. Also change _walk_scope to yield nodes in source order (previously reversed) so the forward-pass cap-state tracking is order-correct. Adds targeted tests for both findings.
Contributor
Author
|
/ok to test 1c7ec39 |
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 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.
Description
Follow-up to #3290: consolidates duplicated memory-pool test helpers and extends the uncapped-pool lint to the bindings tests.
POOL_SIZE,supports_memory_pools(),supports_managed_memory()andxfail_graph_mempool_oom()move intocuda_python_test_helpers/memory.py, replacing copies acrosscuda_bindings_12,cuda_bindings_13(includinglegacy_apiand the Cython tests) andcuda_core.cuda_core/tests/helpers/memory.pygainsskip_if_mempool_unsupported(),skip_if_managed_mempool_unsupported()andis_pool_resource()for the per-module skip and pool-check copies. Two skip reason strings change wording; no condition changes.cuda_bindings_12/tests/test_cudart.pykeeps its localdriverVersionLessThanbecause it does not callcuInit.cuda_bindings_12test_cuda_graphMem_attrleaked its context when it xfailed. It still xfails, but later tests no longer inherit the leaked context, which can change their outcomes.check_mempool_hygiene.pytocuda_bindings_12/testsandcuda_bindings_13/tests. A new rule, with its own message, reports a pool props object (MemPoolProps_v1,CUmemPoolProps,cudaMemPoolProps, ...) that never getsmax_size/maxSizeset in the same function..pyxtests are not covered becauseastcannot parse Cython.Not changed: the bindings 12 interop tests still use the uncapped default pool (the bindings 13 versions were converted in #3140); converting them would change what they cover.
Testing: no usable GPU was available. Per-test outcomes of the touched bindings 13 and
cuda_coremodules are identical tomain(all error or skip at setup on both). The bindings 13 Cython tests build and import, the bindings 12 modules byte-compile, and the lint tests and pre-commit pass. GPU CI is the real check for commit 1.Checklist