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

Stop using the removed importlib.resources.path on Python >=3.13 #12402

Closed
wants to merge 1 commit into from

Conversation

rgommers
Copy link
Contributor

Closes gh-12401

I've tested that run_project_tests.py is happy when changing if sys.version_info >= (3, 13): to (3, 9) instead, but decided to only change this for Python 3.13 and up to be conservative and because I saw in the CPython commit log for importlib.resources that as_file leaked a file descriptor until less than a year ago.

@rgommers
Copy link
Contributor Author

rgommers commented Oct 20, 2023

Actually, let me also build CPython from its current HEAD to ensure they didn't remove more. EDIT: done, all good, project tests pass but building numpy with this branch does not, it yields:

AttributeError: module 'importlib.resources' has no attribute 'read_binary'

so a bit more to do here.

Shouldn't this have been caught by some CI check for use of deprecated features? It does warn in Python 3.11:

$ python --version
Python 3.11.6
$ python -c "from importlib.resources import path; path('mesonbuild.scripts', 'python_info.py')"
<string>:1: DeprecationWarning: path is deprecated. Use files() instead. Refer to https://importlib-resources.readthedocs.io/en/latest/using.html#migrating-from-legacy for migration advice.

@eli-schwartz
Copy link
Member

Shouldn't this have been caught by some CI check for use of deprecated features? It does warn in Python 3.11:

$ python --version
Python 3.11.6
$ python -c "from importlib.resources import path; path('mesonbuild.scripts', 'python_info.py')"
<string>:1: DeprecationWarning: path is deprecated. Use files() instead. Refer to https://importlib-resources.readthedocs.io/en/latest/using.html#migrating-from-legacy for migration advice.

See 9aff189

I did in fact add checks for deprecated features. It's not part of CI though, it's something you have to run manually. Use of deprecated features isn't a bug in the code so I don't want to cause CI regressions, and it's the kind of thing that you only need to run once every 18 months (but could run more often).

In this case I actively stomped quite hard on this particular deprecation warning. IIRC at the time I was fairly sure the upstream plan was to have this be "eternally deprecated" especially because there's... zero cost to keeping it around, and I found the rapid deprecation and from scratch rewrites of the entire API contract of this stdlib module to be completely alarming and difficult to build stable programs on top of.

@rgommers
Copy link
Contributor Author

I did in fact add checks for deprecated features. It's not part of CI though, it's something you have to run manually. Use of deprecated features isn't a bug in the code so I don't want to cause CI regressions,

That seems very reasonable, thanks.

IIRC at the time I was fairly sure the upstream plan was to have this be "eternally deprecated" especially because there's... zero cost to keeping it around, and I found the rapid deprecation and from scratch rewrites of the entire API contract of this stdlib module to be completely alarming and difficult to build stable programs on top of.

Yes, I agree. importlib is a particularly problematic module - lots of bugs and churn.

I think I got them all now, at least this gets numpy to build and I can't find more instances of removed functions with grep (the mesonbuild/mesondata.py one doesn't seem to be exercised much by tests).

@eli-schwartz
Copy link
Member

mesondata should be robustly exercised in CI as I'm pretty sure it's integral to the working of cmake dependencies (e.g. llvm). But you wouldn't typically encounter that when working with python extensions.

I'm actually to blame here because really we should have used mesondata for the places where we are currently using importlib.resources, but I was lazy and inconsistent when authoring the relevant code. Possibly I was confused by the deceptively attractive idea of "simply use the stdlib".

Maybe the correct answer is to use our high level wrapper instead. This probably means admitting that we're never planning to get rid of our utility wrapper modules. I guess the answer to that is "oh well, it is what it is".

@@ -112,8 +113,13 @@ def sanity(self) -> bool:
# Sanity check, we expect to have something that at least quacks in tune

import importlib.resources
if sys.version_info >= (3, 13):
Copy link

Choose a reason for hiding this comment

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

Why not >= (3, 9)? Why use a deprecated function when the replacement is available?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It didn't seem to matter, so just trying to make the minimal change here. If it did matter (as it has before with importlib and traversables, it's complex machinery), I'd rather find out through 3.13 early adopters.

It's also possible that something still changes before 3.13.rc1, and this would allow dropping the if-else.

@eli-schwartz
Copy link
Member

The functional API is on track to be added back into 3.13 alpha and un-deprecated, which would mean that no released version of python will have lacked it.

Once it is merged into CPython I propose to close this PR.

@rgommers
Copy link
Contributor Author

Great! Closing this PR when the API is added back in sounds good to me.

@rgommers
Copy link
Contributor Author

rgommers commented Apr 9, 2024

The APIs have been restored in python/cpython#116609, so I'll close this. Thanks @eli-schwartz and @jaraco.

@rgommers rgommers closed this Apr 9, 2024
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.

AttributeError: module 'importlib.resources' has no attribute 'path' on Python 3.13
3 participants