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

Support Python 3.12 #248

Merged
merged 3 commits into from
Jul 19, 2024
Merged

Conversation

nathanjmcdougall
Copy link
Contributor

No description provided.

@nathanjmcdougall
Copy link
Contributor Author

nathanjmcdougall commented Jul 16, 2024

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

@isabelizimm
Copy link
Collaborator

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
Copy link
Contributor Author

nathanjmcdougall commented Jul 18, 2024

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:
jdidion/atropos#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
Copy link
Collaborator

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
Copy link
Contributor Author

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

Copy link
Collaborator

@isabelizimm isabelizimm left a comment

Choose a reason for hiding this comment

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

beautiful! 🤩🚀

@isabelizimm isabelizimm merged commit 70380b4 into rstudio:main Jul 19, 2024
25 checks passed
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.

2 participants