Skip to content

Move callback wrappers to Python layer #544

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.

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

Merged
merged 2 commits into from
Apr 4, 2025

Conversation

vzhurba01
Copy link
Collaborator

close #531

@vzhurba01 vzhurba01 added enhancement Any code-related improvements P0 High priority - Must do! cuda.bindings Everything related to the cuda.bindings module labels Apr 1, 2025
@vzhurba01 vzhurba01 added this to the cuda-python 12.9.0 & 11.8.7 milestone Apr 1, 2025
@vzhurba01 vzhurba01 self-assigned this Apr 1, 2025
Copy link
Contributor

copy-pr-bot bot commented Apr 1, 2025

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.

@vzhurba01
Copy link
Collaborator Author

/ok to test

This comment has been minimized.

Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

I have reviewed 3 out of 4 files (haven't had time to cover runtime.pyx.in yet).

Q: I assume so far we have not supported passing a pure Python function as callback to any of the bindings directly, without using ctypes or other means as a trampoline?

cdef cuAsyncCallbackData *cbData = NULL
cbData = <cuAsyncCallbackData *>malloc(sizeof(cbData[0]))
if cbData == NULL:
return CUresult.CUDA_ERROR_OUT_OF_MEMORY
Copy link
Member

Choose a reason for hiding this comment

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

Q: Should this be a 1-tuple?

Suggested change
return CUresult.CUDA_ERROR_OUT_OF_MEMORY
return (CUresult.CUDA_ERROR_OUT_OF_MEMORY,)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch. Fixed.

Copy link
Collaborator

@kkraus14 kkraus14 Apr 4, 2025

Choose a reason for hiding this comment

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

It would be good to have type annotations or something else as a way to prevent issues like this from sneaking in.

Copy link
Member

@leofang leofang Apr 4, 2025

Choose a reason for hiding this comment

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

It seems currently Cython cannot raise a compile-time error for either case, only a run-time error (which is not desired I think).

cpdef tuple f():
    return 42

def g() -> tuple:
    return 42

Output:

>>> import test_tuple
>>> test_tuple.f()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "test_tuple.pyx", line 1, in test_tuple.f
    cpdef tuple f():
  File "test_tuple.pyx", line 3, in test_tuple.f
    return 42
TypeError: Expected tuple, got int
>>> test_tuple.g()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "test_tuple.pyx", line 7, in test_tuple.g
    return 42
TypeError: Expected tuple, got int

@vzhurba01
Copy link
Collaborator Author

Q: I assume so far we have not supported passing a pure Python function as callback to any of the bindings directly, without using ctypes or other means as a trampoline?

Correct.

@vzhurba01
Copy link
Collaborator Author

/ok to test

@vzhurba01 vzhurba01 mentioned this pull request Apr 4, 2025
Copy link
Member

@leofang leofang left a comment

Choose a reason for hiding this comment

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

LGTM!

@leofang leofang merged commit 6d07877 into NVIDIA:main Apr 4, 2025
73 checks passed
Copy link

github-actions bot commented Apr 4, 2025

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

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 enhancement Any code-related improvements P0 High priority - Must do!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move callback wrappers to the Python layer
3 participants