Fix bare except in Stream_accept that swallows all exceptions#1632
Fix bare except in Stream_accept that swallows all exceptions#1632Andy-Jost wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
Andy-Jost
added
bug
|
/ok to test a1614ed |
|
I found this issue as a follow-on to this comment while searching for other places where |
cuda_core/cuda/core/_stream.pyx
Outdated
| except TypeError as e: | ||
| if "__cuda_stream__" not in str(e): | ||
| raise |
There was a problem hiding this comment.
cf.
cuda-python/cuda_core/cuda/core/_stream.pyx
Lines 474 to 477 in a1614ed
There was a problem hiding this comment.
Why are we swallowing an AttributeError, just to tell the user "The thing doesn't have this attribute"? This seems super roundabout, and adding a string contains check to this oddity doesn't seem like the right approach.
There was a problem hiding this comment.
I think the overall result is correct. If someone passes an arbitrary object to the stream protocol and that object lacks a __cuda_stream__ attribute, then raising TypeError is the correct response. This matches standard protocols such as iter(), len(), and hash().
That said, I think you're right that it would be arguably cleaner to use hasattr at line 474 rather than catch the AttributeError, though that would mean checking hasattr and then calling the equivalent of getattr separately.
The problem is that Python does not provide a clean method of detecting when an object lacks a specific protocol, and adding something custom here, like ProtocolNotSupportedError, would be even more out of the norm.
I don't see a better option than catching the TypeError and narrowing it with this heuristic to make sure it looks like the one we expect.
There was a problem hiding this comment.
Actually, now that I say that, perhaps Stream_accept near line 512 could just use hasattr to check for __cuda_stream__. But that raises its own complications, since Stream._init has many paths besides the stream protocol one.
There was a problem hiding this comment.
I think the latest upload is probably what you had in mind.
|
|
@Andy-Jost Just to confirm, this PR can wait after we release v0.6.0, or do you want this to be part of v0.6.0 as well? |
|
This is not a critical fix, so I put it under v0.7.0. But I think it is very safe, so if it gets approved in time I will include it with v0.6.0. |
cpcloud
left a comment
cpcloud
left a comment
There was a problem hiding this comment.
Can we fix this without piling on another hack?
cuda_core/cuda/core/_stream.pyx
Outdated
| except TypeError as e: | ||
| if "__cuda_stream__" not in str(e): | ||
| raise |
There was a problem hiding this comment.
Why are we swallowing an AttributeError, just to tell the user "The thing doesn't have this attribute"? This seems super roundabout, and adding a string contains check to this oddity doesn't seem like the right approach.
…#1631) Narrow the catch from bare `except: pass` to `except TypeError` with a string check, so that only missing `__cuda_stream__` is silently handled. Buggy protocol implementations and other exceptions now propagate correctly. Co-authored-by: Cursor <cursoragent@cursor.com>
Andy-Jost
force-pushed
the
fix-stream-accept-bare-except
branch
from
a1614ed to
2f81e58
Compare
|
/ok to test 2f81e58 |
3 participants
Summary
except: passinStream_accepttoexcept TypeErrorwith a string check for"__cuda_stream__", so only the "protocol not supported" case is silently handled__cuda_stream__implementations,RuntimeError,KeyboardInterrupt,SystemExit, etc. now propagate correctlyCloses #1631
Changes
_stream.pyx: Replaceexcept: passwithexcept TypeError as e: if "__cuda_stream__" not in str(e): raiseTest Coverage
Stream_acceptand stream protocol tests cover the happy pathMade with Cursor