Skip to content

cuda.core: fix a few test issues - #2714

Open
juenglin wants to merge 7 commits into
NVIDIA:mainfrom
juenglin:graphics-tests-followup
Open

cuda.core: fix a few test issues#2714
juenglin wants to merge 7 commits into
NVIDIA:mainfrom
juenglin:graphics-tests-followup

Conversation

@juenglin

@juenglin juenglin commented Aug 27, 2026

Copy link
Copy Markdown
Contributor

Description

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.

@copy-pr-bot

copy-pr-bot Bot commented Aug 27, 2026

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.

@github-actions github-actions Bot added cuda.bindings Everything related to the cuda.bindings module cuda.core Everything related to the cuda.core module labels Aug 27, 2026
assert buf.handle == 0


@pytest.mark.xfail(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing per PR #2701 review feedback.

@juenglin juenglin added P1 Medium priority - Should do test Improvements or additions to tests labels Aug 27, 2026
@juenglin juenglin added this to the cuda.core 1.2.0 milestone Aug 27, 2026
@juenglin
juenglin force-pushed the graphics-tests-followup branch from 1c5bd29 to 2b149cf Compare August 27, 2026 21:53
@juenglin
juenglin force-pushed the graphics-tests-followup branch from 2b149cf to 92eb2ee Compare August 27, 2026 22:02
@juenglin

Copy link
Copy Markdown
Contributor Author

/ok to test 6854ccb

@juenglin
juenglin requested review from Andy-Jost and rwgk August 27, 2026 22:16
@juenglin
juenglin marked this pull request as ready for review August 27, 2026 22:17
@juenglin juenglin self-assigned this Aug 27, 2026
@juenglin
juenglin force-pushed the graphics-tests-followup branch from 6854ccb to 9a28ab6 Compare August 27, 2026 23:00
@juenglin

Copy link
Copy Markdown
Contributor Author

/ok to test 9a28ab6

@github-actions

Copy link
Copy Markdown

@juenglin
juenglin force-pushed the graphics-tests-followup branch from 9a28ab6 to f05c176 Compare August 27, 2026 23:31
@juenglin

Copy link
Copy Markdown
Contributor Author

/ok to test f05c176

@rwgk

rwgk commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

PR 2714 initial feedback

codex gpt-5.6-sol ultra, with very minor edits

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.

Reference: https://github.com/pyglet/pyglet/blob/v2.1.14/pyglet/gl/lib.py

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.

Reference: https://github.com/pyglet/pyglet/blob/v2.1.14/pyglet/lib.py

Low: partial GL resource cleanup remains incomplete

The new cleanup begins too late:

  • 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.

The natural home is a new dependency-free module:

cuda_python_test_helpers/cuda_python_test_helpers/graphics.py

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.

A suggested shape is:

# cuda_python_test_helpers/graphics.py

_GL_CONTEXT_UNAVAILABLE_EXC_NAMES = frozenset(
    {
        "NoSuchDisplayException",
        "NoSuchConfigException",
        "NoSuchScreenModeException",
        "WindowException",
        "ContextException",
    }
)

_PYGLET_GL_LIBRARY_IMPORT_ERRORS = frozenset(
    {
        'Library "GL" not found.',
        'Library "EGL" not found.',
    }
)


def is_gl_context_unavailable(exc: BaseException) -> bool:
    exc_type = type(exc)
    if (
        exc_type.__module__.startswith("pyglet")
        and exc_type.__name__ in _GL_CONTEXT_UNAVAILABLE_EXC_NAMES
    ):
        return True

    if (
        isinstance(exc, (FileNotFoundError, AttributeError))
        and "opengl32" in str(exc).lower()
    ):
        return True

    return (
        isinstance(exc, ImportError)
        and str(exc) in _PYGLET_GL_LIBRARY_IMPORT_ERRORS
    )

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()
except Exception as exc:
    if is_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:

  • Accept representative pyglet context/window exceptions.
  • Accept the exact Windows opengl32 failures.
  • Accept the exact Linux GL/EGL loader failures.
  • 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.

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

Labels

cuda.bindings Everything related to the cuda.bindings module cuda.core Everything related to the cuda.core module P1 Medium priority - Should do test Improvements or additions to tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants