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

gh-98040: Remove just the imp module #98573

Merged
merged 17 commits into from
Apr 28, 2023
Merged

gh-98040: Remove just the imp module #98573

merged 17 commits into from
Apr 28, 2023

Conversation

warsaw
Copy link
Member

@warsaw warsaw commented Oct 23, 2022

@ericsnowcurrently - here's a branch that removes just the imp module. It still has failures because of this pip issue.

Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

(pending the pip fix, of course)

@ericsnowcurrently
Copy link
Member

Thanks for splitting this out. It was much easier to review. 😄

Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

This has a merge conflict now.

@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@ericsnowcurrently
Copy link
Member

Sorry about the churn that caused conflicts for you, @warsaw! I don't expect I'll be making any more changes to imp.py or test_imp.py before you merge this.

@ericsnowcurrently
Copy link
Member

@warsaw, FYI, I'm moving the tests I recently added to test_imp over to test_import: gh-102561.

While I was doing that, I noticed there are other tests in test_imp that seem to be more about exercising the import system than the imp module itself, which may make it worth it to keep them. Assuming those ones really aren't imp-specific, do you know if any of them are not already covered by other tests, e.g. in test_import or test_importlib? If so, we should move them out before test_imp is deleted. I'd be glad to do so if it would help (if there are any).

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Apr 11, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@arhadthedev
Copy link
Member

It seems that whoever wanted to merge main into this PR cherry-picked it instead.

The previous head was f45d6a4. I would like to leave the restoration force-push to core developers.

@CAM-Gerlach CAM-Gerlach marked this pull request as ready for review April 11, 2023 20:18
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Apr 11, 2023

FYI, the actual head of this PR branch was 6159e52 . It actually turned out to be fairly straightforward to fix with some rebaseing, i.e.

git fetch upstream
git rebase upstream/main 6159e52ea7000499260d18cee833761811a67fa1

I've (force) pushed the fixed version. (Also accidentally hit the "ready for review button", which I've reversed for now).

@CAM-Gerlach CAM-Gerlach marked this pull request as draft April 11, 2023 20:21
@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Apr 11, 2023

So, grepping a bit, looks like imp is still referred to various potentially undesired places:

  • A few places in the docs, including at least one spot that's causing the docs build to fail, as its resulting in a broken reference in a clean file—I can help fix those
  • The stdlib_module_names.h file, which is causing the check for generated files to fail, which I'm assuming is due to Eric & Co's work and can be fixed by regenerating it via Tools/build/generate_stdlib_module_names.py — it doesn't seem to work on Windows
  • Some comments in the code that still refer to it
  • A couple references in the opcode generation as well
  • Used in the (possibly legacy) importbench script in Tools—should that code or the whole script just be removed, or does it need to be manually converted to importlib? Not sure if its still actually used now that we have a lot of other comprehensive benchmark sources like pyperformance—the script itself mentions this (was last touched 4 years ago).

I'm assuming the test failures due to ensurepip are expected, as referred to above, so I ignored those. I also ignored the references to the very low-level C _imp module used by importlib (and previously imp).

Here's a full list, pruned for false positives and instances that should stay (NEWS, what's new, historical notes, etc):

  • Doc/c-api/import.rst:189: Uses :func:`imp.source_from_cache()` in calculating the source path if
  • Doc/library/functions.rst:1990: module: imp
  • Doc/reference/import.rst:1078: :class:`imp.NullImporter` in the :data:`sys.path_importer_cache`. It
  • Doc/tools/.nitignore:155:Doc/library/imp.rst
  • Lib/pydoc.py:515: (object.__name__ in ('errno', 'exceptions', 'gc', 'imp',
  • Lib/test/test_importlib/util.py:134: if name in ('sys', 'marshal', 'imp'):
  • Modules/_testmultiphase.c:889:/*** Helper for imp test ***/
  • Python/import.c:594: for the first time or via imp.load_dynamic().
  • Python/import.c:596: Here's a summary, using imp.load_dynamic() as the starting point:
  • Python/import.c:598: 1. imp.load_dynamic() -> importlib._bootstrap._load()
  • Python/import.c:3789:"(Extremely) low-level import machinery bits as used by importlib and imp.");
  • Python/makeopcodetargets.py:13: import imp
  • Python/makeopcodetargets.py:20: return imp.load_module(modname, *imp.find_module(modname[modpath]))
  • Python/pylifecycle.c:2172: /* Main is a little special - imp.is_builtin("__main__") will return
  • Python/stdlib_module_names.h:167:"imp",
  • Tools/c-analyzer/TODO:498:Python/import.c:PyImport_ReloadModule():PyId_imp _Py_IDENTIFIER(imp)
  • Tools/importbench/importbench.py:9:import imp
  • Tools/importbench/importbench.py:43: module = imp.new_module(name)
  • Tools/importbench/importbench.py:68: assert not os.path.exists(imp.cache_from_source(mapping[name]))
  • Tools/importbench/importbench.py:83: bytecode_path = imp.cache_from_source(module.__file__)
  • Tools/importbench/importbench.py:111: os.unlink(imp.cache_from_source(mapping[name]))
  • Tools/importbench/importbench.py:113: assert not os.path.exists(imp.cache_from_source(mapping[name]))
  • Tools/importbench/importbench.py:124: os.unlink(imp.cache_from_source(module.__file__))
  • Tools/importbench/importbench.py:144: assert os.path.exists(imp.cache_from_source(mapping[name]))

@CAM-Gerlach
Copy link
Member

Also, not totally sure why, but when I build this commit, I get this delta in the generated files which I cannot reproduce on the main parent commit, adding zipimporter a few places:

diff --git a/Include/internal/pycore_global_objects_fini_generated.h b/Include/internal/pycore_global_objects_fini_generated.h
index 14dfd9ea582..d79b56574f7 100644
--- a/Include/internal/pycore_global_objects_fini_generated.h
+++ b/Include/internal/pycore_global_objects_fini_generated.h
@@ -1232,6 +1232,7 @@ _PyStaticObjects_CheckRefcnt(PyInterpreterState *interp) {
     _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(x));
     _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(year));
     _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(zdict));
+    _PyStaticObject_CheckRefcnt((PyObject *)&_Py_ID(zipimporter));
     _PyStaticObject_CheckRefcnt((PyObject *)&_Py_SINGLETON(strings).ascii[0]);
     _PyStaticObject_CheckRefcnt((PyObject *)&_Py_SINGLETON(strings).ascii[1]);
     _PyStaticObject_CheckRefcnt((PyObject *)&_Py_SINGLETON(strings).ascii[2]);
diff --git a/Include/internal/pycore_global_strings.h b/Include/internal/pycore_global_strings.h
index 6f430bb25eb..77169765294 100644
--- a/Include/internal/pycore_global_strings.h
+++ b/Include/internal/pycore_global_strings.h
@@ -718,6 +718,7 @@ struct _Py_global_strings {
         STRUCT_FOR_ID(x)
         STRUCT_FOR_ID(year)
         STRUCT_FOR_ID(zdict)
+        STRUCT_FOR_ID(zipimporter)
     } identifiers;
     struct {
         PyASCIIObject _ascii;
diff --git a/Include/internal/pycore_runtime_init_generated.h b/Include/internal/pycore_runtime_init_generated.h
index 0452c4c6155..82014a8b10d 100644
--- a/Include/internal/pycore_runtime_init_generated.h
+++ b/Include/internal/pycore_runtime_init_generated.h
@@ -1224,6 +1224,7 @@ extern "C" {
     INIT_ID(x), \
     INIT_ID(year), \
     INIT_ID(zdict), \
+    INIT_ID(zipimporter), \
 }

 #define _Py_str_ascii_INIT { \
diff --git a/Include/internal/pycore_unicodeobject_generated.h b/Include/internal/pycore_unicodeobject_generated.h
index 7114a5416f2..6d4dcc229aa 100644
--- a/Include/internal/pycore_unicodeobject_generated.h
+++ b/Include/internal/pycore_unicodeobject_generated.h
@@ -2007,6 +2007,9 @@ _PyUnicode_InitStaticStrings(PyInterpreterState *interp) {
     string = &_Py_ID(zdict);
     assert(_PyUnicode_CheckConsistency(string, 1));
     _PyUnicode_InternInPlace(interp, &string);
+    string = &_Py_ID(zipimporter);
+    assert(_PyUnicode_CheckConsistency(string, 1));
+    _PyUnicode_InternInPlace(interp, &string);
 }
 /* End auto-generated code */
 #ifdef __cplusplus

@warsaw
Copy link
Member Author

warsaw commented Apr 11, 2023

@warsaw, FYI, I'm moving the tests I recently added to test_imp over to test_import: gh-102561.

While I was doing that, I noticed there are other tests in test_imp that seem to be more about exercising the import system than the imp module itself, which may make it worth it to keep them. Assuming those ones really aren't imp-specific, do you know if any of them are not already covered by other tests, e.g. in test_import or test_importlib? If so, we should move them out before test_imp is deleted. I'd be glad to do so if it would help (if there are any).

I missed this comment earlier but yes, we should audit test_imp.py for general import system tests and move them over. I plan on working on this at Pycon.

@warsaw
Copy link
Member Author

warsaw commented Apr 11, 2023

So, grepping a bit, looks like imp is still referred to various potentially undesired places

Thanks for doing this audit @CAM-Gerlach !

@vstinner
Copy link
Member

The imp module is deprecated for 9 years, since Python 3.4 (2014). Problem: a search for (from imp\b|import imp\b) regex in PyPI top 5,000 projects match 153 projects (260 lines).

That's why I didn't propose a PR to remove it in Python 3.12.

It would be nice for the Python community to collaborate with these projects to reduce the number of impacted projects before removing the imp module.

Example of projects:

  • ansible-7.4.0
  • boto-2.49.0
  • cffi-1.15.1
  • conda-4.3.16
  • django-compat-1.0.15
  • eventlet-0.33.3
  • lxml-4.9.2
  • matplotlib-3.7.1
  • mercurial-6.4.1
  • nose-1.3.7
  • Paste-3.5.2
  • pip-23.0.1
  • pipenv-2023.3.20
  • reportlab-3.6.12
  • setuptools-67.6.1
  • Twisted-22.10.0
  • virtualenv-20.21.0

Full list of projects:

* aiosmtpd-1.4.4.post2 * ansible-7.4.0 * ansible-core-2.14.4 * ansiwrap-0.8.4 * apache-libcloud-3.7.0 * argcomplete-3.0.5 * Arpeggio-2.0.0 * asttokens-2.2.1 * audioread-3.0.0 * auth-0.5.3 * Autologging-1.3.2 * autopep8-2.0.2 * better_exceptions-0.3.3 * boto-2.49.0 * bottle-0.12.25 * cachy-0.3.0 * casadi-3.6.0 * cement-3.0.8 * Cerberus-1.3.4 * cffi-1.15.1 * Cheetah3-3.2.6.post1 * circus-0.18.0 * comtypes-1.1.14 * conda-4.3.16 * conda-pack-0.6.0 * coremltools-6.3.0 * crochet-2.0.0 * Cython-0.29.34 * databricks-cli-0.17.6 * datalab-1.2.1 * dateparser-1.1.8 * dbnd-1.0.12.12 * ddtrace-1.11.2 * debugpy-1.6.7 * diff-match-patch-20200713 * distlib-0.3.6 * distribute-0.7.3 * django-compat-1.0.15 * django_compressor-4.3.1 * django-configurations-2.4.1 * django-filter-23.1 * django-grappelli-3.0.5 * django-heroku-0.3.1 * django-nine-0.2.7 * django-user_agents-0.4.0 * dominate-2.7.0 * dtale-2.14.0 * dvc-2.54.0 * eventlet-0.33.3 * fastrlock-0.8.1 * fire-0.5.0 * flasgger-0.9.5 * Flask-AppBuilder-4.3.1 * flatbuffers-23.3.3 * func_timeout-4.3.5 * future-0.18.3 * gevent-22.10.2 * glom-23.3.0 * google-compute-engine-2.8.13 * grafanalib-0.7.0 * gsutil-5.23 * h2o-3.40.0.3 * hachoir-3.2.0 * hdfs-2.7.0 * hjson-3.1.0 * hyper-0.7.0 * imageio-2.27.0 * IMAPClient-2.3.1 * importlab-0.8 * invoke-2.0.0 * j2cli-0.3.10 * joblib-1.2.0 * Kivy-2.1.0 * logilab-common-1.9.8 * lxml-4.9.2 * markdown2-2.4.8 * matplotlib-3.7.1 * mercurial-6.4.1 * metaflow-2.8.3 * ml_collections-0.1.1 * multiprocessing-2.6.2.1 * mysql-connector-2.2.9 * mysql-connector-python-rf-2.2.2 * newrelic-8.8.0 * nose-1.3.7 * numba-0.56.4 * oletools-0.60.1 * os_sys-2.1.4 * Paste-3.5.2 * pathtools-0.1.2 * pbr-5.11.1 * pdm-2.5.2 * pep562-1.1 * pex-2.1.131 * pip-23.0.1 * pipenv-2023.3.20 * plac-1.3.5 * ply-3.11 * psutil-5.9.4 * ptvsd-4.3.2 * pvlib-0.9.5 * py2neo-2021.2.3 * pycryptodome-3.17 * pycryptodomex-3.17 * pydevd-2.9.5 * pydevd-pycharm-231.8109.197 * pydoop-2.0.0 * pyexcel-0.7.0 * pyhocon-0.3.60 * pyinstaller-5.10.0 * pyLDAvis-3.4.0 * Pympler-1.0.1 * pynose-1.4.2 * pyodbc-4.0.38 * pyopencl-2022.3.1 * pyrepl-0.9.0 * Pyro4-4.82 * pysaml2-7.4.1 * pysmi-0.3.4 * pysnmp-4.4.12 * pytest-openfiles-0.5.0 * pytest-shutil-1.7.0 * python-crfsuite-0.9.9 * python-i18n-0.3.9 * PyXB-1.2.6 * pyxdg-0.28 * raven-6.10.0 * rcssmin-1.1.1 * reportlab-3.6.12 * rjsmin-1.2.1 * SCons-4.5.2 * scrypt-0.8.20 * selinux-0.3.0 * setupmeta-3.4.0 * setuptools-67.6.1 * simplejson-3.19.1 * snapshottest-0.6.0 * social-auth-core-4.4.1 * sox-1.4.1 * sqlalchemy-migrate-0.13.0 * stone-3.3.1 * strip-hints-0.1.10 * sympy-1.11.1 * tecton-0.6.5 * Theano-1.0.5 * Twisted-22.10.0 * twofish-0.3.0 * uwsgi-2.0.21 * virtualenv-20.21.0 * vowpalwabbit-9.8.0 * workflow-2.1.6 * yacs-0.1.8 * Yapsy-1.12.2

@vstinner
Copy link
Member

pip and setuptools:

PYPI-2023-04-13/pip-23.0.1.tar.gz: pip-23.0.1/src/pip/_vendor/distlib/wheel.py: import imp
PYPI-2023-04-13/pip-23.0.1.tar.gz: pip-23.0.1/src/pip/_vendor/pkg_resources/__init__.py: import imp as _imp
PYPI-2023-04-13/setuptools-67.6.1.tar.gz: setuptools-67.6.1/pkg_resources/__init__.py: import imp as _imp

tacaswell added a commit to tacaswell/pythran that referenced this pull request May 1, 2023
The `imp` module has been deprecated from py3.4 [1] and will
be removed in py3.12 [2].

This patch is required to use pythran on the current main branch of Python.

[1] https://docs.python.org/3.11/library/imp.html?highlight=imp#module-imp
[2] python/cpython#98573
tacaswell added a commit to tacaswell/pythran that referenced this pull request May 1, 2023
The `imp` module has been deprecated from py3.4 [1] and will
be removed in py3.12 [2].

This patch is required to use pythran on the current main branch of Python.

[1] https://docs.python.org/3.11/library/imp.html?highlight=imp#module-imp
[2] python/cpython#98573
serge-sans-paille pushed a commit to serge-sans-paille/pythran that referenced this pull request May 2, 2023
The `imp` module has been deprecated from py3.4 [1] and will
be removed in py3.12 [2].

This patch is required to use pythran on the current main branch of Python.

[1] https://docs.python.org/3.11/library/imp.html?highlight=imp#module-imp
[2] python/cpython#98573
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.

None yet

9 participants