cuda.core: fix a few test issues - #2714
Conversation
2b149cf to
92eb2ee
Compare
|
/ok to test 6854ccb |
6854ccb to
9a28ab6
Compare
|
/ok to test 9a28ab6 |
This comment has been minimized.
This comment has been minimized.
9a28ab6 to
f05c176
Compare
|
/ok to test f05c176 |
PR 2714 initial feedbackcodex gpt-5.6-sol ultra, with very minor edits Overall, this is a useful follow-up to PR 2701: removing dead FindingsHigh:
|
Per PR NVIDIA#2714 review (Ralf K): GLException is pyglet's generic GL-error class, raised after any GL call that reports an error (GL_INVALID_ENUM, etc.). Including it in the "GL unavailable" set hid real bugs in our GL allocation code as skips. Drop GLException from _GL_UNAVAILABLE_EXC_NAMES and restructure the helpers so the skip catch wraps only context/window creation (_open_gl_window). GL object allocation (_allocate_gl_buffer / _allocate_gl_texture) runs outside the catch, so a GLException from allocation propagates and fails the test.
Per PR NVIDIA#2714 review (Ralf K): on Linux without libGL or libEGL, pyglet raises ImportError('Library "GL" not found.') / ImportError('Library "EGL" not found.') from pyglet/lib.py. These are genuine "GL unavailable" setup outcomes but the predicate only recognized pyglet exceptions and the Windows opengl32 built-in exceptions, so the tests failed on such Linux runners. Recognize the exact pyglet GL/EGL loader messages so a genuine GL setup is skipped. A different ImportError from our own code does not match.
Per PR NVIDIA#2714 review (Ralf K): the GL availability predicate was duplicated in cuda_core/tests/test_graphics.py and cuda_bindings/tests/test_graphics_apis.py. Both test environments already depend on cuda-python-test-helpers. Move the predicate to cuda_python_test_helpers/graphics.py as is_gl_context_unavailable and import it from both test files. The shared helper does not import pyglet (importing pyglet.gl / pyglet.window triggers the shadow-window side effect) and classifies by exception module/name and tightly matched built-in loader errors. Add focused tests for the predicate: accepts the pyglet context/window exceptions, the Windows opengl32 failures, and the Linux GL/EGL loader ImportErrors; rejects unrelated TypeError/AttributeError/ ImportError/FileNotFoundError and an allocation-time GLException.
Per PR NVIDIA#2714 review (Ralf K): _is_nvfatbin_available caught only nvfatbin.nvFatbinError around nvfatbin.version(), but loading is lazy. An absent libnvfatbin raises cuda.pathfinder.DynamicLibNotFoundError; an absent nvFatbinVersion symbol raises cuda.bindings._internal.utils.FunctionNotFoundError. Neither is an nvFatbinError, so normal unavailability aborted collection of the whole module instead of skipping the two nvfatbin-dependent tests. Catch ImportError, DynamicLibNotFoundError, FunctionNotFoundError as "not available" and let nvFatbinError (a genuine API-status failure from a successfully loaded library) propagate. Update the AGENTS.md capability-probe example to match.
Per PR NVIDIA#2714 review (Ralf K): the suppressed import of nvrtc plus the (AttributeError, CUDAError) catch around nvrtcVersion() had two bugs. - Missing libnvrtc raises cuda.pathfinder.DynamicLibNotFoundError; missing nvrtcVersion raises cuda.bindings._internal.utils.FunctionNotFoundError. Neither is a CUDAError. - If the suppressed import fails, nvrtc stays unbound and nvrtc.nvrtcVersion() raises NameError, not AttributeError. The PCH marker is evaluated at module import time, so these aborted collection instead of reporting PCH/NVRTC as unavailable. Replace the suppressed import with an explicit sentinel under a narrow ImportError catch, then catch the exact dynamic-library and missing-symbol exceptions around nvrtcVersion(). A CUDAError from a successfully loaded version API propagates as a real bug.
Per PR NVIDIA#2714 review (Ralf K): supports_ipc_mempool caught CUDAError broadly and returned False, so any cuInit or cuDeviceGetAttribute failure (invalid device, deinitialized driver) was silently treated as "IPC unsupported" and callers skipped. Unsupported handle types are normally represented by a successful query whose bitmask lacks the POSIX-FD bit. Inspect the raw CUresult from cuDeviceGetAttribute and treat only CUDA_ERROR_NOT_SUPPORTED as "unsupported"; let other driver errors propagate via handle_return so a real bug is not hidden as a skip.
Per PR NVIDIA#2714 review (Ralf K): the GL setup cleanup began too late. - _open_gl_window: win.switch_to() could fail after the window was constructed but before the protected cleanup region could access it. - _allocate_gl_buffer / _allocate_gl_texture: if glGen* succeeded and a later setup call failed, the generated GL object was not deleted. - In headless mode there is no window close to release the object. Wrap switch_to() in _open_gl_window so it closes a constructed window on failure. Wrap the GL allocation calls after glGen* in _allocate_gl_buffer / _allocate_gl_texture so they delete the generated object on failure. Initialize buf_id / tex_id to None in the context managers so the finally cleanup does not NameError when allocation raises before returning a handle. Use contextlib.suppress for the best-effort cleanup blocks.
Per PR NVIDIA#2714 review (Ralf K): the conditional-handle diagnostic skip was duplicated between test_graph_definition_integration.py and helpers/graph_kernels.py, and the two copies already differed (one caught CUDAError and accepted two spellings; the other caught NVRTCError and accepted one). Extract skip_if_nvrtc_lacks_conditional_handle into helpers/graph_kernels.py (with the two narrow diagnostic phrases) and reuse it from both compile_conditional_kernels and _compile_heat_kernels / _compile_bisect_kernels. A genuine compile error re-raises so a real bug is not hidden as a skip.
Apply the guidance updates suggested during PR NVIDIA#2714 review: - "Skip only real setup failures": note the example exception names are illustrative, add GLException to an explicit "do not include" list, and list the platform-specific "library not loadable" manifestations (Linux ImportError for GL/EGL, Windows FileNotFoundError/AttributeError for opengl32). - "Capability probes": add an example of a probe that inspects a raw CUresult for a documented availability code (the supports_ipc_mempool fix). - New section "Clean up partial setup on failure": if setup allocates a resource and a later step fails, clean up the partial resource before re-raising. - "Shared test support": search tests/helpers and cuda_python_test_helpers for prior art before adding a new helper; consolidate duplicates across packages into cuda_python_test_helpers.
f05c176 to
b608b64
Compare
|
/ok to test b608b64 |
|
/ok to test 7028efd |
|
The below is from codex again. I'm not sure myself about the "Add mocked tests" suggestion in the last "Low" finding. All the other ones I'd try to resolve, within reason. Re Prior Feedback The requested Findings
|
- Finding 1 (IPC probe): supports_ipc_mempool inspected the raw CUresult and special-cased CUDA_ERROR_NOT_SUPPORTED. Ralf says unsupported handle types are represented by a successful bitmask without the POSIX-FD bit, so the special case is unnecessary. Remove it; let handle_return raise on any non-success and rely on the bitmask check. Update AGENTS.md example to match. - Finding 3 (headless EGL): is_gl_context_unavailable rejected MissingFunctionException (pyglet raises it when libEGL exists but eglQueryDevicesEXT / eglGetPlatformDisplayEXT entry points do not). Add it to the recognized set. - Finding 4 (NVRTC sentinel): test_program caught ImportError for nvrtc but cuda_utils already imports nvrtc at module level, so the ImportError catch was dead code. Import nvrtc directly alongside CUDAError/handle_return and keep only the loader/symbol catches around nvrtcVersion(). - Finding 5 (provenance): move the is_gl_context_unavailable tests from cuda_python_test_helpers/tests/test_graphics.py to cuda_core/tests/test_helpers.py with human_reviewed markers, so CI actually runs them. Drop the source file.
|
/ok to test 3d36ecb |
Thanks, I ignored the suggestions for the mocked tests. |
rwgk
left a comment
There was a problem hiding this comment.
LGTM, there is just one accidental empty line I think.
| `from helpers.memory import create_managed_memory_resource_or_skip`. | ||
| - Search `tests/helpers/` and `cuda_python_test_helpers/` for prior art | ||
| before adding a new helper; consolidate duplicates across | ||
|
|
There was a problem hiding this comment.
Looks like an accidental empty line?
This comment has been minimized.
This comment has been minimized.
1 similar comment
|
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