-
Notifications
You must be signed in to change notification settings - Fork 210
feat: Extend plugin support for IO and from_* methods/functions
#3753
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
base: main
Are you sure you want to change the base?
Changes from 15 commits
c19b368
d24b570
19f4b56
2a18910
9f14d08
0195eb5
47e432b
e446236
314d185
3b1143d
bcb2e2c
56d9f42
d6ba539
bae0a80
c5fe618
2f681e6
a4d397c
b807ef0
e65a05d
b658306
fbf2c35
b8aa459
055a03e
3da5081
d7e5350
f5ab38e
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 |
|---|---|---|
| @@ -0,0 +1,13 @@ | ||
| # `narwhals.plugins` | ||
|
|
||
| For an overview of how to write a plugin, see [extensions and plugins](../extending.md). | ||
|
|
||
| ::: narwhals.plugins | ||
| handler: python | ||
| options: | ||
| members: | ||
| - Plugin | ||
| - PluginName | ||
| - from_native | ||
| show_source: false | ||
| show_bases: false |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,11 +3,13 @@ | |
| from typing import TYPE_CHECKING, Any | ||
|
|
||
| from narwhals._compliant import CompliantNamespace | ||
| from narwhals._utils import not_implemented | ||
| from test_plugin.dataframe import DictFrame, DictLazyFrame | ||
| from narwhals._utils import Implementation, not_implemented | ||
| from test_plugin.dataframe import DictDataFrame, DictFrame, DictLazyFrame | ||
|
|
||
| if TYPE_CHECKING: | ||
| from narwhals.typing import NormalizedPath | ||
| from narwhals.utils import Version | ||
| from test_plugin.series import DictSeries | ||
|
|
||
|
|
||
| class DictNamespace(CompliantNamespace[DictLazyFrame, Any]): | ||
|
|
@@ -17,9 +19,44 @@ def __init__(self, *, version: Version) -> None: | |
| def from_native(self, native_object: DictFrame) -> DictLazyFrame: | ||
| return DictLazyFrame(native_object, version=self._version) | ||
|
|
||
| @property | ||
| def _dataframe(self) -> type[DictDataFrame]: | ||
| return DictDataFrame | ||
|
|
||
| @property | ||
| def _series(self) -> type[DictSeries]: | ||
| from test_plugin.series import DictSeries | ||
|
|
||
| return DictSeries | ||
|
|
||
| # 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( | ||
|
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. 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
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. Things to consider
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. FYI, I started trying to adapt some tests for improving typing coverage in (feat/extend-from-backend...tests/3753-typing-cov). Footnotes
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.
Addressed that in d7e5350 |
||
| self, source: NormalizedPath, *, separator: str = ",", **kwds: Any | ||
| ) -> DictLazyFrame: | ||
| import csv | ||
| from pathlib import Path | ||
|
|
||
| with Path(source).open(newline="", encoding="utf-8") as file: | ||
| header, *rows = list(csv.reader(file, delimiter=separator)) | ||
| data = {name: [row[index] for row in rows] for index, name in enumerate(header)} | ||
| return DictLazyFrame(data, version=self._version) | ||
|
|
||
| def scan_parquet(self, source: NormalizedPath, **kwds: Any) -> DictLazyFrame: | ||
| import pyarrow.parquet as pq | ||
|
|
||
| data: DictFrame = pq.read_table(source, **kwds).to_pydict() | ||
| return DictLazyFrame(data, version=self._version) | ||
|
|
||
| # NOTE: `not_implemented.__get__` reads `instance._implementation` to build its | ||
| # error message, so `_implementation` itself must be a real value. | ||
| _implementation = Implementation.UNKNOWN | ||
|
|
||
| is_native: Any = not_implemented() | ||
| _expr: Any = not_implemented() | ||
| _implementation: Any = not_implemented() | ||
| corr: Any = not_implemented() | ||
| cov: Any = not_implemented() | ||
| len: Any = not_implemented() | ||
|
|
@@ -37,5 +74,3 @@ def from_native(self, native_object: DictFrame) -> DictLazyFrame: | |
| selectors: Any = not_implemented() | ||
| coalesce: Any = not_implemented() | ||
| struct: Any = not_implemented() | ||
| scan_csv: Any = not_implemented() | ||
| scan_parquet: Any = not_implemented() | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,73 @@ | ||
| from __future__ import annotations | ||
|
|
||
| from typing import TYPE_CHECKING, Any | ||
|
|
||
| from narwhals._utils import Implementation | ||
|
|
||
| if TYPE_CHECKING: | ||
| from collections.abc import Iterable | ||
|
|
||
| from typing_extensions import Self | ||
|
|
||
| from narwhals._utils import Version, _LimitedContext | ||
| from narwhals.series import Series | ||
|
|
||
|
|
||
| class DictSeries: | ||
| """Minimal eager series, kept to the smallest surface exercised in narwhals' tests.""" | ||
|
|
||
| _implementation = Implementation.UNKNOWN | ||
|
|
||
| def __init__( | ||
| self, values: Iterable[Any], *, name: str = "", version: Version | ||
| ) -> None: | ||
| self._values: list[Any] = list(values) | ||
| self._name = name | ||
| self._version = version | ||
|
|
||
| @classmethod | ||
| def from_iterable( | ||
| cls, | ||
| data: Iterable[Any], | ||
| /, | ||
| *, | ||
| context: _LimitedContext, | ||
| name: str = "", | ||
| dtype: Any = None, # noqa: ARG003 | ||
| ) -> Self: | ||
| return cls(data, name=name, version=context._version) | ||
|
|
||
| @classmethod | ||
| def from_numpy(cls, data: Any, /, *, context: _LimitedContext) -> Self: | ||
| return cls(data.tolist(), version=context._version) | ||
|
|
||
| def __narwhals_series__(self) -> Self: | ||
| return self | ||
|
|
||
| def __narwhals_namespace__(self) -> Any: | ||
| from test_plugin.namespace import DictNamespace | ||
|
|
||
| return DictNamespace(version=self._version) | ||
|
|
||
| @property | ||
| def native(self) -> list[Any]: | ||
| return self._values | ||
|
|
||
| @property | ||
| def name(self) -> str: | ||
| return self._name | ||
|
|
||
| def alias(self, name: str) -> Self: | ||
| return self.__class__(self._values, name=name, version=self._version) | ||
|
|
||
| def is_empty(self) -> bool: | ||
| return not self._values | ||
|
|
||
| def scatter(self, indices: Self, values: Self) -> Self: | ||
| data = list(self._values) | ||
| for index, value in zip(indices.native, values.native, strict=True): | ||
| data[index] = value | ||
| return self.__class__(data, name=self._name, version=self._version) | ||
|
|
||
| def to_narwhals(self) -> Series[Any]: | ||
| return self._version.series(self, level="full") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
If you compare that to what I have here, the namespacing is provided by the entry-points group:
narwhals/pyproject.toml
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:
I understand we started with passing modules around, but it doesn't fit into the type system like
Literal["grizzlies"]or evenLiteralStringcould.If we need to support modules (e.g. there is high usage of
nw.get_native_namespace) then sure, have that option tooThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
narwhalsprefix. 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_plugincosts 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