Introduce context.call_overload and raise dynamic exception in guard_match_core_dims
#9524
Conversation
guilhermeleobas
force-pushed
the
guilhermeleobas/gufunc_call_overload
branch
from
cd14d40
to
9dc97bd
Compare
guilhermeleobas
changed the title
context.call_overloadcontext.call_overload and raise dynamic exception in guard_match_core_dims
guilhermeleobas
force-pushed
the
guilhermeleobas/gufunc_call_overload
branch
from
d388c18
to
7d50fe1
Compare
|
Checking - is this still a draft? |
|
Oh, sorry! It's ready for review. |
There was a problem hiding this comment.
Many thanks for the patch @guilhermeleobas, it's good to see that the dynamic exceptions work can enhance error messages like this. I've made a few comments inline, my greatest concerns at present are ensuring that the call_overload API is appropriate and also that the error messages being generated are as clear and informative as possible. Thanks again!
| @@ -904,6 +904,15 @@ def call_internal_no_propagate(self, builder, fndesc, sig, args): | |||
| sig.args, args) | |||
| return status, res | |||
|
|
|||
| def call_overload(self, builder, impl, sig, args): | |||
There was a problem hiding this comment.
I've written an almost identical call_overload locally as part of a large ufunc refactoring effort. I elected for it to take sig.args / an argument tuple opposed to a signature. When an overload is resolved, the return type is a function of the implementation, opposed to what is prescribed by a signature, this API perhaps suggests otherwise?
numba/cpython/tupleobj.py
Outdated
| @lower_constant(types.Tuple) | ||
| @lower_constant(types.NamedTuple) | ||
| def unituple_constant(context, builder, ty, pyval): | ||
| def unituple_constant_(context, builder, ty, pyval): |
There was a problem hiding this comment.
Why has this gained an underscore at the end?
There was a problem hiding this comment.
flake8, I believe
numba/np/npyimpl.py
Outdated
| @@ -297,7 +302,8 @@ def raise_impl(self_shape, other_shape): | |||
| ) | |||
| tup = (context.make_tuple(builder, sig.args[0], self.shape), | |||
| context.make_tuple(builder, sig.args[1], other.shape),) | |||
| context.compile_internal(builder, raise_impl, sig, tup) | |||
| context.typing_context.refresh() | |||
There was a problem hiding this comment.
Why does the typing context need refreshing?
There was a problem hiding this comment.
Function is declared inside the scope of guard_match_core_dim. If the typing context is not refreshed, Numba fails at a later stage:
File "/Users/guilhermeleobas/git/numba/numba/np/npyimpl.py", line 624, in numpy_gufunc_kernel
arg_a.guard_match_core_dims(ufunc, arg_b, idx_a, idx_b)
File "/Users/guilhermeleobas/git/numba/numba/np/npyimpl.py", line 308, in guard_match_core_dims
context.call_overload(builder, raise_impl, sig, tup)
File "/Users/guilhermeleobas/git/numba/numba/core/base.py", line 911, in call_overload
fnty = self.typing_context.resolve_value_type(impl)
File "/Users/guilhermeleobas/git/numba/numba/core/typing/context.py", line 397, in resolve_value_type
raise typeof_exc
ValueError: Cannot determine Numba type of <class 'function'>
There was a problem hiding this comment.
OK. If it's possible for the call_overload function to fail due to inconsistent state in the typing registries, should a typing context refresh be made unconditionally at the start of the the call_overload function implementation to prevent this from ever happening?
There was a problem hiding this comment.
Wouldn't that be too slow? Refreshing the typing context is not needed if the function is declared in the global scope.
numba/tests/npyufunc/test_gufunc.py
Outdated
| msg = ('operand 0 has a mismatch with operand 1 one of its core ' | ||
| 'dimension(s), with bar signature (n),(n) -> () ' | ||
| '(shape (5, 3, 2) is different from (3))') |
There was a problem hiding this comment.
The information content is a great improvement, but I'm struggling to understand what this means in its current form. Is it possible to match the NumPy message (and maybe enhance it with shape info/function names)?
There was a problem hiding this comment.
I've changed the exception message to match NumPy. That would probably be less confusing(?)
stuartarchibald
added
4 - Waiting on author
guilhermeleobas
added
4 - Waiting on reviewer
PR is separated into four commits:
overload_method(UniTuple, '__str__')and testscontext.call_overloadguard_match_core_dimsto call an overload that can emit a dynamic exceptiontupleobj.pyfrom.flake8