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

Passing basic polars dataframes to alt.Chart results in Pylance errors #3294

Closed
christopherball opened this issue Dec 21, 2023 · 4 comments · Fixed by #3297
Closed

Passing basic polars dataframes to alt.Chart results in Pylance errors #3294

christopherball opened this issue Dec 21, 2023 · 4 comments · Fixed by #3297
Assignees
Labels

Comments

@christopherball
Copy link

I took the default boilerplate chart code from an altair example and simply converted the cars Pandas DataFrame to a Polars one (for testing purposes as I am using Polars), and while the code appeared to still execute, Pylance is catching some type issues. Restarted VSCode and still see the same problem:

image

@binste binste self-assigned this Dec 21, 2023
@christopherball
Copy link
Author

christopherball commented Dec 21, 2023

Have additionally confirmed this same exact error surfaces with a simple native Polar dataframe to rule out conversion-related issues:

import polars as pl
import numpy as np
import altair as alt

df = pl.DataFrame(
    {
        "a": range(5),
        "b": np.random.rand(5),
    }
)

alt.Chart(df).mark_point().encode(
    x="a",
    y="b",
).interactive()

Until this typing issue is fixed, all developers using Altair with Polars DataFrames will see this error and be forced to place # type: ignore on every alt.Chart call (assuming they are using Pylance of course).

@dangotbanned
Copy link
Contributor

dangotbanned commented Dec 22, 2023

I was just about to open an issue on this, been having the same problem since I updated to 5.2.0.

Earlier this week I did some digging, and although I don't know enough about the altair codebase to resolve this, I do have some notes that might speed up a fix for the core devs.

Minimal repro

import altair as alt
import polars as pl

def make_chart(chart: alt.Chart) -> alt.Chart:
    return chart.encode(x="x", y="y").mark_point()

df: pl.DataFrame = pl.DataFrame({"x": [1, 1, 2, 3, 4], "y": [1, 2, 3, 4, 5]})

# all of these produce identical, valid charts
make_chart(alt.Chart(df)) # should be the only type check issue
make_chart(alt.Chart(df.to_arrow()))
make_chart(alt.Chart(df.to_pandas()))

Notes

@jonmmease
Copy link
Contributor

Thanks for the reports @christopherball and @dangotbanned

We can probably fix this particular issue by changing to a Protocol definition as @dangotbanned suggested. But I just tried running pyright on our code base and it turns up hundreds of errors that mypy doesn't flag, so I'm not sure what to make of this. It may be difficult to test this change in isolations without a major effort to satisfy pyright.

@binste do you have any thoughts here?

@binste
Copy link
Contributor

binste commented Dec 23, 2023

Thanks for the report @christopherball and the very useful debugging information @dangotbanned! Fully agree, we should switch the definitions to Protocols instead of ABC as other libraries (of course) don't use subclasses of our ABCs.

I'll submit a PR soon to fix this. Just switching to Protocols does not yet fully solve it, there is somehow some other difference in the protocols that Polars uses and that we use. I'll try to track it down.

Btw, the issue can also be replicated with mypy. Regarding pyright errors on our code base, I think as mypy is the reference implementation for a static type checker, we should mainly aim to satisfy mypy. Pyright is in some ways more strict/follows different rules. In this case, it's in general a wrong type annotation and so we definitely want to fix it.

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