Skip to content

Fix #1122: Reinstate graphics APIs #1123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 7 commits into from
Oct 13, 2025
Merged

Fix #1122: Reinstate graphics APIs #1123

merged 7 commits into from
Oct 13, 2025

Conversation

@mdboom
Copy link
Contributor

@mdboom mdboom commented Oct 10, 2025

These inadvertently got commented out in #914.

@copy-pr-bot
Copy link
Contributor

copy-pr-bot bot commented Oct 10, 2025

Auto-sync is disabled for ready for review pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

@mdboom mdboom added the to-be-backported Trigger the bot to raise a backport PR upon merge label Oct 10, 2025
@mdboom mdboom self-assigned this Oct 10, 2025
@mdboom mdboom requested a review from leofang October 10, 2025 14:57
@mdboom mdboom added the P0 High priority - Must do! label Oct 10, 2025
@mdboom
Copy link
Contributor Author

mdboom commented Oct 10, 2025

/ok to test

@github-actions

This comment has been minimized.

Sign in to view
@mdboom
Copy link
Contributor Author

mdboom commented Oct 10, 2025

/ok to test

leofang
leofang previously approved these changes Oct 10, 2025
View reviewed changes
@leofang leofang added the cuda.bindings Everything related to the cuda.bindings module label Oct 10, 2025
@leofang leofang added this to the cuda-python 13-next, 12-next milestone Oct 10, 2025
cpcloud
cpcloud requested changes Oct 10, 2025
View reviewed changes
cuda_bindings/tests/test_graphics_apis.py Outdated

@pytest.mark.skipif(PySide6 is None, reason="PySide6 not installed")
def test_graphics_api_smoketest():
from PySide6 import QtGui, QtOpenGL
Copy link
Contributor

@cpcloud cpcloud Oct 10, 2025

Choose a reason for hiding this comment

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

Seems like we can avoid a bunch of the noisy skipif and imports by using importorskip here:

Suggested change
from PySide6 import QtGui, QtOpenGL
PySide6 = pytest.importorskip("PySide6")
QtGui = PySide6.QtGui
QtOpenGL = PySide6.QtOpenGL

Copy link
Contributor Author

@mdboom mdboom Oct 10, 2025

Choose a reason for hiding this comment

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

I didn't know about the importorskip trick.

Still need to import the subpackages explicitly, though. PySide6 doesn't import everything automatically.

Copy link
Contributor

@cpcloud cpcloud Oct 10, 2025

Choose a reason for hiding this comment

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

You can pass a dotted path to importorskip, making this code even shorter:

QtGui = pytest.importorksip("PySide6.QtGui")
# etc

Copy link
Contributor

@cpcloud cpcloud Oct 10, 2025

Choose a reason for hiding this comment

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

Didn't realize they were subpackages, otherwise I would've made that my original suggestion!

@mdboom
Copy link
Contributor Author

mdboom commented Oct 10, 2025

/ok to test

@mdboom mdboom requested a review from cpcloud October 10, 2025 19:38
cpcloud
cpcloud previously approved these changes Oct 10, 2025
View reviewed changes
Copy link
Contributor

@cpcloud cpcloud left a comment

Choose a reason for hiding this comment

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

The additional skip can be done later if desired.

rparolin
rparolin reviewed Oct 10, 2025
View reviewed changes
cuda_bindings/tests/test_graphics_apis.py Outdated


def test_graphics_api_smoketest():
_ = pytest.importorskip("PySide6")
Copy link
Collaborator

@rparolin rparolin Oct 10, 2025

Choose a reason for hiding this comment

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

I don't love skippable tests because I feel too much complexity is introduced from the lack of symmetry between local and ci builds. For example, there is a potential for the tests to bit-rot if they aren't run frequently enough by devs or the ci runners.

@mdboom
Copy link
Contributor Author

mdboom commented Oct 13, 2025

/ok to test

@mdboom
Copy link
Contributor Author

mdboom commented Oct 13, 2025

/ok to test

@mdboom
Copy link
Contributor Author

mdboom commented Oct 13, 2025

I have updated this to use the pyglet-based test as suggested by @pijyoi (and pyglet is small enough, I think it's fine to add a a test-only dependency) and added a the "always expected to fail" test as suggested by @rparolin. I think this is ready to go.

@mdboom
Copy link
Contributor Author

mdboom commented Oct 13, 2025

/ok to test

@leofang leofang linked an issue Oct 13, 2025 that may be closed by this pull request
[BUG]: cudaGraphicsGLRegisterImage returns null handle with cuda-bindings 12.9.3 #1122
Closed
1 task
@leofang leofang merged commit 73da391 into NVIDIA:main Oct 13, 2025
71 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 13, 2025
@mdboom @leofang @github-actions
Fix #1122: Reinstate graphics APIs (#1123)
7b9171d 
* Fix #1122: Reinstate graphics APIs

* Add a test

* Update cuda_bindings/tests/test_graphics_apis.py

Co-authored-by: Leo Fang <[email protected]>

* Use importorskip

* Improve tests

* Fix test

* Fix Windows tests

---------

Co-authored-by: Leo Fang <[email protected]>
(cherry picked from commit 73da391)
@github-actions github-actions bot mentioned this pull request Oct 13, 2025
Merged
@github-actions
Copy link

github-actions bot commented Oct 13, 2025

Successfully created backport PR for 12.9.x:

@github-actions
Copy link

github-actions bot commented Oct 13, 2025

Doc Preview CI
Preview removed because the pull request was closed or merged.

leofang added a commit that referenced this pull request Oct 14, 2025
@github-actions @mdboom
@leofang @pre-commit-ci
[Backport 12.9.x] Fix #1122: Reinstate graphics APIs (#1128)
82e98e2 
* Fix #1122: Reinstate graphics APIs (#1123)

* Fix #1122: Reinstate graphics APIs

* Add a test

* Update cuda_bindings/tests/test_graphics_apis.py

Co-authored-by: Leo Fang <[email protected]>

* Use importorskip

* Improve tests

* Fix test

* Fix Windows tests

---------

Co-authored-by: Leo Fang <[email protected]>
(cherry picked from commit 73da391)

* [pre-commit.ci] auto code formatting

---------

Co-authored-by: Michael Droettboom <[email protected]>
Co-authored-by: Leo Fang <[email protected]>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@rwgk
Copy link
Collaborator

rwgk commented Nov 4, 2025

FYI

See #1210 (comment)

test_graphics_apis.py::test_graphics_api_smoketest is SKIPPED in all CI jobs.

This isn't critical at the moment. However, SWQA stumbled over this test, PR #1210 is for resolving that issue.

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

Reviewers

@leofang leofang leofang approved these changes

@rparolin rparolin Awaiting requested review from rparolin

@cpcloud cpcloud Awaiting requested review from cpcloud

Assignees

@mdboom mdboom

Labels

cuda.bindings Everything related to the cuda.bindings module P0 High priority - Must do! to-be-backported Trigger the bot to raise a backport PR upon merge

Projects

None yet

Milestone

cuda-python 13.0.3 & 12.9.4

Development

Successfully merging this pull request may close these issues.

[BUG]: cudaGraphicsGLRegisterImage returns null handle with cuda-bindings 12.9.3

5 participants

@mdboom @rwgk @cpcloud @rparolin @leofang