search

vega / altair

Declarative statistical visualization library for Python
https://altair-viz.github.io/
BSD 3-Clause "New" or "Revised" License
9.37k stars 793 forks source link

ci: Support Python 3.13 #3591

Closed dangotbanned closed 1 week ago

dangotbanned commented 2 months ago

Resolves https://github.com/vega/altair/issues/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

mattijn commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

dangotbanned commented 3 weeks ago

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 commented 3 weeks ago

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 commented 3 weeks ago

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

dangotbanned commented 2 weeks ago

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 commented 2 weeks ago

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 commented 2 weeks ago

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:

https://github.com/vega/altair/blob/211f7c517d7c005af521b242e51e414a084b8350/tests/vegalite/v5/test_api.py#L1614-L1656

I had to change those a bit when dropping 3.8 to fix https://github.com/vega/altair/pull/3647#issuecomment-2426312642

Interesting if they cause problems in both cases

dangotbanned commented 2 weeks ago

@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 (https://github.com/narwhals-dev/narwhals/pull/1094) but I must say I'm feeling a bit out of my depth on this one

MarcoGorelli commented 2 weeks ago

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 https://github.com/vega/altair/pull/3672

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

dangotbanned commented 2 weeks ago

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 🙏

mattijn commented 2 weeks ago

Indeed thanks @MarcoGorelli! Appreciated👍