You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up to #2701 addressing review feedback and the test-guidance cleanup noted in cleanup-importorskip-dead-code.md.
Remove test_close_while_mapped_passes_stream_override (xfail from cuda.core: Fix graphics tests #2701); the stream-forwarding behavior of Buffer.close() is covered elsewhere, and the test asserted on internal call dispatch via patch.object on an immutable Cython type.
Fix a partial-resource leak in the GL setup helpers (_setup_gl_buffer/_setup_gl_texture in cuda_core and cuda_bindings): a failure after the window opens now closes the window before re-raising.
Remove dead-code pytest.importorskip calls for declared dependencies: Cython/setuptools in test_build_hooks.py and cuda.pathfinder in test_device_launch.py, replaced with top-level imports so a missing install fails collection.
Narrow the GL setup except Exception to pyglet's "GL unavailable" exceptions (matched by name to avoid pyglet's import-time shadow-window side effect), so a bug in our own setup code re-raises instead of being hidden as a skip.
Narrow _compile_heat_kernels/_compile_bisect_kernels in test_graph_definition_integration.py from bare except Exception to a narrow match on NVRTC's cudaGraphConditionalHandle is undefined diagnostic, so a real compile error fails instead of skipping.
Checklist
New or existing tests cover these changes.
The documentation is up to date with these changes.
Overall, this is a useful follow-up to PR 2701: removing dead importorskip calls, deleting the permanently-xfailed implementation-detail test, and narrowing broad exception handling are all good directions. I do have several requested changes, primarily around where the new availability classifiers draw their boundaries.
Findings
High: GLException still hides real setup regressions
cuda_core/tests/test_graphics.py:65 and cuda_bindings/tests/test_graphics_apis.py:29 classify every pyglet GLException as "GL unavailable." The surrounding catches include the actual glGen*, glBind*, glBufferData, and glTexImage2D calls.
Pyglet's default GL error checking raises GLException after any GL call that reports an error, including GL_INVALID_ENUM, GL_INVALID_VALUE, and GL_INVALID_OPERATION. Consequently, an incorrect constant, argument, or context-state regression in our test setup is converted into a skip, which conflicts with this PR's stated goal of surfacing bugs in our own setup code.
Please restrict "GL unavailable" classification to context/window creation. Once a context exists, errors from buffer or texture allocation should propagate and fail the test. If GLException("No GL context; create a Window first") must be supported as an availability case, match that exact condition rather than every GLException.
High: the nvfatbin availability probe misses the actual loader failures
cuda_core/tests/test_module.py:49 catches only nvfatbin.nvFatbinError around nvfatbin.version(). Loading is lazy, however:
An absent libnvfatbin raises cuda.pathfinder.DynamicLibNotFoundError.
An absent nvFatbinVersion symbol raises cuda.bindings._internal.utils.FunctionNotFoundError.
Neither is an nvfatbin.nvFatbinError.
Because _is_nvfatbin_available() is evaluated while constructing the marker at cuda_core/tests/test_module.py:54, either normal unavailability case aborts collection of the entire module instead of skipping the two nvfatbin-dependent tests.
Please catch the exact library/symbol-unavailable exceptions and allow genuine nvfatbin API-status failures to propagate. The newly added example in cuda_core/tests/AGENTS.md:145-153 repeats the same incorrect catch and should be fixed at the same time.
High: the NVRTC version probe can also abort collection
cuda_core/tests/test_program.py:57 has two analogous problems:
Missing libnvrtc or nvrtcVersion raises DynamicLibNotFoundError or FunctionNotFoundError, neither of which is a CUDAError.
If the suppressed import at lines 41-42 fails, nvrtc remains unbound; line 54 then raises NameError, not AttributeError.
The PCH marker is evaluated at module import time, so these paths abort collection instead of reporting PCH/NVRTC as unavailable.
Please replace the suppressed import with an explicit sentinel established under a narrow import catch, then catch the exact dynamic-library and missing-symbol exceptions around nvrtcVersion(). A CUDAError returned by a successfully loaded version API should be considered separately rather than conflated with "library unavailable."
Medium: supports_ipc_mempool still turns all driver failures into skips
cuda_core/tests/helpers/__init__.py:52 now catches CUDAError rather than Exception, but it still maps every failure from cuInit or cuDeviceGetAttribute to False. Callers interpret False as an unsupported capability and skip.
Unsupported handle types are normally represented by a successful attribute query whose bitmask lacks the POSIX-FD bit. Errors such as an invalid device, invalid attribute, deinitialized driver, or another driver regression should not silently become "unsupported." Please let query failures propagate, or classify only a specifically documented availability result by inspecting the raw CUresult.
Medium: genuine Linux GL-library unavailability is not recognized
The new predicate only recognizes pyglet-namespaced exception classes and the Windows opengl32 built-in exceptions. On Linux, pyglet raises a built-in ImportError('Library "GL" not found.') or ImportError('Library "EGL" not found.') when those system libraries cannot be loaded.
Those are genuine "GL unavailable" setup outcomes, but they now fail the tests. The existing ctypes.util.find_library("EGL") preflight covers only the simplest headless case; it does not cover a visible-display path without libGL or a library that is discoverable but cannot actually be loaded.
Please recognize the exact pyglet GL/EGL loader messages, or perform a reliable load preflight. Do not catch arbitrary ImportError.
In both implementations, win.switch_to() can fail after the window has been constructed but before the protected cleanup region can access it.
If glGenBuffers or glGenTextures succeeds and a later setup call fails, the generated GL object is not deleted.
In headless/shared-context mode there is no window close to release that object; even in the windowed path, pyglet's shadow/shared context can keep it alive.
Please initialize the window and GL object handles before one encompassing protected region, delete any generated object before closing the window, and make _open_gl_window() close a constructed window if switch_to() fails.
Relevant locations are cuda_core/tests/test_graphics.py:90, cuda_core/tests/test_graphics.py:105, cuda_core/tests/test_graphics.py:128, and cuda_bindings/tests/test_graphics_apis.py:54.
Low: conditional-handle diagnostic matching is duplicated
cuda_core/tests/graph/test_graph_definition_integration.py:124 adds another conditional-handle diagnostic/skip implementation even though cuda_core/tests/helpers/graph_kernels.py:79 already implements the same policy.
The copies already differ:
One catches CUDAError while the other catches NVRTCError.
One accepts two diagnostic spellings while the other accepts one.
Please extract a shared NVRTCError classifier/skip helper into helpers/graph_kernels.py and reuse it. If that is intentionally deferred, add reciprocal keep-in-sync comments.
Requested GL helper consolidation
Please consolidate _GL_UNAVAILABLE_EXC_NAMES and _is_gl_unavailable rather than keeping two copies.
Both cuda_bindings and cuda_core test environments already depend on cuda-python-test-helpers, and both test conftest.py files contain source-tree fallback wiring for it. This avoids coupling the bindings tests to cuda_core/tests/helpers and does not introduce a package dependency cycle.
The shared helper should not import pyglet. It can classify by exception module/name and tightly matched built-in loader errors, preserving the requirement that pyglet.gl and pyglet.window not be imported until after headless mode is configured.
This intentionally omits the broad GLException entry. If the exact "No GL context" GLException is observed during context establishment and needs to skip, add a narrow message check for that case only.
The callers should also stage setup so the predicate is used only around context/window establishment:
try:
win=_open_gl_window()
exceptExceptionasexc:
ifis_gl_context_unavailable(exc):
pytest.skip(f"Could not create GL context: {type(exc).__name__}: {exc}")
raise# GL object allocation is outside the unavailable-context catch.# GLException from these calls must fail the test.buf_id=_allocate_gl_buffer(...)
Please add focused, environment-independent tests for the shared predicate:
Reject unrelated TypeError, AttributeError, ImportError, and FileNotFoundError.
Reject an allocation-time GLException such as an invalid-enum error.
The example at cuda_core/tests/AGENTS.md:106 should then reference this shared helper rather than showing exception names that do not match the implementation. If consolidation is declined, the fallback is reciprocal comments naming the exact other file, for example:
# Keep in sync with cuda_bindings/tests/test_graphics_apis.py.
and:
# Keep in sync with cuda_core/tests/test_graphics.py.
Other reviewed changes
The following changes looked good:
Replacing the declared-dependency importorskip calls for Cython, setuptools, and cuda-pathfinder.
Removing the permanently-xfailed stream-dispatch implementation-detail test.
Narrowing the conditional-kernel compile skip to specific diagnostics, subject to sharing the classifier noted above.
The remaining graph, build-hook, module, and program test edits outside the findings above.
Validation
Reviewed all nine files in the seven-commit PR diff from merge base db94059.
git diff --check passes.
The current GitHub Actions build, GPU-test, docs, API, security, and pre-commit matrix is green.
I did not run pytest locally because this checkout has no repository-standard TestVenv.
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
cuda.bindingsEverything related to the cuda.bindings modulecuda.coreEverything related to the cuda.core moduleP1Medium priority - Should dotestImprovements or additions to tests
2 participants
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 #2701 addressing review feedback and the test-guidance cleanup noted in
cleanup-importorskip-dead-code.md.test_close_while_mapped_passes_stream_override(xfail from cuda.core: Fix graphics tests #2701); the stream-forwarding behavior ofBuffer.close()is covered elsewhere, and the test asserted on internal call dispatch viapatch.objecton an immutable Cython type._setup_gl_buffer/_setup_gl_textureincuda_coreandcuda_bindings): a failure after the window opens now closes the window before re-raising.pytest.importorskipcalls for declared dependencies:Cython/setuptoolsintest_build_hooks.pyandcuda.pathfinderintest_device_launch.py, replaced with top-level imports so a missing install fails collection.except Exceptionto pyglet's "GL unavailable" exceptions (matched by name to avoid pyglet's import-time shadow-window side effect), so a bug in our own setup code re-raises instead of being hidden as a skip._compile_heat_kernels/_compile_bisect_kernelsintest_graph_definition_integration.pyfrom bareexcept Exceptionto a narrow match on NVRTC'scudaGraphConditionalHandle is undefineddiagnostic, so a real compile error fails instead of skipping.Checklist