MAINT: Remove internal uses of _inspect module #16787
Conversation
|
xref #16299, which contains a similar but IMO clearer implementation in a comment. |
|
How much does this affect import times? |
It should be negligible as importing
Thanks @eric-wieser - I had missed the code in the issue. I will take a look |
|
@eric-wieser - agreed, your implementation in #16299 is much more clear: updating now. |
|
@eric-wieser Ping. |
Replace usage of numpy.compat._inspect with implementation based on the standard inspect module.
Replace usage of numpy.compat._inspect with implementation based on the standard inspect module.
Use the parameter replacement mechansism for inspect.signature to verify consistent signatures between the dispatcher and primary function implementation. Co-authored-by: Eric Wieser <wieser.eric@gmail.com>
7799bf1 to
6e35c2a
Compare
|
There is a significant increase in import time, from 62ms to 74ms: |
|
Thanks @mattip , there is indeed a significant increase in import time. I was looking at the posted timings with
I'm not clear on why this is happening (perhaps a change in how the imports are being cached?). The rest of the difference seems like it may come down to the new implementation |
| if dispatcher_spec.defaults != (None,) * len(dispatcher_spec.defaults): | ||
| raise RuntimeError('dispatcher functions can only use None for ' | ||
| 'default argument values') | ||
| implementation_sig = inspect.signature(implementation) |
There was a problem hiding this comment.
I can only assume the timings are bad because inspect.signature is slower than getargspec
There was a problem hiding this comment.
Yes, some (very rough) profiling indicates that this is the case. Using the following setup:
from numpy.core.overrides import verify_matching_signatures
def foo(
a, b, m, n, k, copy=False, color="red", normed=False, weights=None,
bins=10, density=None, size=1024, shape=(3, 4)
):
pass
def _foo_dispatcher(
a, b, m, n, k, copy=None, color=None, normed=None, weights=None,
bins=None, density=None, size=None, shape=None
):
passtimeit gives:
On 6e35c2a (this PR):
In [2]: %timeit verify_matching_signatures(foo, _foo_dispatcher)
73.1 µs ± 879 ns per loop (mean ± std. dev. of 7 runs, 10000 loops each)
On 2283e26 (root of branch):
In [2]: %timeit verify_matching_signatures(foo, _foo_dispatcher)
4.85 µs ± 11.4 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each)
So the implementation based on inspect.signature is about 15x slower than the original based on _inspect.getargspec.
One idea I had was to try to re-implement this with inspect.getfullargspec, which is the more full-featured extension of the original (now deprecated) inspect.getargspec. I will update with -X importtime & profiling info from the attempt.
There was a problem hiding this comment.
One idea I had was to try to re-implement this with inspect.getfullargspec, which is the more full-featured extension of the original (now deprecated) inspect.getargspec.
This is implemented in terms of inspect.signature, then does some extra processing. You'd do better to probe the __code__ object directly.
Perhaps the comparison is the slow part.
|
For reference: |
|
Hmm, so it seems like a slowdown is inevitable by switching to the |
|
You can shave off a little time by poking at |
If the decorators are for internal use only, I suppose we could do it as a unit test instead? @shoyer, were these decorators intended for public consumption? |
Indeed these decorators are for internal use only. The signature verification could certainly be done in unit tests only. When I tested it originally, it did not have a material effect on import times so I didn't bother. |
|
@rossbar will you be picking this up again? |
|
As it stands this would increase import time which is a blocker. The next step would be to investigate getting rid of the signature verification at import time, but I don't plan on getting into that in this PR so I'll close it. |
Refactor functions in
np.ma.coreandnp.core.overridesto replace uses of functions from thecompat._inspectmodule to the standardinspectmodule.Note in both cases I chose to use the class-based
inspect.SignatureAPI to replace uses of the deprecatedinspect.getargspec. If it is preferred, the use of theSignaturecould be replaced instead with usage ofinspect.getfullargspec, which is more similar to the original.