Add a missed except *

[shwina]

No description provided.

[bdice]

LGTM. (Also I fixed the title of the PR.)

I discovered while reviewing that except + is for C++ exceptions and except * is for Python exceptions! That was new to me - I hadn't used except * before, only except +.

[vyasr]

I discovered while reviewing that except + is for C++ exceptions and except * is for Python exceptions! That was new to me - I hadn't used except * before, only except +.

You can also do things like except 0 as a way to specify that a function will return some error code on exit when an exception occurs (and I think there's also a way to say that "0 may indicate an error", something like except 0?). It's consistent with the way languages without exceptions like C use error codes, and Cython is C inspired :)

[vyasr]

rerun tests

[jakirkham]

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

[shwina]

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

[jakirkham]

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

[jakirkham]

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

[vyasr]

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.

[shwina]

Sounds good to me

[jakirkham]

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.

[bdice]

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.

[jakirkham]

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.

[jakirkham]

@gpucibot merge