Skip to content
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

Add a missed except * #1057

Merged
merged 1 commit into from
Jun 17, 2022
Merged

Add a missed except * #1057

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion python/rmm/_lib/device_buffer.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,7 +344,7 @@ cdef void _copy_async(const void* src,
void* dst,
size_t count,
ccudart.cudaMemcpyKind kind,
cuda_stream_view stream) nogil:
cuda_stream_view stream) nogil except *:
Copy link
Member

Choose a reason for hiding this comment

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

Is except * compatible with nogil? Or are we holding the GIL when raising that exception

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch, John! Should this function even be marked nogil? It clearly needs the GIL to raise the RuntimeError that it does

Copy link
Member

Choose a reason for hiding this comment

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

Yeah it's an interesting question. Was thinking this might be permissible, but maybe only in Cython 3 ( cython/cython#3558 )

Copy link
Member

@jakirkham jakirkham Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suppose there are a few options:

  1. Drop the nogil
  2. Use a with gil block in this function if an error comes up
  3. Return -1 (or the error code) and raise outside this function
  4. ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call, I didn't even think about that. I think we should probably not mark the function nogil, but just wrap the cudaMemcpyAsync call in a with nogil block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me

Copy link
Member

Choose a reason for hiding this comment

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

It's worth noting there is some overhead to releasing the GIL. So if the function is fast, it might not be worth releasing the GIL.

Copy link
Contributor

@bdice bdice Jun 16, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can leave this as nogil even with the raise?

Oh, and the with gil: is no longer required for the raise since 0.29. It's injected automatically when raise is used in a nogil section.

cython/cython#3558 (comment)

Also confirmed in this Changelog entry from 0.29:

Raising exceptions from nogil code will automatically acquire the GIL, instead of requiring an explicit with gil block.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks Bradley! 🙏

Was looking for that text in the docs as I was thinking that might be the case, but only found it in relation to 3.0.0. Missed the changelog entry.

As we already require Cython 0.29 as a minimum, we should be good here.

"""
Asynchronously copy data between host and/or device pointers

Expand Down