Do not report a registered type as "Unknown type" in get_cuda_native_handle - #2551
Open
LeSingh1 wants to merge 1 commit into
Open
Do not report a registered type as "Unknown type" in get_cuda_native_handle#2551LeSingh1 wants to merge 1 commit into
LeSingh1 wants to merge 1 commit into
Conversation
…handle
get_cuda_native_handle() wraps both the registry lookup and the getter call
in one try:
try:
return _handle_getters[obj_type](obj)
except KeyError:
raise TypeError("Unknown type: " + str(obj_type)) from None
The except clause is meant for "this type has no registered getter", but it
also fires for a KeyError raised *inside* the getter. When that happens the
diagnosis is wrong twice over: the reported type is registered, and
`from None` suppresses the context so the traceback that would show the real
failure is gone.
>>> _add_cuda_native_handle_getter(Registered, getter_that_raises_keyerror)
>>> get_cuda_native_handle(Registered())
TypeError: Unknown type: <class 'Registered'>
Move the getter call out of the try. The unregistered-type path is
unchanged, which the existing test_get_handle_error still covers.
Contributor
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
get_cuda_native_handle()wraps both the registry lookup and the getter call in asingle
try:The
except KeyErroris there for one thing: "this type has no registered getter". But_handle_getters[obj_type](obj)is two operations, so aKeyErrorraised inside thegetter is caught too — and then the diagnosis is wrong twice over:
exactly the wrong place;
from Nonesets__suppress_context__, so the traceback that would have shown the realfailure is not printed at all.
Reproduced (no GPU needed):
Fix
Move the getter call out of the
try, so only the dict lookup is guarded. Theunregistered-type path is byte-for-byte unchanged, and the existing
test_get_handle_errorstill covers it.This is a latent mis-diagnosis rather than a crash — today's registered getters are
generated one-liners — but it is the kind of "error path that fires for the wrong reason
and then destroys the evidence" that #2122 is about, and the fix is three lines with no
behaviour change on either existing path.
Tests
One test added next to the existing
test_get_handle/test_get_handle_errorpair incuda_bindings/tests/test_utils.py: it registers a getter that raisesKeyError(viamonkeypatch.setitem, so the global registry is restored) and asserts theKeyErrorpropagates. Fails on
mainwithTypeError: Unknown type: <class 'Registered'>.Verified against
upstream/mainand with the change; the happy path and theunregistered-type path both keep passing either way.
ruff checkandruff format --checkclean.Note: #2541 also touches
cuda_bindings/tests/test_utils.py, appending at the end of thefile; this change lands mid-file next to the existing handle tests, so the two are
independent. Happy to rebase whichever lands second.