chore: Isolate interchange-level support#3721
Conversation
Going scorched-earth to find out who's using it downstream Part 1 of (#3558 (comment))
Only 4 failing tests left
The error was checking a flag that we didn't set?
|
thanks for looking into this just noting that whatever solution we go with shouldn't require downstream libraries (marimo / altair / ....) to change anything on their side. i'm fairly confident that it should be possible to remove this from v2/main without affecting v1 usage at all |
100% agreed @MarcoGorelli ! My plan now is to:
I'm still kinda in a fact-finding stage, but feel like I understand interchange better than yesterday 😄 |
It was raising in `Series.from_iterable` before, because of `list[bool]`
- Still quite a mess - No more dataframe in `IbisLazyFrame` - Probably too much typing
Not sure `to_native` was ever supposed to work on interchange, but they're testing it
| @property | ||
| def _native_frame(self) -> Original_co: | ||
| return self._df |
There was a problem hiding this comment.
I added this alias temporarily so that to_native keeps working.
But we should probably update these guys:
narwhals/src/narwhals/translate.py
Lines 111 to 114 in 35e1681
- if isinstance(narwhals_object, BaseFrame):
- return narwhals_object._compliant_frame._native_frame
- if isinstance(narwhals_object, Series):
- return narwhals_object._compliant_series.native
+ if isinstance(narwhals_object, (BaseFrame, Series)):
+ return narwhals_object._compliant.nativeMay reveal some more downstream dependencies
- Everything left is fully covered - pretty sure marimo needs some more
- We don't have the `DataFrame | LazyFrame` complexity anymore - It now has 1 **less** type parameter!
If two modules are dependent on one another, they are one module
just checking the thing that's passed now
dangotbanned
changed the title
|
If anyone can point me towards evidence of downstream use cases (that aren't in the description) that this will break please let me know! I spent some time searching, but I'm open to widening the API if shown something new 🙂 |
@MarcoGorelli I'm still in agreement with you on this ... It looks to stem from (vega/altair#3452 (review)), which I think was well-intentioned. ExampleHere we have a very simple chart. I was suprised to find that these steps didn't trigger import altair as alt
import polars as pl
df = pl.DataFrame({"x": [1, 2, 3], "y": [4, 5, 6]})
chart = alt.Chart(df).mark_point().encode(x="x", y="y")Reading back through (vega/altair#3452) - the reason was to avoid a copy to This call 2 manages to hit chart.to_dict()Show logs (polars)
Now here's the same thing, but using import altair as alt
import polars as pl
import duckdb
df = pl.DataFrame({"x": [1, 2, 3], "y": [4, 5, 6]})
chart = alt.Chart(duckdb.from_arrow(df)).mark_point().encode(x="x", y="y")This call increases to 10 times in total, with 4 different objects: chart.to_dict()Show logs (duckdb)
Questions
Solution(s)?For DuckDB there are two things that wouldn't have been available back in (vega/altair#3452):
For everything else, I feel like wrapping the native object early would be the place to start? Footnotes
|
FBruzzesi
left a comment
FBruzzesi
left a comment
There was a problem hiding this comment.
Thanks @dangotbanned - I left a couple of comments, the real blocker is _should_interchange regression and maybe implementation.
Another doubt I have is about the introduction of two new # pragma: no cover:
_duckdb/utils.pyaddedif version is Version.V1: # pragma: no cover_duckdb/dataframe.pyaddedif validate_backend_version: # pragma: no cover
| def _should_interchange(tp_native: type[Any]) -> TypeIs[type[DataFrameLike]]: | ||
| if not _hasattr_static(tp_native, "__dataframe__"): | ||
| return (duckdb := get_duckdb()) and issubclass(tp_native, duckdb.DuckDBPyRelation) | ||
| has_top_level_df = (get_polars, get_pandas, get_dask_dataframe, get_modin) |
There was a problem hiding this comment.
cudf is missing from this list which would lead to a regression "demoting" cudf.DataFrame's to interchange level for v1.
More generally this function is an inversion of logic (from inclusion to exclusion) and seems quite hard to maintain/keep in sync, for many reasons, one of which is that we don't typically look nor touch interchange stuff anymore - what about dog-feeding _is_native_dataframe / _is_<known_to_exclude>_lazyframe?
There was a problem hiding this comment.
cudfis missing from this list which would lead to a regression "demoting" cudf.DataFrame's to interchange level for v1.
Well spotted!
So I started out thinking cudf supported interchange, but removed it when I couldn't find it anywhere
I'm looking again now and it looks like this is the support?
There is also a mention here of narwhals and interchange:
# polars 1.40 deprecated the DataFrame interchange protocol, which narwhals
# 2.16.0 still exercises; narwhals' filterwarnings=error turns the resulting
# DeprecationWarning into a failure. These run for every constructor, so skip
# them across all runs. test_get_level also relies on the interchange protocol.
# Remove once narwhals stops using the deprecated protocol.And possibly this was when it got removed:
There was a problem hiding this comment.
More generally this function is an inversion of logic (from inclusion to exclusion) and seems quite hard to maintain/keep in sync, for many reasons, one of which is that we don't typically look nor touch interchange stuff anymore
I'll do my best to walk through this tomorrow and explain how I landed here - there were a couple of concerns I was trying to balance
In terms of keeping in sync, IIUC this logic is frozen - which is why I opted to make it the case to handle first and keep everything confined to v1/"interchange"
There was a problem hiding this comment.
Yes CuDF supports (or at least supported) interchange and is deprecating/ has deprecated it, but some versions we support definitely can enter such path.
There was a problem hiding this comment.
@FBruzzesi hopefully these guys are helpful (https://github.com/narwhals-dev/narwhals/pull/3721/files/4bbdf114e109163077ab22bcab1c622f93dc3f23..955a677a1ebd5960686a8f13ebd90b91fb91c432)
Another thing I could speak on, but didn't address in those is: Why is this not using is_*_dataframe? or why is this checking on the type?
They're kinda the same question really 😅
I wanted to remove the concept of interchange from translate.py and v2, where it still lingered since (#2814).
So my choices were:
- Repeat all of
_from_native_implinv1, with the tweaks that allow "interchange" - Handle "interchange" in
v1, then defer to main
I picked option 2, since it requires less maintenance.
I also saw a way that option 2 could use caching for the very complicated checking 🙂.
But, that needed to switch to using the type vs the instance and so I couldn't reuse the existing guards
| msg = ( | ||
| f"{method_name!r} is not supported for interchange-level dataframes.\n\n" | ||
| "Hint: you probably called `from_native` on an object which isn't fully " | ||
| "supported by `narwhals.stable.v1`, yet implements `__dataframe__`." |
There was a problem hiding this comment.
If this can trigger from a DuckDBPyRelation, then that's actually not the case: it does not implement __dataframe__ (not sure about ibis case)
There was a problem hiding this comment.
This is a pretty interesting point.
What is being called interchange-level actually is entirely disjoint from it 😂
DuckDBPyRelationit does not implement__dataframe__
Which was news to be me before I started this PR!
(not sure about ibis case)
ibis does support it and I was using that directly in an earlier version (https://github.com/narwhals-dev/narwhals/blob/b7e533d9f2e10be02713617b3cfb556eb236da66/src/narwhals/_ibis/interchange.py)
I don't know what exactly to call this support - but I'll take any name you can think of 🙏
|
Thanks for the review @FBruzzesi
I believe all of those are from Essentially - I found no downstream using any of this and it has never been documented as working 1. I will add things back if needed (#3721 (comment)) - but I would like to see the use case 🙂 What we have here is what people are using, which is very similar to (#3558 (comment)), but also narwhals/src/narwhals/_interchange/lazyframe.py Lines 33 to 38 in 4bbdf11 narwhals/src/narwhals/_interchange/lazyframe.py Lines 42 to 48 in 4bbdf11 narwhals/src/narwhals/_interchange/lazyframe.py Lines 62 to 63 in 4bbdf11 Footnotes
|
- If they removed it, we'd want to treat they like duckdb - but only beyond that future version
| return duckdb_type | ||
| if isinstance_or_issubclass(dtype, dtypes.Enum): | ||
| if version is Version.V1: | ||
| if version is Version.V1: # pragma: no cover |
There was a problem hiding this comment.
Probably should remove this branch (same reason as #3721 (comment))
| We *could* use them with interchange-level support - but we have a better option at home. | ||
|
|
||
| Sadly this type is [not yet](https://www.youtube.com/watch?v=xUsPD7iwETY) spellable: | ||
|
|
||
| (DataFrameLike & (pl.DataFrame | pd.DataFrame | pa.Table | dd.DataFrame | mpd.DataFrame | cudf.DataFrame) | ||
| # | union | ||
| # intersection | ||
|
|
||
| We gathers all the types (that are already imported) which fit the the right-hand-side, | ||
| as we need to refine from the set produced by checking `__dataframe__`. |
There was a problem hiding this comment.
Best description I read in a while 😂
3 participants
Description
This PR started as an experiment and snowballed into an informed refactor™.
Note
TL;DR: In (#3558 (comment)) I had no clue what interchange meant
It now has clear boundaries, based on what people actually use
All related tests can be run with
uv run pytest tests/interchangeRelated issues
levelfrom main and v2 classes init #3558levelfrom main and v2 classes init #3558 (comment)Experiment
To find out what interchange really means:
Break interchange (e745e37)
Identify downstream breakage
Found out we have coverage from 3 downstream packages for
eager_or_interchange_only=TrueIdentify downstream usage
Altair
v1.from_native(eager_or_interchange_only=True)altair.Chart.to_dictaltair.utils.data.is_data_typealtair.utils.core.to_eager_narwhals_dataframealtair.utils.core.parse_shorthandv1.get_levelaltair.utils.core.to_eager_narwhals_dataframepa.Tableand then using narwhals as normalMarimo
Plotly
plotly.express._core.build_dataframev1.from_native(eager_or_interchange_only=True)v1.get_levelSummary
from_arrow(DataFrameLike, backend="pyarrow")Re-introduce interchange
pyfixestDataFrame.to_pandaspandas😔vegafusionv1.get_level() == "full"pyarrowConsider extending our downstream ci testing if I find someone using more thanfound nothing new!marimoPossible follow-ups
Found a good point to stop, but still had a few ideas that could follow.
I'm not in any rush to do this and would be happy to lock interchange up and never look back 😄
Docs
Be explicit about what is supported for
ibisandduckdb(and suggest usingv2instead)Feat
Almost all use cases I found for interchange support would be solved by a new
v1function like:Where the default is
"pyarrow"because Altair, Plotly, and Vegafusion already require it for interchange support.Tests
ibistests are 80% setup codeibis,duckdb,pl.DataFrame.__dataframe__, custompolarsentirely because of (feat(python!): Remove interchange from the python API pola-rs/polars#28152)columns,collect_schema,to_native,get_columnCI
Following #3629, we no longer test Vegafusion in downstream ci 1.
An alternative to reintroducing that workflow is to run Altair again, but with Vegafusion enabled.
That should get us some more coverage for
v1, without needing to compile from sourceFootnotes
Not sure why, guessing speed? ↩