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.

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

Drop void return type when raising exceptions #436

Closed

Conversation

jakirkham
Copy link
Member

Fixes #435

After poking at this a bit locally, it seems Cython's except * is picking up exceptions set through asyncio even when it shouldn't. Not sure if this behavior is due to Python or Cython. That said, we can avoid using except * here by using cdef without the void return type (this is the same as cdef object). Based on running things locally this seems to behave correctly for me. Though please check as well.

@jakirkham jakirkham requested a review from a team as a code owner March 7, 2020 05:52
@jakirkham jakirkham mentioned this pull request Mar 7, 2020
@jakirkham
Copy link
Member Author

cc @quasiben @pentschev

@quasiben
Copy link
Member

quasiben commented Mar 7, 2020

This still hangs for me with the dask-cuda merge benchmark. My Cython expertise is quite low but is there now conflict with argument and definitions in core_dep.pxd

ctypedef void (*ucp_send_callback_t)(void *request, ucs_status_t status) # noqa
?

@jakirkham
Copy link
Member Author

Where are you running this? FWIW I ran this on dgx-15 without issues. Just want to exclude the possibility of hardware issues getting in the mix.

That's a good point. So I guess the exception will get ignored in those case anyways. Yeah it's hard to solve this correctly without more-or-less doing what Mads has already done. If we are not happy with this, maybe we can try to help get his work in? Sorry I don't have a better answer 😞

@jakirkham
Copy link
Member Author

Should add when I tested this I was using Vibhu's MRE from PR ( #402 ). This seems potentially more complex than the merge benchmark. So I'm pretty sure I'm exercise all the same things, but please let me know if that's incorrect.

@pentschev
Copy link
Member

Yeah, there's no error with the merge benchmark, Ben and I were testing both that and the merge benchmark. It's intriguing why it doesn't fail with that, in fact the tests for u1 and u16 fail, but u256 and larger pass, so it's probably due to the size of the transfer, but right now I have no clue where the issue may lie.

@madsbk
Copy link
Member

madsbk commented Mar 9, 2020

This should be fixed in #418

@madsbk
Copy link
Member

madsbk commented Mar 10, 2020

#439 replaces this PR

@madsbk madsbk closed this Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Confusing Breakage
4 participants