Skip to content

Fix binding logic in @overload_glue. - #7996

Merged
sklam merged 3 commits into
numba:mainfrom
stuartarchibald:fix/7982
May 3, 2022
Merged

sklam merged 3 commits into
numba:mainfrom
stuartarchibald:fix/7982

Conversation

@stuartarchibald

@stuartarchibald stuartarchibald commented Apr 20, 2022 •

Copy link
Copy Markdown
Contributor

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
func_name = self.key.__name__

def typer(ptr, shape, dtype=types.none):
def typer(ptr, shape, dtype=None):

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

@sklam sklam left a comment

Copy link
Copy Markdown
Member

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)?

Comment thread numba/core/overload_glue.py Outdated
Comment thread numba/core/overload_glue.py Outdated
@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
Comment thread numba/core/overload_glue.py Outdated
Comment thread numba/core/overload_glue.py Outdated
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
Copy Markdown
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
Copy Markdown
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

Comment thread numba/core/overload_glue.py Outdated
Rename variable and fix code/docstring.

@sklam sklam left a comment

Copy link
Copy Markdown
Member

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