-
Notifications
You must be signed in to change notification settings - Fork 210
feat(typing): Introduce PluginName NewType so plugin backends can type check
#3794
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,7 +1,7 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from types import ModuleType | ||
| from typing import TYPE_CHECKING, Literal | ||
| from typing import TYPE_CHECKING, Literal, NewType | ||
|
|
||
| from narwhals._typing_compat import TypeVar | ||
| from narwhals._utils import Implementation, _NoDefault | ||
|
|
@@ -91,7 +91,25 @@ | |
| - An Implementation, such as: `Implementation.DASK`, `Implementation.PYSPARK`, ... | ||
| """ | ||
|
|
||
| BackendT = TypeVar("BackendT", bound=Backend) | ||
| PluginName = NewType("PluginName", str) | ||
| """Name of a plugin backend's [entry point](https://packaging.python.org/en/latest/specifications/entry-points/). | ||
|
|
||
| Plugin backends are discovered at runtime, so their names cannot join the | ||
| `Literal` unions that describe the built-in backends. `PluginName` is a | ||
| [`NewType`](https://typing.python.org/en/latest/spec/aliases.html#newtype): | ||
| type checkers treat it as a distinct subtype of `str`, meaning | ||
|
FBruzzesi marked this conversation as resolved.
Outdated
|
||
|
|
||
| - an arbitrary `str` is still rejected where a `backend` is expected, and | ||
| - a value explicitly wrapped as `PluginName("...")` is accepted. | ||
|
|
||
| The contract is that a wrapped string **must** name an installed plugin's | ||
| entry point in the `narwhals.plugins` group. | ||
|
dangotbanned marked this conversation as resolved.
Outdated
|
||
|
|
||
|
dangotbanned marked this conversation as resolved.
|
||
| Signatures that dispatch to plugins include it in the `IntoBackend` | ||
| parameter, e.g. `IntoBackend[EagerAllowed | PluginName]`. | ||
| """ | ||
|
|
||
| BackendT = TypeVar("BackendT", bound=Backend | PluginName) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for this! Much better idea than I had, while getting to the same goal π₯³ Edit: woops that was meant to start the review |
||
| IntoBackend: TypeAlias = BackendT | ModuleType | ||
| """Anything that can be converted into a [`narwhals.Implementation`][]. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,7 @@ | |
| from typing_extensions import Self | ||
|
|
||
| from narwhals._compliant import CompliantSeries | ||
| from narwhals._typing import EagerAllowed, IntoBackend, NoDefault | ||
| from narwhals._typing import EagerAllowed, IntoBackend, NoDefault, PluginName | ||
| from narwhals.dataframe import DataFrame, MultiIndexSelector | ||
| from narwhals.dtypes import DType | ||
| from narwhals.typing import ( | ||
|
|
@@ -121,7 +121,7 @@ def from_numpy( | |
| values: _1DArray, | ||
| dtype: IntoDType | None = None, | ||
| *, | ||
| backend: IntoBackend[EagerAllowed], | ||
| backend: IntoBackend[EagerAllowed | PluginName], | ||
| ) -> Series[Any]: | ||
| """Construct a Series from a NumPy ndarray. | ||
|
|
||
|
|
@@ -186,7 +186,7 @@ def from_iterable( | |
| values: Iterable[Any], | ||
| dtype: IntoDType | None = None, | ||
| *, | ||
| backend: IntoBackend[EagerAllowed], | ||
| backend: IntoBackend[EagerAllowed | PluginName], | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note Not a review, just trying to add context from our conversation on discord If you are downstream from Narwhals and make use of our typing - we have been suggesting (#3149 (comment)) you write things like: backend: IntoBackend[EagerAllowed]Following this PR, that type describes exactly the same thing. If downstream wants to opt-in to plugins, the update to their typing small: - backend: IntoBackend[EagerAllowed]
+ backend: IntoBackend[EagerAllowed | PluginName]But it gives a very clear signal (to their users) that plugins should work.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All this should go somewhere! but where exactly? π§
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also made me realize that one should import from two modules: from narwhals.typing import IntoBackend, EagerAllowed
from narwhals.plugins import PluginName
...
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
We could do either the PR description or the release notes until we find somewhere else? Both can work too π |
||
| ) -> Series[Any]: | ||
| """Construct a Series from an iterable. | ||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.