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

Fix binding logic in @overload_glue. #7996

Merged
merged 3 commits into from
May 3, 2022
Merged

Conversation

stuartarchibald
Copy link
Contributor

@stuartarchibald stuartarchibald commented Apr 20, 2022

Updates the binding logic to accommodate calling @overload_glue
wrapped functions with named arguments.

Fixes #7982

Updates the binding logic to accommodate calling `@overload_glue`
wrapped functions with named arguments.

Fixes numba#7982
@stuartarchibald stuartarchibald changed the title Attempt to fix binding logic in overload_glue. Fix binding logic in @overload_glue. Apr 21, 2022
@stuartarchibald stuartarchibald marked this pull request as ready for review April 21, 2022 12:39
@stuartarchibald stuartarchibald added the Effort - medium Medium size effort needed label Apr 21, 2022
@@ -1254,7 +1254,7 @@ class NumbaCArray(CallableTemplate):
def generic(self):
func_name = self.key.__name__

def typer(ptr, shape, dtype=types.none):
def typer(ptr, shape, dtype=None):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change needs some thinking about. I think the default now matches what an @overload would implement, but I'm not sure if this is correct WRT @overload_glue. There's also the question of whether the subsequent type check needs to be a is_nonelike() type check for the case of a types.none appearing, not sure if this is actually possible yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... that's a problem. It will be a types.none when called with c_arr = numba.carray(arr, (3,), dtype=None).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following will fail:

import ctypes

import numba
import numpy as np


@numba.cfunc(numba.types.void(
    numba.types.CPointer(numba.types.double)
))
def func(arr: np.ndarray) -> None:
    c_arr = numba.carray(arr, (3,), dtype=None)
    c_arr[-1] = 1


my_arr = np.array([1, 2, 3], dtype=np.float_).ctypes.data_as(ctypes.POINTER(ctypes.c_double))
func(my_arr)

The OverloadSelector cannot find the impl:

File "/path/to/numba/numba/core/base.py", line 44, in find
    out = self._find(sig)
File "/path/to/numba/numba/core/base.py", line 53, in _find
    raise errors.NumbaNotImplementedError(f'{self}, {sig}')
numba.core.errors.NumbaNotImplementedError: <numba.core.base.OverloadSelector object at 0x7ff7d85c3310>, (float64*, UniTuple(Literal[int](3) x 1), none)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From OOB chat, time would be better spent translating this to @overload than trying to fix this edge case in @overload_glue, added this as issue: #8009

Copy link
Member

@sklam sklam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The patch looks good in general. Just one nitpick on the change to use generator for the varnames. Why not just a simple sequence (list/tuple)?

numba/core/overload_glue.py Outdated Show resolved Hide resolved
numba/core/overload_glue.py Outdated Show resolved Hide resolved
@sklam sklam added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Apr 27, 2022
@stuartarchibald stuartarchibald added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Apr 27, 2022
numba/core/overload_glue.py Outdated Show resolved Hide resolved
Comment on lines 31 to 32
"""This generates a function based on the argnames generated by the
varnames_gen generator, the "body_func" is the function that'll
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""This generates a function based on the argnames generated by the
varnames_gen generator, the "body_func" is the function that'll
"""This generates a function based on the argnames provided by
varnames, the "body_func" is the function that'll

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed these and the side-effects of fixing these in 2e833f7

numba/core/overload_glue.py Outdated Show resolved Hide resolved
Rename variable and fix code/docstring.
Copy link
Member

@sklam sklam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch!

@sklam sklam added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels May 3, 2022
@sklam sklam merged commit bd1e11a into numba:main May 3, 2022
@stuartarchibald stuartarchibald mentioned this pull request Sep 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge Effort - medium Medium size effort needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

np.full ignores keyword argument names
2 participants