feat: Extend plugin support for IO and from_* methods/functions - #3753
feat: Extend plugin support for IO and from_* methods/functions#3753FBruzzesi wants to merge 26 commits into
from_* methods/functions#3753Conversation
This comment was marked as resolved.
This comment was marked as resolved.
|
Thanks both! @dangotbanned I think the implementation is not too far from what you have implemented, yet the machinery we have here is different. |
|
hey @dangotbanned just checking if you're ok with us going ahead with this |
|
@MarcoGorelli will do my best to take a peek later today Update: Really sorry, I ran out of time for today. Otherwise I will give it another try tomorrow 😘 |
Apologies again for the delay, but starting now |
dangotbanned
left a comment
dangotbanned
left a comment
There was a problem hiding this comment.
Thanks @FBruzzesi, this is a partial review with non-blocking suggestions/comments
| Functions and constructors which accept a `backend` argument can also dispatch to a | ||
| plugin. Users can pass: | ||
|
|
||
| - the plugin's entry point name (e.g. `backend="narwhals-grizzlies"`), |
There was a problem hiding this comment.
Note
My main issue is I would like there to be a single valid string per-plugin
I guess this is my bad as I missed what we have documented for how to name a plugin:
- an entrypoint defined in a
pyproject.tomlfile:[project.entry-points.'narwhals.plugins'] narwhals-<library name> = 'narwhals_<library name>'
If you compare that to what I have here, the namespacing is provided by the entry-points group:
Lines 63 to 65 in c57e72c
Which is the same pattern given as an example for https://packaging.python.org/en/latest/guides/creating-and-discovering-plugins/#using-package-metadata
Suggestion
Could we just have 1 option for plugins, which is:
- - the plugin's entry point name (e.g. `backend="narwhals-grizzlies"`),
- - the plugin's module name (e.g. `backend="narwhals_grizzlies"`),
- - or the plugin's module itself (e.g. `backend=narwhals_grizzlies`).
+ the plugin's entry point name (e.g. `backend="grizzlies"`)I understand we started with passing modules around, but it doesn't fit into the type system like Literal["grizzlies"] or even LiteralString could.
If we need to support modules (e.g. there is high usage of nw.get_native_namespace) then sure, have that option too
There was a problem hiding this comment.
I don't think this is a good strategy to be honest. Yesterday we were discussing the "plain dict" plugin name, and it might not come with a narwhals prefix. All we need is to register the entrypoint within the "narwhals.plugins" section: [project.entry-points.'narwhals.plugins'].
Our test-plugin is also not following the narwhals-prefix pattern
There was a problem hiding this comment.
The dash-underscore string duality (e.g. "narwhals-daft" vs "narwhals_daft") mirrors the pypi name vs import-name reality that many other modules ended up having. Matching both in _find_plugin costs three lines and is "forgiving", I don't think it's too ambiguous (as we document it).
I am fine to support only one as string, and I would lean more toward the pypi name
There was a problem hiding this comment.
I split this from (#3753 (review)) and ended it early after discovering this - which is blocking for me.
There are many different ways we could address the issue, but generally we need to update our typing so that plugins integrate with our typing.
That doesn't mean a plugin gets the same level of typing support as builtin backends.
But it should mean that you do not get a type error by doing things correctly
|
Alright, changes since last iteration:
|
I'm removing "changes requested" because we unblocked that in #3794
thanks! should we interpret this as approval? |
Those sets are disjoint @MarcoGorelli! 😉 I was trying to avoid repeating (#3753 (comment)), since I haven't reviewed (#3753 (comment)). I suppose here I can add that I've reviewed a non-blocking change too (#3793). @FBruzzesi this PR is next on my list along with (#3685) and (#3552). Personally, I will spend 30mins-2hrs on most reviews. Sometimes they are quicker, but they can also stretch to a full day. Footnotes
|
| def _series_like( | ||
| reference: SeriesT, values: Iterable[Any], dtype: IntoDType | None | ||
| ) -> SeriesT: | ||
| """Build a Series from `values` using `reference`'s own backend.""" | ||
| source = reference._compliant_series | ||
| compliant = type(source).from_iterable(values, name="", context=source, dtype=dtype) | ||
| return reference._with_compliant(compliant) |
There was a problem hiding this comment.
Note
Just a quick thought, not a review and not blocking
I was a little confused here at first, but I think it is pointing at implementation still needing to be decoupled somehow?
For example, I'd want to be able to write this as:
reference.from_iterable("", values, dtype, backend=reference.backend)Or maybe even this?
reference.from_iterable("", values, dtype, backend=reference)Which could work by extending IntoBackend to include some new attribute/method like this:
reference.__narwhals_backend__There was a problem hiding this comment.
Thanks for the question. implementation is definitely not enough since it's UNKNOWN for plugins. I am not 100% sure what __narwhals_backend__ should do. We have __narwhals_namespace__ and __native_namespace__ that we can use. I would avoid introduce a third - I will think and check if we can use one of those rather than re-implementing something such as _series_like or introducing something new as __narwhals_backend__ would
There was a problem hiding this comment.
Partial review, I've got these guys left (anything without a comment I haven't got to yet):
-
docs/extending.md -
packages/test-plugin/src/test_plugin/dataframe.py -
packages/test-plugin/src/test_plugin/namespace.py -
src/narwhals/_utils.py- I need to follow-up on the comments RE typing
-
tests/plugins_test.py- I'm working on the typing in feat/extend-from-backend...tests/3753-typing-cov
-
src/narwhals/plugins.py
| ns = eager_namespace( | ||
| backend, | ||
| version=cls._version, | ||
| function_name="DataFrame.from_arrow", | ||
| hint_example="nw.DataFrame.from_arrow(df, backend='pyarrow')", | ||
| ) |
There was a problem hiding this comment.
I like this idea!
One tweak I would do is remove the hint_example parameter.
We still should show the example!
But, you could have a mapping on the other side like this instead:
{
"DataFrame.from_arrow": "nw.DataFrame.from_arrow(df, backend='pyarrow')",
"DataFrame.from_dict": "nw.DataFrame.from_dict({'a': [1, 2]}, backend='pyarrow')",
"DataFrame.from_dicts": "nw.DataFrame.from_dicts([{'a': 1}, {'a': 2}], backend='pyarrow')",
...: ...
}And just key into it using function_name?
| class PluginError(NarwhalsError): | ||
| """Exception raised when a plugin backend does not support the requested operation. |
| "IntoSeries", | ||
| "IntoSeriesT", | ||
| "LazyAllowed", | ||
| "NormalizedPath", |
dangotbanned
left a comment
dangotbanned
left a comment
There was a problem hiding this comment.
@FBruzzesi thanks for addressing so much in (#3753 (comment))
| # IO methods below follow the namespace contract used by `narwhals.functions` | ||
| # (see "IO functions: the namespace contract" in `docs/extending.md`). | ||
| # `read_*` are deliberately left unimplemented: `test_plugin` wraps dicts | ||
| # lazily, so it only supports `scan_*`. | ||
|
|
||
| def scan_csv( |
There was a problem hiding this comment.
I'm a bit confused by this, since the test suite implements them in a subclass?
In my head it seems like the test suite should aim to avoid importing from test_plugin and access things through the Plugin interface.
If we can't do that - then it would point to gaps in the definition of Plugin or Compliant* that we need to fill
There was a problem hiding this comment.
Things to consider
- Is
test_pluginenough, or do we require multiple packages? - If we need to import from
test_plugindirectly for a test, are there any alternative ways we could write it within the current definitions we have?
There was a problem hiding this comment.
FYI, I started trying to adapt some tests for improving typing coverage in (feat/extend-from-backend...tests/3753-typing-cov).
But now I'm thinking that parts of that 1 are solving the wrong problem and that addressing 1 would make the tests simpler
Footnotes
-
Adding typing to the dynamic creations of modules/plugins/namespaces ↩
There was a problem hiding this comment.
DictNamespace deliberately did not implement read_*, so that test_read_plugin_scan_only could assert the PluginError. But test_read_plugin_eager_namespace needed those same methods to exist, to cover the eager-read success path. So the test added a subclass to get a second namespace shape, and wrapping it in a types.ModuleType turned it into a second plugin. The subclass wasn't the goal, it was the workaround for a contradiction I'd created by keeping DictNamespace scan-only.
Addressed that in d7e5350
| def _plugin_io_namespace( | ||
| backend: IntoBackend[Backend | PluginName], method_name: str, /, *, version: Version | ||
| ) -> Any: |
There was a problem hiding this comment.
I tried changing the return type to PluginNamespace and that produces:
Show 2 pyright errors
narwhals/functions.py
narwhals/functions.py:611:34 - error: Cannot access attribute "read_csv" for class "PluginNamespace[CompliantDataFrameAny | CompliantLazyFrameAny, CompliantAny]"
Attribute "read_csv" is unknown (reportAttributeAccessIssue)
narwhals/functions.py:714:34 - error: Cannot access attribute "read_parquet" for class "PluginNamespace[CompliantDataFrameAny | CompliantLazyFrameAny, CompliantAny]"
Attribute "read_parquet" is unknown (reportAttributeAccessIssue)
2 errors, 0 warnings, 0 informationsThe places where this doesn't cause an error are already declaring the return inline with Any
Show scan_*
narwhals/src/narwhals/functions.py
Line 665 in c5fe618
narwhals/src/narwhals/functions.py
Line 788 in c5fe618
I believe this is why the requirements are explained in text here:
I'm struggling to make a suggestion without referring to dead horse. So I'm not making one.
I will say that when I tried to type this on *Namespace - it was possible - but got verbose very quickly
| return tuple(entry_point.name for entry_point in _discover_entrypoints()) | ||
|
|
||
|
|
||
| def _find_plugin(backend_name: str, /) -> ModuleType | None: |
There was a problem hiding this comment.
This suggestion (_find_plugin) starts a chain reaction of things to fix
| def _find_plugin(backend_name: str, /) -> ModuleType | None: | |
| def _find_plugin(backend_name: PluginName, /) -> ModuleType | None: |
This revealed:
Show 1 pyright error
narwhals/plugins.py
narwhals/plugins.py:97:61 - error: Argument of type "PluginName | Literal['pandas', 'cudf', 'modin', 'pyarrow', 'polars', 'pyspark', 'sqlframe', 'pyspark[connect]', 'dask', 'duckdb', 'ibis']" cannot be assigned to parameter "backend_name" of type "PluginName" in function "_find_plugin"
Type "PluginName | Literal['pandas', 'cudf', 'modin', 'pyarrow', 'polars', 'pyspark', 'sqlframe', 'pyspark[connect]', 'dask', 'duckdb', 'ibis']" is not assignable to type "PluginName"
"Literal['cudf']" is not assignable to "PluginName" (reportArgumentType)
1 error, 0 warnings, 0 informations
There was a problem hiding this comment.
Added a note in _find_plugin to justify why its input is a string:
Note:
The parameter is a plain `str`, not a `PluginName`: the module spelling is a
valid input and is *not* an entry point name.I know this is not what you want is something different: #3753 (comment)
As mentioned in such thread: I like the flexibility we allow by checking both if backend_name in {entry_point.name, entry_point.module}.
Maybe the note might be more explicit:
Note:
The parameter is a plain `str`, not a `PluginName`: `entry_point.module` is a
valid input, and only `entry_point.name` is a `PluginName`.| def _backend_namespace(backend: IntoBackend[Backend | PluginName], /) -> ModuleType: | ||
| """Resolve a backend which is not a Narwhals `Implementation` to a plugin namespace. |
There was a problem hiding this comment.
Fix 1 from _find_plugin:
| def _backend_namespace(backend: IntoBackend[Backend | PluginName], /) -> ModuleType: | |
| """Resolve a backend which is not a Narwhals `Implementation` to a plugin namespace. | |
| def _backend_namespace(backend: IntoBackend[PluginName], /) -> ModuleType: | |
| """Resolve a backend which is not a Narwhals `Implementation` to a plugin namespace. |
| return tuple(entry_point.name for entry_point in _discover_entrypoints()) | ||
|
|
||
|
|
||
| def _find_plugin(backend_name: str, /) -> ModuleType | None: |
There was a problem hiding this comment.
| def _find_plugin(backend_name: str, /) -> ModuleType | None: | |
| def _find_plugin(backend_name: str, /) -> Plugin | None: |
| return None | ||
|
|
||
|
|
||
| def _backend_namespace(backend: IntoBackend[Backend | PluginName], /) -> ModuleType: |
There was a problem hiding this comment.
| def _backend_namespace(backend: IntoBackend[Backend | PluginName], /) -> ModuleType: | |
| def _backend_namespace(backend: IntoBackend[Backend | PluginName], /) -> Plugin: |
| raise ValueError(msg) | ||
|
|
||
|
|
||
| def _plugin_hook(native_namespace: ModuleType, name: str, /) -> Any: |
There was a problem hiding this comment.
This is exclusively used with "__narwhals_namespace__", so the return should be:
| def _plugin_hook(native_namespace: ModuleType, name: str, /) -> Any: | |
| def _plugin_hook(native_namespace: ModuleType, name: str, /) -> PluginNamespace: |
But the signature could drop the name parameter too?
| # NOTE: `_hasattr_static` alone is not enough: `_series` and `_dataframe` may be | ||
| # `not_implemented` descriptors, which exist statically but raise on instance | ||
| # access, so the statically-retrieved attribute is checked against `not_implemented`. | ||
| eager_allowed = all( | ||
| (attr := getattr_static(namespace, name, None)) is not None | ||
| and not isinstance(attr, not_implemented) | ||
| for name in ("_series", "_dataframe") | ||
| ) |
There was a problem hiding this comment.
|
Thanks for all the inputs @dangotbanned 🙏🏼 I will check and (start to) address them in a bit |
|
No rush, as you may be able to tell from the review timestamps - this took me a while today 😅 |
|
@dangotbanned I tried to keep the commits scoped to single or grouped comments you made. Hopefully it makes them easier to review for you. Please close the threads yourself as you consider them closed - some of them I couldn't go ahead with your suggestion fully, mostly because of #3753 (comment) |
3 participants
Description
The core changes are in the
eager_namespacefor the backend resolution logic, which allows forbackend=arguments to resolve for Narwhals plugins: users can pass a plugin's entry point name, module name, or the module itself.{read,scan}_{csv,parquet}dispatch to same-named top-level plugin hooks.from_*,new_series,DataFrame.from_*,Series.from_*) work with zero extra plugin code if the plugin's compliant namespace implements theEagerNamespaceprotocol.Most of the lines changes are in
packages/test-pluginandtests/plugins_test.pyso that we have coverage for the functionalities.What type of PR is this? (check all applicable)
Related issues
scan_csvfor plugins #3713Implementationfor plugins #3042: thenarwhals.pluginsentry-point group is the registration mechanism we have. Not sure we can do much more than this dispatching