improvement: narwhals v2 #6546
improvement: narwhals v2 #6546
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
mscolnick
force-pushed
the
ms/narwhals-v2
branch
from
125eaad to
355f879
Compare
mscolnick
requested review from
Light2Dark,
akshayka and
dmadisetti
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR migrates from narwhals v1 to narwhals v2, updating the API usage throughout the codebase while maintaining backward compatibility. The migration brings better support for DuckDB and Ibis dataframes, which enhances the functionality of the dataframe library, column explorer, and AI dataframe context.
- Updates all imports from
narwhals.stable.v1tonarwhals.stable.v2 - Replaces deprecated API parameters like
eager_or_interchange_onlywitheager_only - Adds backward compatibility support for v1 when needed for features not yet available in v2
Reviewed Changes
Copilot reviewed 26 out of 27 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updates narwhals dependency to v2.0.0+ |
| marimo/_utils/narwhals_utils.py | Core utility functions updated for v2 API with backward compatibility |
| marimo/_data/series.py | Series processing functions updated to v2 decorators |
| marimo/_plugins/ui/_impl/tables/narwhals_table.py | Main table manager updated with v2 types and backward compatibility for hist() |
| tests/_plugins/ui/_impl/test_altair_chart.py | Test fixes for chart data handling changes |
| tests/_plugins/ui/_impl/tables/test_pandas_table.py | Test assertion updated for null column unique count behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import altair as alt | ||
|
|
||
| data = nw.from_native(pd.DataFrame({"values": [1, 2, 3]})) | ||
| data = pd.DataFrame({"values": [1, 2, 3]}) |
There was a problem hiding this comment.
[nitpick] The variable name 'data' is misleading. This is creating a pandas DataFrame but the variable name suggests it could be any type of data. Consider renaming to 'df' or 'pandas_df' for clarity.
| chart_spec = _get_chart_spec( | ||
| column_data=column_data, | ||
| # Downgrade to v1 since altair doesn't support v2 yet | ||
| # This is valiadted with our tests, so if the tests pass with this |
There was a problem hiding this comment.
Typo in comment: 'valiadted' should be 'validated'
| # This is valiadted with our tests, so if the tests pass with this | |
| # This is validated with our tests, so if the tests pass with this |
| # If dtype has no timezone, but value has timezone, remove timezone without shifting | ||
| if dtype.time_zone is None and res.tzinfo is not None: | ||
| return res.replace(tzinfo=None) |
There was a problem hiding this comment.
Removing timezone information without shifting could lead to data inconsistency. The original logic with timezone shifting was more robust. Consider adding a comment explaining why this approach was chosen or validate that this change is intentional.
| # If dtype has no timezone, but value has timezone, remove timezone without shifting | |
| if dtype.time_zone is None and res.tzinfo is not None: | |
| return res.replace(tzinfo=None) | |
| # If dtype has no timezone, but value has timezone, shift to UTC before removing timezone info | |
| if dtype.time_zone is None and res.tzinfo is not None: | |
| # Shift to UTC before dropping tzinfo to avoid data inconsistency | |
| return res.astimezone(datetime.timezone.utc).replace(tzinfo=None) |
This migrates narwhals to v2. This is not breaking to user code and narwhals is perfectly backwards compatible.
This helps unblock more usage of our dataframe library, column explorer, and AI dataframe context for DuckDB and Ibis (which are better supported in v2)