Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cuda_bindings/tests/test_graphics_apis.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
gl_context_not_on_nvidia_gpu_reason,
is_gl_context_unavailable,
open_gl_window,
tcc_skip_reason,
)

from cuda.bindings import runtime as cudart
Expand Down Expand Up @@ -56,6 +57,8 @@ def _allocate_gl_texture(win):
@contextlib.contextmanager
def _gl_context():
"""Yield ``(tex_id, tex_target)`` with a current GL context, or skip if GL is unavailable."""
if reason := tcc_skip_reason():
pytest.skip(reason)
_configure_pyglet_headless()

try:
Expand Down
9 changes: 9 additions & 0 deletions cuda_core/tests/test_graphics.py

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hm, the test_graphics_apis.py tests do not need the same skip?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@coderabbitai Do you know?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
ast-grep outline cuda_bindings/tests/test_graphics_apis.py --items all
rg -n -C3 'tcc|TCC|open_gl_window|is_gl_context_unavailable|cuGL|cuGraphics|skip|fixture' cuda_bindings/tests/test_graphics_apis.py
fd -i '^graphics\.py$'

Length of output: 2328


🏁 Script executed:

#!/bin/bash
sed -n '55,150p' cuda_bindings/tests/test_graphics_apis.py
ast-grep outline cuda_python_test_helpers/cuda_python_test_helpers/graphics.py --items all
rg -n -A45 -B3 '^def (is_gl_context_unavailable|open_gl_window)\(' cuda_python_test_helpers/cuda_python_test_helpers/graphics.py
rg -n -C3 'cudaDevAttrTccDriver|CU_DEVICE_ATTRIBUTE_TCC_DRIVER|cudaDeviceGetAttribute\(|cudaGetDevice\(' cuda_bindings/tests --glob '*.py'

Length of output: 9291


@juenglin suggestion: Yes—the GL smoke tests need the same early TCC skip.

In cuda_bindings/tests/test_graphics_apis.py, _gl_context() calls open_gl_window() without checking TCC mode. The reported GLException would propagate before the test reaches its cudaErrorOperatingSystem handling. Both parameterizations of test_cuda_gl_register_image_smoketest use this path.

Add the check before _configure_pyglet_headless(). Use cudart.cudaGetDevice() and cudart.cudaDeviceGetAttribute(cudart.cudaDeviceAttr.cudaDevAttrTccDriver, device) to avoid a dependency on cuda.core. Check both return statuses; query failures should fail, not skip.

Keep test_cuda_register_image_invalid enabled. It does not create a GL context and only checks that registration with an invalid handle returns an error.

This conclusion comes from code inspection. I did not test it on TCC hardware.

You are interacting with an AI system.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would it be better to work the skip into is_gl_context_unavailable then?

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
ast-grep outline cuda_python_test_helpers/cuda_python_test_helpers/graphics.py --items all
ast-grep outline cuda_bindings/tests/test_graphics_apis.py --items all
rg -n -A65 -B3 '^def (is_gl_context_unavailable|open_gl_window)\(' cuda_python_test_helpers/cuda_python_test_helpers/graphics.py
rg -n -A55 -B3 '^def (_gl_context|_configure_pyglet_headless)\(' cuda_bindings/tests/test_graphics_apis.py

Length of output: 9434


@juenglin suggestion: Share the TCC check, but keep it separate from is_gl_context_unavailable().

That function classifies an exception after GL setup fails. TCC is a property of the selected CUDA device, so check it before GL setup. A GL context could also open on another GPU even when the selected CUDA device uses TCC.

Do not add generic GLException handling. The helper deliberately rejects it because it can indicate a real GL error.

A separate helper in cuda_python_test_helpers.graphics could return a skip reason for TCC. Both test suites could call it before configuring pyglet or opening a window. Use cuda.bindings for the device query, rather than introducing a cuda.core dependency. Let query failures propagate.

You are interacting with an AI system.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@coderabbitai Please make the change you suggested.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📌 Tracking this review finding. CodeRabbit will verify future changes against the original feedback and will only mark it fixed when the code satisfies it.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Fixed in #3054.

Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
is_gl_context_unavailable,
open_gl_window,
select_headless_egl_device_for_cuda,
tcc_skip_reason,
)

import cuda_python_test_helpers
Expand Down Expand Up @@ -60,6 +61,12 @@ def _register_gl_image(tex_id, target):
raise


def _skip_if_tcc():
"""Skip on TCC: compute-only driver model, no OpenGL, so CUDA-GL interop is unavailable."""
if reason := tcc_skip_reason():
pytest.skip(reason)


def _configure_pyglet_headless():
"""On headless Linux: enable EGL mode, matched to the current CUDA device, or skip if EGL is absent."""
if sys.platform.startswith("linux") and not (os.environ.get("DISPLAY") or os.environ.get("WAYLAND_DISPLAY")):
Expand Down Expand Up @@ -157,6 +164,7 @@ def _allocate_gl_texture(win, width, height):
@contextlib.contextmanager
def _gl_context_and_buffer(nbytes=1024):
"""Yield ``(gl_buffer_name, nbytes)`` with a current GL context, or skip if GL is unavailable."""
_skip_if_tcc()
_configure_pyglet_headless()

try:
Expand Down Expand Up @@ -187,6 +195,7 @@ def _gl_context_and_buffer(nbytes=1024):
@contextlib.contextmanager
def _gl_context_and_texture(width=16, height=16):
"""Yield ``(tex_id, tex_target)`` with a current GL context, or skip if GL is unavailable."""
_skip_if_tcc()
_configure_pyglet_headless()

try:
Expand Down
29 changes: 29 additions & 0 deletions cuda_python_test_helpers/cuda_python_test_helpers/graphics.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,35 @@ def open_gl_window():
return None


def tcc_skip_reason() -> str | None:
"""Return a skip reason if the current CUDA device uses the TCC driver model, else None.

TCC (Tesla Compute Cluster) is a Windows compute-only driver model with no
OpenGL support, so CUDA-GL interop is unavailable on a TCC device. The
check queries the currently-selected device via the CUDA runtime API
(``cuda.bindings``) rather than ``cuda.core``, so both the ``cuda_core``
and ``cuda_bindings`` graphics tests can share it without the latter
taking a ``cuda.core`` dependency.

Runtime query failures (e.g. no CUDA device) propagate rather than being
masked as a skip, so a real environment problem surfaces instead of being
hidden behind a skipped test.
"""
from cuda.bindings import runtime as cudart

err, device = cudart.cudaGetDevice()
if err != cudart.cudaError_t.cudaSuccess:
raise RuntimeError(f"cudaGetDevice failed: {cudart.cudaGetErrorString(err)[1].decode()!r}")
err, is_tcc = cudart.cudaDeviceGetAttribute(cudart.cudaDeviceAttr.cudaDevAttrTccDriver, device)
if err != cudart.cudaError_t.cudaSuccess:
raise RuntimeError(
f"cudaDeviceGetAttribute(cudaDevAttrTccDriver) failed: {cudart.cudaGetErrorString(err)[1].decode()!r}"
)
if is_tcc:
return "Device is in TCC mode; TCC does not support OpenGL, so CUDA-GL interop is unavailable."
return None


def gl_context_not_on_nvidia_gpu_reason() -> str | None:
"""Return a skip reason if the current GL context is not on an NVIDIA GPU, else None.

Expand Down
Loading