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

Proof of concept: annotation-based argument extractor/replacer #274

Closed
wants to merge 2 commits into from

Conversation

peterbell10
Copy link
Contributor

This explores the posibility of automatically generating the argument extractor and replacer based on inline type annotations (for simple cases). So, for example scipy.fft.fft might look like:

@_dispatch
def fft(x: ua.typing.Dispatchable[ArrayLike, np.ndarray], n=None, axis=-1,
        norm=None, overwrite_x=False, workers=None, *, plan=None):
    pass

The signature is inspected ahead-of-time so as to minimize call overhead but a Python implementation is still too slow. ~400 ns for
even a simple f(x) signature. However, if it were lowered to C++ then the concept may still be viable.

This also relies on typing.Annotated to specify dispatch_type and coercible but typing.Annotated itself requires Python 3.9. It may be possible to emulate on earlier python versions, otherwise I think the dispatch metadata would need to go elsewhere. Perhaps something like:

@_dispatch([ua.DispatchableArg(name='x', dispatch_type=np.ndarray, coercible=True)])
def fft(x, n=None, axis=-1, norm=None, overwrite_x=False, workers=None, *, plan=None):
    pass

cc @thomasjpfan

This explores the posibility of automatically generating the argument
extractor and replacer based on inline type annotations (for simple
cases). So, for example `scipy.fft.fft` might look like:

```python
@_dispatch
def fft(x: ua.typing.Dispatchable[ArrayLike, np.ndarray], n=None, axis=-1,
        norm=None, overwrite_x=False, workers=None, *, plan=None):
    pass
```

The signature is inspected ahead-of-time so as to minimize call
overhead but a Python implementation is still too slow. ~400 ns for
even a simple `f(x)` signature. However, if it were lowered to C++
then the concept may still be viable.

This also relies on `typing.Annotated` to specify `dispatch_type` and
`coercible` but `typing.Annotated` itself requires Python 3.9. To
support older Pythons versions we'd have to move the annotations
out-of-line. Perhaps something like:

```python
_generate_arg_extractor_replacer(func, [
    DispatchableArg(name='a', dispatch_type='int', coercible=True),
    DispatchableArg(name='b', dispatch_type='float', coercible=False),
  ])
```
@peterbell10 peterbell10 marked this pull request as draft March 2, 2022 19:33
@hameerabbasi
Copy link
Collaborator

I'd be very supportive of this! It'd cover almost all use-cases besides np.block or something crazy like that. I think we could eventually even do np.concatenate and np.stack.

@BvB93
Copy link
Contributor

BvB93 commented Mar 3, 2022

@_dispatch
def fft(x: ua.typing.Dispatchable[ArrayLike, np.ndarray], n=None, axis=-1,
       norm=None, overwrite_x=False, workers=None, *, plan=None):
  pass

Should ArrayLike be interpretted as the annotation for x here by type checkers? If so, you may wanna trick them into treating ua.typing.Dispatchable as an alias for typing.Annotated, otherwise there is a big chance type checkers won't be able to understand the necasary logic behind __class_getitem__ (the heavy special casing behind typing.Annotated is not exactly helpful here either).

Also, something that might be important to remember is from __future__ import annotations, which converts all annotations into strings within a given module.

__all__ = ["Dispatchable"]

if not TYPE_CHECKING:
    class Dispatchable: ...  # Insert class details
else:
    from typing import Annotated as Dispatchable

def test_func(a: Dispatchable[str | list[str], str], b: int) -> None: ...

# Revealed type is "def (a: Union[builtins.str, builtins.list[builtins.str]], b: builtins.int)"
reveal_type(test_func)

@peterbell10
Copy link
Contributor Author

otherwise there is a big chance type checkers won't be able to understand the necasary logic behind __class_getitem__

Hrm you're right. It seems mypy doesn't actually evaluate the types at all, so __class_getitem__ is never run (see python/mypy#11501). Instead it special cases classes that inherit from Generic[T] which doesn't work here because coercible isn't a type.

In that case I'm thinking DispatchableArg is the way to go. It's compatible with all Python versions and isn't really any longer than writing Annotated[T, Dispatchable(...)].

@peterbell10 peterbell10 force-pushed the annotation-extractor-replacer branch from 6bc44fe to 19b72ed Compare March 3, 2022 15:03
@thomasjpfan
Copy link

In general, I like the idea of using a decorator because it simplifies the workflow for API authors.

Minor nit: If we are going with DispatchableArg, then I prefer this API:

@_dispatch(x=ua.DispatchableArg(type=np.ndarray, coercible=True))
def fft(x, n=None, axis=-1, norm=None, overwrite_x=False, workers=None, *, plan=None):
    pass

@peterbell10
Copy link
Contributor Author

@thomasjpfan I'm not sure that will work because the general decorator also needs to specify domain. So what if your function has an argument called domain. We could support dict(x=ua.DispatchableArg(...)) though I suppose. Not sure if that's actually cleaner though.

@thomasjpfan
Copy link

thomasjpfan commented Mar 4, 2022

Brainstorming: A dispatcher for a given domain:

dispatch = Dispatcher(domain="scipy.fft")

@dispatch.register(
	x=DispatchableArg(...)
)
def fft(x):
    ...

I think x= is slightly cleaner because x= looks more like a parameter. With there is some name="x" minor cognitive overhead connecting the string "x" to the function parameter x.

Edit: I see other issues with using kwargs. Likely name="x" is best.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants