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

ci: Support Python 3.13 #3591

Merged
merged 18 commits into from
Nov 4, 2024
Merged

ci: Support Python 3.13 #3591

merged 18 commits into from
Nov 4, 2024

Conversation

dangotbanned
Copy link
Member

@dangotbanned dangotbanned commented Sep 14, 2024

Resolves #3587

Minimal/fallback support for 3.13.

Ideally I'd like to minimise hard dependencies we have on typing_extensions before October 2024. However, this would be the simpler option if it passes CI

Minimal/fallback support for `3.13`.

Ideally I'd like to minimise hard dependencies we have on `typing_extensions` before October 2024.
However, this would be the simpler option if it passes CI

Resolves #3587
@dangotbanned dangotbanned self-assigned this Oct 14, 2024
@mattijn
Copy link
Contributor

mattijn commented Oct 25, 2024

I think it is noble to wait for support for python 3.13 for all our upstream soft dependencies. But I'm also not against releasing before all our soft dependencies provide support (in my understanding pyarrow). What you think @dangotbanned?

@dangotbanned
Copy link
Member Author

I think it is noble to wait for support for python 3.13 for all our upstream soft dependencies. But I'm also not against releasing before all our soft dependencies provide support (in my understanding pyarrow). What you think @dangotbanned?

@mattijn nothing noble here 😄

I'm happy with releasing before pyarrow>=18.0.0 in principle.
The issue is we'd need to remove pyarrow from altair[all] (https://github.com/vega/altair/actions/runs/11448782729/job/31853033909?pr=3591)

Also maybe do some decoupling in tests/., if there are any imports that assume pyarrow is always installed.

Doing either of those is fine with me if you'd like to give it a try?


Personally though, I've got a few issues/PRs that I'd rather have ready for 5.5.0 (+ some others that are less pressing):

I think there's more value in delivering some/all of these vs a slightly earlier 5.5.0, if that makes sense?

@mattijn
Copy link
Contributor

mattijn commented Oct 25, 2024

Hmpf. Decoupling altair[all] is not something I was aiming for. Let's wait.

@dangotbanned
Copy link
Member Author

Hmpf. Decoupling altair[all] is not something I was aiming for. Let's wait.

No worries @mattijn

Also, IIRC pyarrow was blocking for geopandas - which I think is relevant to you?

@mattijn
Copy link
Contributor

mattijn commented Oct 25, 2024

Yeah, I assume that geospatial users of Altair are the minority, so if that's causing the delay, also feel a bit responsible for that.

@dangotbanned
Copy link
Member Author

Yeah, I assume that geospatial users of Altair are the minority, so if that's causing the delay, also feel a bit responsible for that.

Its not on you @mattijn!
just giving you a heads up if you were eager to use 3.13 is all

Seems to be able to build (sometimes) with these changes.
Still fails a good chunk of the tests
@dangotbanned
Copy link
Member Author

I'm all out of ideas on this one 😞

@mattijn @jonmmease @joelostblom @binste
Pinging in case any of you guys can make sense of the logs

@jonmmease
Copy link
Contributor

It looks like pip is first installing pyarrow 18, but then installs 17, which it tries to build from source since there aren't wheel published for Python 3.13. I think the issue is that ibis 9.5.0 has an upper bound on pyarrow of <18. See https://github.com/ibis-project/ibis/blob/9.5.0/pyproject.toml. This was relaxed on their man branch, but a new version hasn't been published yet.

I don't recall offhand which of our tests need ibis with the polars backend, but we could see if theres a way to rework them without it.

@dangotbanned
Copy link
Member Author

Thanks @jonmmease

I don't recall offhand which of our tests need ibis with the polars backend, but we could see if theres a way to rework them without it.

If that is the issue, these tests are the one's that need it:

def test_ibis_with_date_32():
ibis.set_backend("polars")
df = pl.DataFrame(
{"a": [1, 2, 3], "b": [date(2020, 1, 1), date(2020, 1, 2), date(2020, 1, 3)]}
)
tbl = ibis.memtable(df)
result = alt.Chart(tbl).mark_line().encode(x="a", y="b").to_dict()
assert next(iter(result["datasets"].values())) == [
{"a": 1, "b": "2020-01-01T00:00:00"},
{"a": 2, "b": "2020-01-02T00:00:00"},
{"a": 3, "b": "2020-01-03T00:00:00"},
]
@pytest.mark.skipif(
Version("1.5") > PANDAS_VERSION,
reason="A warning is thrown on old pandas versions",
)
@pytest.mark.xfail(
sys.platform == "win32", reason="Timezone database is not installed on Windows"
)
def test_ibis_with_vegafusion(monkeypatch: pytest.MonkeyPatch):
ibis.set_backend("polars")
df = pl.DataFrame(
{
"a": [1, 2, 3],
"b": [datetime(2020, 1, 1), datetime(2020, 1, 2), datetime(2020, 1, 3)],
}
)
tbl = ibis.memtable(df)
# "poison" `arrow_table_from_dfi_dataframe` to check that it does not get called
# if we use the vegafusion transformer
monkeypatch.setattr(
"altair.utils.data.arrow_table_from_dfi_dataframe", lambda x: 1 / 0
)
tbl = ibis.memtable(df)
with alt.data_transformers.enable("vegafusion"):
result = alt.Chart(tbl).mark_line().encode(x="a", y="b").to_dict(format="vega")
assert next(iter(result["data"]))["values"] == [
{"a": 1, "b": "2020-01-01T00:00:00.000"},
{"a": 2, "b": "2020-01-02T00:00:00.000"},
{"a": 3, "b": "2020-01-03T00:00:00.000"},
]

I had to change those a bit when dropping 3.8 to fix #3647 (comment)

Interesting if they cause problems in both cases

@dangotbanned
Copy link
Member Author

@MarcoGorelli
I was wondering if you had any ideas on if/how we could move this forward, prior to ibis>=10.0.0?

I'd been following the 3.13 PR you had (narwhals-dev/narwhals#1094) but I must say I'm feeling a bit out of my depth on this one

@MarcoGorelli
Copy link
Contributor

MarcoGorelli commented Nov 3, 2024

hey - I'd suggest removing Ibis from CI, I think we can use duckdb for that test, I'll make a PR

here you go #3672

(note that we continue to test Ibis in CI in Narwhals, so support for it should stay strong)

@dangotbanned
Copy link
Member Author

hey - I'd suggest removing Ibis from CI, I think we can use duckdb for that test, I'll make a PR

here you go #3672

(note that we continue to test Ibis in CI in Narwhals, so support for it should stay strong)

@MarcoGorelli looks to have done the job, thanks again 🙏

@dangotbanned dangotbanned marked this pull request as ready for review November 3, 2024 20:31
@mattijn
Copy link
Contributor

mattijn commented Nov 3, 2024

Indeed thanks @MarcoGorelli! Appreciated👍

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