Skip to content
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

fix: fix altair api break, cleanup DependencyManager #2006

Merged
merged 9 commits into from
Aug 12, 2024

Conversation

mscolnick
Copy link
Contributor

@mscolnick mscolnick commented Aug 12, 2024

Fixes #2005

Altair made an (arguably) breaking change. Their data transformers now pass narwhal dataframes instead of pandas, which breaks some downstream features in marimo. This PR now supports narwhals

This is nice actually nice since now we can pass polars, pyarrow, etc to altair charts and get back the polars, pyarrow, etc charts.

This also DRYs up DependencyManager

Copy link

vercel bot commented Aug 12, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 12, 2024 8:29pm
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 12, 2024 8:29pm

@@ -365,7 +365,7 @@ def visit_Call(self, node: ast.Call) -> None:
elif isinstance(first_arg, ast.JoinedStr):
sql = normalize_sql_f_string(first_arg)

if isinstance(sql, str) and DependencyManager.has_duckdb() and sql:
if isinstance(sql, str) and DependencyManager.duckdb.has() and sql:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be has_at_version? extract_statements isn't available in some earlier versions of duckdb.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yea i can add it here too

@@ -33,7 +33,7 @@ def sql(
Returns:
The result of the query.
"""
DependencyManager.require_duckdb("to execute sql")
DependencyManager.duckdb.require("to execute sql")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be require_at_version?


@staticmethod
def require_numpy(why: str) -> None:
def require(self, why: str) -> None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we also have a require_at_version?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i can add it, but I dont think it is used yet

polars = Dependency("polars")
numpy = Dependency("numpy")
altair = Dependency("altair", min_version="5.3.0", max_version="6.0.0")
duckdb = Dependency("duckdb")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

min_version=1.0.0? Or do we support duckdb < 1.0 in some places?

@MarcoGorelli
Copy link
Contributor

Hey, sorry to chime in, just wanted to

  1. say hi, I'm a fan of Marimo 😍
  2. apologise for having introduced the breaking change (I was worried that at least something would happen... 🥶 )
  3. invite you to please feel free to reach out if you need or would like any assistance with Narwhals
  4. understand whether this needs patching in Altair. From your comment "This is nice actually nice since now we can pass polars, pyarrow, etc to altair charts and get back the polars, pyarrow, etc charts." I think you're OK with the change, but it just should have been called out more explicitly as breaking?

Thanks 🙏

@mscolnick
Copy link
Contributor Author

Hi @MarcoGorelli - thanks for chiming in (and being a fan!)

  1. No worries. I'm not sure how to make this backward compatible - could just pass the native DF instead of narwhals' DF. Speaking for marimo, its fine for us to fix forward. But I am not sure how much this affects other downstream projects.
  2. I will definitely reach out, thank you for offering.
  3. Yea maybe a callout (although I found out first from a user instead of the changelog). This is a better change longer term for marimo. I guess in future, we can collaborate on RC releases to make sure they work with marimo.

@mscolnick mscolnick merged commit bf0dd47 into main Aug 12, 2024
21 of 31 checks passed
@mscolnick mscolnick deleted the ms/fix-altair-break branch August 12, 2024 20:55
Copy link

🚀 Development release published. You may be able to view the changes at https://marimo.app?v=0.7.20-dev13

@MarcoGorelli
Copy link
Contributor

just fyi, the next Altair version should address this vega/altair#3550

@mscolnick
Copy link
Contributor Author

thank you @MarcoGorelli!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Possible incompatibility when using marimo_csv transformer with altair 5.4.0
3 participants