refactor: Replace ad-hoc dispatch with custom @singledispatch#3410
refactor: Replace ad-hoc dispatch with custom @singledispatch#3410FBruzzesi merged 28 commits intodtypes/supertypingfrom
@singledispatch#3410Conversation
This is more generally useful and a LOT easier to read from the outside
narwhals/_dispatch.py
Outdated
| def register( # noqa: D417 | ||
| self, tp: type[Any], *tps: type[Any] | ||
| ) -> Callable[[Passthrough], Passthrough]: | ||
| """Register types to dispatch via the decorated function. | ||
|
|
||
| Arguments: | ||
| *tps: One or more **concrete** types. |
There was a problem hiding this comment.
I've left out validating the types for now.
3.9:@functools.singledispatchsupportstype[Any]3.10: Same support, but that's the version which introducestypes.UnionType3.11:@functools.singledispatchsupportstyping.Union | types.UnionType | type[Any]
Since #3204 is not resolved, I'd rather leave this runtime check until then
…s/supertyping-dispatch
…s/supertyping-dispatch
…s/supertyping-dispatch
|
@FBruzzesi
I think this was a documentation issue on my part. Do you still have the opinion that (https://github.com/narwhals-dev/narwhals/blob/308f389f2e68079053e9c659394cdd0d4207410d/narwhals/_dispatch.py) is intense to maintain?
I thought replacing lots of other non-supertyping stuff wouldn't be a good idea in this PR, since it was based on Here's three unrelated use-cases (dtypes/supertyping-dispatch...@just_dispatch-use-cases).
I do feel the need to stress that these are not the only places that can use it! 😅 |
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks @dangotbanned - I like this implementation much more than the first one.
I can see the benefits, and removing _SupertypeCase is already a big advantage I think.
It's a bit unclear how the typing issues get fixed with this approach, but I guess I will need an editor to better understand all the details 🙈
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks @dangotbanned - last comment to resolve is #3410 (comment)
One additional comment I have, that might be worth adding in just_dispatch docs is that we need to make sure that this is used only internally (i.e. functions that are not publicly exposed) as we don't do anything to maintain information such as docstrings. I didn't give it a try in an editor, but I would expect that to be the case as we go from a function to an instance wrapping such function.
| assert dispatch_no_bound(i64) == "i64" | ||
| assert repr_int(i64) == "i64" | ||
| assert dispatch_no_bound(nw.UInt8()) == "uint8" | ||
| assert dispatch_no_bound(stdlib_decimal) == "decimal" |
There was a problem hiding this comment.
Just to double check with you: there is no type checker error because regardless of the input argument annotation (DType), after being decorated, all we care about is the return type in the annotation (i.e. JustDispatch[str]).
There was a problem hiding this comment.
Just to double check with you: there is no type checker error because regardless of the input argument annotation (
DType), after being decorated,
Yeah that is to be expected (at least partially)
So the @just_dispatch-decorated function gets this signature:
narwhals/narwhals/_dispatch.py
Lines 50 to 52 in ddd5d98
I actually made it stricter than [@functools.singledispatch], which doesn't require1 an argument:
def __call__(self, /, *args: Any, **kwargs: Any) -> _T: ...Besides that tweak, I need to note this only applies to the initally decorated function.
You can see in the old tests how all others still work as expected
Why?
I had initially planned to use upper_bound to derive the type, but removed it in (02b7811).
Partially that was to get mypy off my back 😅 - however the more I thought about it - the only accurate type is object.
Here's a contrived example, but it shows that you can register any type:
Show code
import polars as pl
from polars.datatypes import classes as pl_dtypes
import narwhals as nw
from narwhals._dispatch import just_dispatch
from narwhals._utils import qualified_type_name
from narwhals.dtypes import DType
from narwhals.dtypes._utils import validate_into_dtype
from narwhals.typing import IntoDType # noqa: TC001
@just_dispatch
def into_dtype(obj: IntoDType) -> DType:
if isinstance(obj, DType):
return obj
validate_into_dtype(obj)
return obj()
@into_dtype.register(pl_dtypes.DataTypeClass)
def from_polars_meta(obj: type[pl.DataType] | pl_dtypes.DataTypeClass) -> DType:
return into_dtype(getattr(nw, obj.__name__))
@into_dtype.register(
*pl_dtypes.SignedIntegerType.__subclasses__(),
*pl_dtypes.UnsignedIntegerType.__subclasses__(),
pl.Float32,
pl.Float64,
pl.String,
pl.Boolean,
pl.Binary,
pl.Date,
pl.Time,
pl.Categorical,
pl.Unknown,
pl.Object,
)
def from_polars(obj: pl_dtypes.DataType) -> DType:
return from_polars_meta(obj.base_type())
ok_1 = into_dtype(nw.Date)
ok_2 = into_dtype(nw.Date())
ok_3 = into_dtype(pl.Date)
ok_4 = into_dtype(pl.Date())
print("\n".join(qualified_type_name(dtype) for dtype in [ok_1, ok_2, ok_3, ok_4]))narwhals.dtypes._classes.Date
narwhals.dtypes._classes.Date
narwhals.dtypes._classes.Date
narwhals.dtypes._classes.Date
I didn't declare the function supported some types, but made it possible to do so later 😄
Footnotes
-
It does runtime validation to enforce
>=1argument, but that isn't helpful for typing 😢 ↩
| assert dispatch_upper_bound(stdlib_decimal) == "need to be explicit" | ||
|
|
||
|
|
||
| def test_just_dispatch_no_auto_subclass(dispatch_no_bound: JustDispatch[str]) -> None: |
There was a problem hiding this comment.
Really nice test with Datetime distinction between main and v1 👌🏼
Description
Adds a slimmed-down version of
@functools.singledispatch, with the initial use replacing_supertyping._same_supertypein #3396.I can see a few other places (particularly
DType-related) that could later benefit.So, I went ahead and added more tests + docs than I would normally do for an internal tool.
Why not
@functools.singledispatch?Most of the stdlib code is dedicated to two features I don't want to use.
mro-based dispatch
That allows you to register
ABCs (likeMapping,Iterable, etc) or any regular class and have it's subclasses match.Definitely pretty clever stuff, but I'd prefer to have a simpler version that needs every class to be added explicitly
Registration via annotation
Forward references are resolved eagerly1 and only classes and unions of classes are supported.
IMO, that's just a complicated way of accepting
*types: type[Any]in the decorator - so I did that instead 😅Related issues
get_supertype#3396Footnotes
ibishas@lazy_singledispatchthat appears to solve this issue ↩