search

rstudio / pins-python

https://rstudio.github.io/pins-python/
MIT License
52 stars 12 forks source link

Support Python 3.12 #248

Closed nathanjmcdougall closed 4 months ago

nathanjmcdougall commented 4 months ago

I note that the CI / Tests pipeline hasn't triggered, but it would be pretty important to run this before considering this PR.

isabelizimm commented 4 months ago

I appreciate you turning these tests on 🚀

And, to clarify why CI/Tests does not run immediately since forks don't have access to the secrets used to test across all the backends. On initial PR creation from a fork, you'll get the secret-less versions of the tests until I tell git to run all the cloud-based tests. No action needed on your part! That step is part of my review process 💭

nathanjmcdougall commented 4 months ago

Thanks for explaining that.

The new test failed since the adlfs==2022.2.0 package in the test extra in setup.cfg didn't support 3.12 via an issue similar to this one: https://github.com/jdidion/atropos/issues/135

I have bumped adlfs to the latest version. However, I am not sure why it is pinned in the first place; it would be good to find out and potentially this should be documented in a comment. I think it should be a safe change since this is just for testing purposes.

isabelizimm commented 4 months ago

Going off historical commits/PRs, it looks like there was a point in time where the latest version of adlfs was broken for our CI, although I don't have insight into exactly what that failure was. I agree the pin is pretty safe since it is just for testing! I have a slight preference to set it >=2024.4.1, which feels less surprising than pinning a specific version...which I know is not how it was 🙈 but that way we'll hopefully not end up in a situation with a 2 year old, potentially stale version again.

nathanjmcdougall commented 4 months ago

I agree, that's better, I've made that change.