feat(typing): Introduce PluginName NewType so plugin backends can type check - #3794
Conversation
| parameter, e.g. `IntoBackend[EagerAllowed | PluginName]`. | ||
| """ | ||
|
|
||
| BackendT = TypeVar("BackendT", bound=Backend | PluginName) |
There was a problem hiding this comment.
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
dangotbanned
left a comment
There was a problem hiding this comment.
Thanks @FBruzzesi, just a few suggestions/questions from me
| import narwhals.stable.v1.dependencies as nw_v1_dependencies | ||
| import narwhals.stable.v2.dependencies as nw_v2_dependencies | ||
| from narwhals import dependencies as nw_dependencies | ||
| from narwhals._typing import PluginName |
There was a problem hiding this comment.
This suggestion is only a part of it 😄
| from narwhals._typing import PluginName | |
| from narwhals.plugins import PluginName |
I guess the main part is, do we need to make a decision here on this bit of (#3753 (comment))?
All that is left is to decide where the wrapping in
PluginNamehappens
I see 3 options for that, but it could be a mix of them too:
Show me the money
- The plugin author does it explicitly, somewhere here (and/or alongside where they write
Implementation.UNKNOWN)narwhals/src/narwhals/plugins.py
Lines 107 to 114 in 314d185
- We wrap
EntryPoint.nameimplicitly herenarwhals/src/narwhals/plugins.py
Lines 54 to 56 in 314d185
- We expose
PluginNameand users/downstream do everything imperatively
I like first choice.
The second would block how we might solve How do we introduce the concepts of {Eager,Lazy}{Allowed,Only} for plugins, when we can't define them in a Literal?.
The third I'd be okay with as an escape hatch - but as the only option it would be cumbersome.
Maybe the simplest thing we could do in this PR is make PluginName accessible from narwhals.plugins?
It could be defined in _typing.py or in plugins.py
There was a problem hiding this comment.
That's a good point. I would also prefer option 1 if feasible. As you also mentioned, for now I will just make PluginName exposed in plugins.py
There was a problem hiding this comment.
As you also mentioned, for now I will just make
PluginNameexposed inplugins.py
Also made me realize that one should import from two modules:
One solution to that is make the symbol(s) available in multiple places
Examples
1
This module defines and exports 2 protocols.
While I was implementing them, having to import Implementation from _utils was giving me a headache.
So I made it available here too
2
This package imports and re-exports those 2 protocols, plus some other stuff
3
I defined PluginName here. I don't have a reason, but that is what I did 😂
narwhals/src/narwhals/_plan/typing.py
Line 137 in fbe56e2
I guess the only difference vs what you had was that this is typing.py vs _typing.py.
I probably wouldn't have blinked if the import came from anything public 😄
It isn't a perfect solution, as you might question which is the canonical one?
But idk, sometimes the world is a hierarchy and sometimes you have things that fit multiple boxes
| dtype: IntoDType | None = None, | ||
| *, | ||
| backend: IntoBackend[EagerAllowed], | ||
| backend: IntoBackend[EagerAllowed | PluginName], |
There was a problem hiding this comment.
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.
It does not indicate plugin support, meaning that there should be no expectation that downstream tests against plugins.
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.
I hope that downstream (authors) will be more likely to update their tests, to account for plugins, when these concepts are visible in each signature with a backend 🙂
There was a problem hiding this comment.
All this should go somewhere! but where exactly? 🧐
There was a problem hiding this comment.
Also made me realize that one should import from two modules:
from narwhals.typing import IntoBackend, EagerAllowed
from narwhals.plugins import PluginName
...There was a problem hiding this comment.
All this should go somewhere! but where exactly? 🧐
We could do either the PR description or the release notes until we find somewhere else?
For release notes I mean if we had a short highlight message in this style (https://github.com/vega/altair/releases#release-v5.5.0)
Both can work too 😅
| - Wrap an entry point name to pass it wherever a `backend` is expected, e.g. `PluginName("my-plugin")`. | ||
| - Add it to a signature's `IntoBackend` parameter to advertise plugin support, e.g. `IntoBackend[EagerAllowed | PluginName]`. | ||
|
|
||
| See the `narwhals.plugins` module for how plugin authors register entry points |
There was a problem hiding this comment.
So... when I went to add the reference via [...][], I realized that we have no api-reference for plugins 😭 that's a needed follow-up
There was a problem hiding this comment.
that's a needed follow-up
Agreed!
For now, something I discovered with (microsoft/pylance-release#6611) - is that it supports lots of stuff (by default) that we either:
- haven't even got configured for
mkdocstrings - aren't actually possible
Cross ref to a function in the same module, but isn't API reference
mkdocstrings has a support for 2 kinds of cross-refs that we haven't got enabled.
IIRC, the syntax was different and it didn't work (yet?) in zensical.
But it still wouldn't be able to do this wizardry 😄
Cross ref to 3rd-party symbols
We can do something similar by adding inventories (if they have one ibis-project/ibis#11723)
But this let's you link links direct to the source code 🤯
Try these out
diff --git a/src/narwhals/_typing.py b/src/narwhals/_typing.py
index 189a1543b..c47190a26 100644
--- a/src/narwhals/_typing.py
+++ b/src/narwhals/_typing.py
@@ -110,6 +110,16 @@ PluginName = NewType("PluginName", str)
See the `narwhals.plugins` module for how plugin authors register entry points
and the contract a wrapped name must satisfy.
+
+
+## Some examples of working cross-refs
+Try these out in your IDE
+
+- [`importlib.metadata.EntryPoint.name`][]
+- [`IntoBackend`][]
+- [`EagerAllowed`][]
+- [`narwhals.typing.LazyAllowed`][]
+- [`narwhals.plugins.Plugin`][]
"""
BackendT = TypeVar("BackendT", bound=Backend | PluginName)
There was a problem hiding this comment.
Doesn't seem to work with modules though. Will merge without it for now and follow up with proper docs


Description
Other prerequisite for #3753
What type of PR is this? (check all applicable)
Related
from_*methods/functions #3753 (comment))