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

Fixes for overload_method #3704

Merged
merged 8 commits into from Jan 30, 2019
Merged

Fixes for overload_method #3704

merged 8 commits into from Jan 30, 2019

Conversation

sklam
Copy link
Member

@sklam sklam commented Jan 22, 2019

Fix #3489. overload_method now supports keyword arguments.
Fix #3683. attribute dispatcher cache will consider the argument types.

@seibert
Copy link
Contributor

seibert commented Jan 23, 2019

Failing on Python 2.7:

2019-01-22T22:03:06.4366190Z ======================================================================
2019-01-22T22:03:06.4366360Z ERROR: test_overload_method_literal_unpack (numba.tests.test_extending.TestHighLevelExtending)
2019-01-22T22:03:06.4367580Z ----------------------------------------------------------------------
2019-01-22T22:03:06.4367670Z Traceback (most recent call last):
2019-01-22T22:03:06.4367720Z   File "numba/tests/test_extending.py", line 899, in test_overload_method_literal_unpack
2019-01-22T22:03:06.4367870Z     bar(A)
2019-01-22T22:03:06.4367930Z   File "numba/dispatcher.py", line 350, in _compile_for_args
2019-01-22T22:03:06.4368430Z     error_rewrite(e, 'typing')
2019-01-22T22:03:06.4368640Z   File "numba/dispatcher.py", line 315, in error_rewrite
2019-01-22T22:03:06.4368700Z     raise e
2019-01-22T22:03:06.4368750Z TypingError: Caused By:
2019-01-22T22:03:06.4368870Z Traceback (most recent call last):
2019-01-22T22:03:06.4368930Z   File "numba/compiler.py", line 243, in run
2019-01-22T22:03:06.4368980Z     stage()
2019-01-22T22:03:06.4369030Z   File "numba/compiler.py", line 482, in stage_nopython_frontend
2019-01-22T22:03:06.4369150Z     self.locals)
2019-01-22T22:03:06.4369200Z   File "numba/compiler.py", line 1025, in type_inference_stage
2019-01-22T22:03:06.4369240Z     infer.propagate()
2019-01-22T22:03:06.4369370Z   File "numba/typeinfer.py", line 858, in propagate
2019-01-22T22:03:06.4369420Z     raise errors[0]
2019-01-22T22:03:06.4370020Z TypingError: Invalid use of BoundFunction((<class 'numba.types.npytypes.Array'>, 'litfoo') for array(float64, 1d, C)) with parameters (Literal[str](LiTeRaL))
2019-01-22T22:03:06.4370190Z  * parameterized
2019-01-22T22:03:06.4370760Z [1] During: resolving callee type: BoundFunction((<class 'numba.types.npytypes.Array'>, 'litfoo') for array(float64, 1d, C))
2019-01-22T22:03:06.4370850Z [2] During: typing of call at numba/tests/test_extending.py (896)
2019-01-22T22:03:06.4370970Z 
2019-01-22T22:03:06.4371010Z 
2019-01-22T22:03:06.4371050Z File "numba/tests/test_extending.py", line 896:
2019-01-22T22:03:06.4371100Z         def bar(A):
2019-01-22T22:03:06.4371220Z             return A.litfoo("LiTeRaL")
2019-01-22T22:03:06.4371270Z             ^
2019-01-22T22:03:06.4371290Z 
2019-01-22T22:03:06.4371320Z 
2019-01-22T22:03:06.4372030Z Invalid use of BoundFunction((<class 'numba.types.npytypes.Array'>, 'litfoo') for array(float64, 1d, C)) with parameters (Literal[str](LiTeRaL))
2019-01-22T22:03:06.4372090Z 
2019-01-22T22:03:06.4372710Z ----------------------------------------------------------------------

Copy link
Contributor

@stuartarchibald stuartarchibald 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. I've tried it out and it seems to fix the problem, it also unblocks #3468. Couple of minor queries to resolve then good for merge.

def _adjust_omitted_args(argtypes, argvals):
"""Add dummy arguments for each missing types.Omitted in *argtypes*.
"""
if len(argtypes) > len(argvals):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this branch ever executed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, kwargs support requires this branch to patch the arguments. When the signature contains Omitted types, those locations are missing in the actual arguments. This re-adds them as required by anything base on Dispatcher.

Copy link
Contributor

Choose a reason for hiding this comment

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

hmmm, the added unit tests do not seem to trip this branch, and if I edit the code locally to remove the call to the branch (i.e. revert the change on this line https://github.com/numba/numba/pull/3704/files#diff-d403faa8b837ae083d77e0cba8abd6c7R648) the unit tests for numba.tests.test_extending still pass, can you reproduce this effect? If you can, I'm wondering, practically, how to trip this....?

Copy link
Member Author

Choose a reason for hiding this comment

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

you're right. It turns out adjusting the pysignature fixes the actual problem already.

@@ -627,7 +645,7 @@ def method_impl(context, builder, sig, args):
call = context.get_function(disp_type, sig)
# Link dependent library
context.add_linking_libs(getattr(call, 'libs', ()))
return call(builder, args)
return call(builder, _adjust_omitted_args(sig.args, args))
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of scope so new ticket added... this needs enhancing to also do validation #3708, it was too easy to create invalid overloads when testing this.

numba/tests/test_extending.py Outdated Show resolved Hide resolved
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Jan 24, 2019
@sklam sklam 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 Jan 25, 2019
Copy link
Contributor

@stuartarchibald stuartarchibald 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 fixes. Looks good. Can be merged once CI passes.

@stuartarchibald stuartarchibald 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 Jan 28, 2019
@stuartarchibald stuartarchibald added this to Ready to Merge in Active Jan 29, 2019
@sklam sklam merged commit 342c529 into numba:master Jan 30, 2019
Active automation moved this from Ready to Merge to Done Jan 30, 2019
@sklam sklam deleted the fix/ovmethod branch January 30, 2019 17:46
@seibert seibert mentioned this pull request Feb 4, 2019
6 tasks
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
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants