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

Fix Python 3.12 compatibility #6965

Merged
merged 1 commit into from
Aug 4, 2023
Merged

Fix Python 3.12 compatibility #6965

merged 1 commit into from
Aug 4, 2023

Conversation

frenzymadness
Copy link
Contributor

.joinpath with an empty argument no longer works as before thanks to this change in CPython: python/cpython@cea910e#diff-2e741d925220d74a9cc04cda1314d3649d9d189c0efc7db18e5387a51225b61c

This change mimics the old internal behavior by using _paths attribute directly.

I'm not saying it's perfect but it's the shortest solution I was able to come up with when building notebook 7rc2 with Python 3.12 in Fedora rawhide.

@github-actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch frenzymadness/notebook/patch-3

@frenzymadness
Copy link
Contributor Author

I've reported the issue in CPython upstream: python/cpython#106614

@jtpio jtpio added the bug label Jul 17, 2023
@jtpio jtpio added this to the 7.0 milestone Jul 17, 2023
@jtpio
Copy link
Member

jtpio commented Jul 19, 2023

Thanks @frenzymadness for looking into this.

Looks like the lint failure might be relevant (Python 3.11 is used in the workflow).

@frenzymadness
Copy link
Contributor Author

I'm not sure what to do here and I'm at EuroPython now so I don't have much time to look into it. The problem is that documentation says that files returns Traversable and it's true that Traversable does not have _path attribute but in this specific case, files returns MultiplexedPath which is a subclass of Traversable and it has that attribute. This is also true in Python 3.11 so it's nothing coming with the new release.

@jtpio jtpio modified the milestones: 7.0, 7.0.x Jul 19, 2023
@jtpio
Copy link
Member

jtpio commented Jul 27, 2023

Since it's just the lint check failing, maybe we can add a comment to ignore the check?

The tests seem to be passing on 3.8 and 3.11.

@jtpio
Copy link
Member

jtpio commented Jul 27, 2023

Also maybe we could start testing on Python 3.12 on CI too? (and add it to the matrix)

@frenzymadness
Copy link
Contributor Author

This is not really a big deal so if you are okay with skipping the check, so I am. I dug a little bit deeper and opened one more issue for importlib.resources where a change in typing might fix the problem here: python/importlib_resources#286

@jtpio
Copy link
Member

jtpio commented Aug 2, 2023

I guess it would be fine yes, also this line is only for the tests.

@frenzymadness
Copy link
Contributor Author

Rebased and added a comment to ignore the typing issue.

@jtpio
Copy link
Member

jtpio commented Aug 3, 2023

Should we also try adding 3.12 to the test matrix on CI?

@frenzymadness
Copy link
Contributor Author

Should we also try adding 3.12 to the test matrix on CI?

Good idea but the configuration is kinda complex so I'm not sure I'm able to do it properly.

.joinpath with an empty argument no longer works as before thanks to this change in CPython: python/cpython@cea910e#diff-2e741d925220d74a9cc04cda1314d3649d9d189c0efc7db18e5387a51225b61c

This change mimics the old internal behavior by using _paths attribute directly.
@jtpio
Copy link
Member

jtpio commented Aug 4, 2023

Sure, I'll have a look in a separate PR: #6999

Copy link
Member

@jtpio jtpio left a comment

Choose a reason for hiding this comment

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

Thanks!

@jtpio jtpio mentioned this pull request Aug 4, 2023
@jtpio jtpio merged commit 0188ad8 into jupyter:main Aug 4, 2023
22 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants