Use flatten_args for _impl_caches in templates
#9457
Conversation
This reverts commit cc9ab81.
|
Numba compilation still has at least 2 issues:
In this PR, I only solved one issue (different from above 2): if a overloaded function is compiled in caller's typeinfer pass, then I can make sure it won't recompile in caller's native lowering pass. (I hope so, but not pretty sure) |
|
An example to show why we need this PR: from os import environ
environ["NUMBA_CHROME_TRACE"] = "3.json"
environ["NUMBA_DUMP_LLVM"] = "1"
from numba import njit
from numba.core import types
from numba.typed import Dict
@njit
def goo(k, j):
d0 = Dict.empty(types.int64, types.int64)
d = Dict.empty(types.int64, value_type=types.int64)
d1 = Dict.empty(key_type=types.int64, value_type=types.int64)
d[1] = 1
d[2] = 2
d[3] = 3
_ = d.get(k)
d.pop(k)
d.pop(j, None)
del d[3]
@njit
def joo(k):
d = Dict.empty(types.int64, value_type=types.int64)
d[1] = 1
return d.pop(k, None)
print(goo(1, 2))
print(joo(0))without this patch, we compile these modules: with this patch, we compile these modules: The diff is without it, our main will compile |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| @@ -1952,7 +1952,7 @@ def getitem(a, a_i): | |||
| raise TypeError("np.random.choice() first argument should be " | |||
| "int or array, got %s" % (a,)) | |||
|
|
|||
| if size in (None, types.none): | |||
| if size in (None, types.none) or isinstance(size, types.Omitted): | |||
There was a problem hiding this comment.
Not a proper review, but this ought to be cgutils.is_nonelike(size)?
There was a problem hiding this comment.
Yes, I also notice this API to check it, will refactor.
|
I didn't change this file, but CI reports the flake8 errors in it. Emm... btw, the last real failure is about overriding @overload flags in a test, try to understand the test case first.. |
|
@sklam I'm not sure if I choose the right way to fix them. Maybe the real fix should happen at the start of "funny" things, rather than at the end of them. I choose the latter way in this PR. The real issue is the inconsistency between |
numba/core/typing/templates.py
Outdated
| dict_flags = dict(flags.options) if flags is not None else {} | ||
| for key, value in self._jit_options.items(): | ||
| if key in dict_flags: | ||
| dict_flags["key"] = value |
There was a problem hiding this comment.
Dev note: check why flags are not correct; e.g. no_cpython_wrapper
numba/core/typing/templates.py
Outdated
| # FIXME: what's the real thing need to do is adding Ommitted to the args | ||
| # check the pyfunc signature, if exist some kwargs, e.g., default=None, | ||
| # check whether kws specify them, if not, | ||
| # please add Omitted(default=None) to args | ||
| ov_sig = inspect.signature(self._overload_func) | ||
| if len(args) + len(kws) < len(ov_sig.parameters): | ||
| for param in list(ov_sig.parameters.values())[len(args):]: | ||
| name = param.name | ||
| default = param.default | ||
| if default != inspect.Parameter.empty and name not in kws: | ||
| # add numba type of this param into kws | ||
| # it must be an Omitted type with a default value | ||
| kws[name] = types.Omitted(default) |
There was a problem hiding this comment.
Dev note: we should normalize signatures
| if sig is not None: | ||
| flatten_args = sig.args | ||
|
|
||
| if cache_key is not None: | ||
| self._impl_cache[cache_key] = disp, flatten_args |
There was a problem hiding this comment.
Dev note: needs investigation. sometimes sig.args is different from the given args.
sklam
added
the
4 - Waiting on reviewer
|
interesting failure: Traceback (most recent call last):
File "/home/vsts/work/1/s/numba/tests/test_compiler_flags.py", line 40, in test_fastmath_in_overload
self.assertEqual(b, "Has fastmath")
AssertionError: 'No fastmath' != 'Has fastmath'
- No fastmath
? ^^
+ Has fastmath
? ^^^It could inspire me how to fix |
|
added a naive release doc and fixed the failed test case. TODO: add some new tests to check if this PR can deduplicate the expected unnecessary compilations. |
|
this failure (numba.numba (Linux py310_np123)) is a CI infra issue, not code issue. |
Fixes #9440
Fixes #9462