[Django] #21628: Stop using the `imp` module
Django
------------------------------------------------+------------------------
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Severity: Normal | Keywords:
Triage Stage: Accepted | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
It's deprecated in Python 3.4: http://docs.python.org/3.4/library/imp.
Django uses it in a handful of files.
Notably, it calls `imp.acquire_lock()` and `imp.release_lock()` in
`AppCache.populate()`. These functions don't have a replacement. I'm not
sure how to deal with that, maybe call them only on Python < 3.4?
--
Ticket URL: <https://code.djangoproject.com/ticket/21628>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
We'll most likely have to write some wrappers in django.utils.importlib to
cover all Python versions.
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:1>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic84):
As @aaugustin pointed in a github comment, these functions used to be
implemented as normal reentrant threads and that caused deadlocks
(#18251).
It seems the behavior of those functions changed in 3.3 as well, dunno if
the change affected us:
"Changed in version 3.3: The locking scheme has changed to per-module
locks for the most part. A global import lock is kept for some critical
tasks, such as initializing the per-module locks."
FWIW the python bug where the deprecation was discussed:
http://bugs.python.org/issue17177.
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:2>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by merb):
@aaugustin i think we could just ignore this issue.
Currently I tried:
https://code.djangoproject.com/attachment/ticket/18251/threading_lock.tar.gz
On Python 2.7.5+ and on Python 3.3.2+ the issue never occured. Without
calling {{{imp.acquire_lock()}}} or {{{imp.release_lock()}}}
Maybe the "test" itself is somehow not correct anymore.
But I think its due to the fact that we now (since the last commit
https://github.com/django/django/compare/fe1389e911b0...4a56a93cc458) use
https://pypi.python.org/pypi/importlib a backport of importlib for python
2.7 due to the fact that the next django version won't be compatible to
python 2.6. So just remove imp and we are fine.
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:3>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by merb):
* cc: merb (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:4>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"d674fe6dee16735dd2670243153326806b7e6cb0"]:
{{{
#!CommitTicketReference repository=""
revision="d674fe6dee16735dd2670243153326806b7e6cb0"
Used a regular lock for app registry population.
Since the app registry is always populated before the first request is
processed, the situation described in #18251 for the old app cache
cannot happen any more.
Refs #18251, #21628.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:5>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
The app registry no longer uses the import lock, but there's still 5 uses
of `imp.find_module`, one of `imp.load_module` and one of
`imp.new_module`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:6>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by mjtamlyn):
It's worth noting that `imp.find_module` is the only one of these which is
used in the source code (4 times), the others are only used in tests. Two
of the uses in the main codebase are in a function which needs rewriting
as it's about the old `sys.path` hackery from the old project layout, and
we've already decided we are willing to *maybe* break some stuff there.
There's already a `TODO` to this effect.
The others are in `module_loading` and we will need to write a separate
path for py3+
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:7>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Alex Gaynor <alex.gaynor@…>):
In [changeset:"9d487ae2a16d1efd501e65c78e6885c8cdcd4421"]:
{{{
#!CommitTicketReference repository=""
revision="9d487ae2a16d1efd501e65c78e6885c8cdcd4421"
Refs #21628 -- removed one usage of the imp module in the tests. It is
deprecated in Python 3.4
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:8>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
FWIW, the only remaining usage of `imp` is in
`django.utils.module_loading.module_has_submodule` (+ in the corresponding
test module, `test_module_loading.py`).
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:9>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aymeric Augustin <aymeric.augustin@…>):
In [changeset:"d7f1f316bcb21853597515a25e6e43639b9ed021"]:
{{{
#!CommitTicketReference repository=""
revision="d7f1f316bcb21853597515a25e6e43639b9ed021"
Simplified module_has_submodule on Python >= 3.3.
Stopped using the imp module on Python >= 3.3. Refs #21628.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:10>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Removing usage in the tests is quite difficult. I tried to use the
replacements from importlib but I keep hitting infinite recursions.
In fact it's a bit difficult to figure out what matters in the test. They
were introduced in #13464 but that ticket is short on details.
If I understand Python's deprecation policy correctly, the `imp` module
won't be removed before Python 4. We could simply silence the deprecation
warning.
The alternative would be to run these tests only under Python < 3.3. The
implementation on Python >= 3.3 is drastically simpler and may not require
as many tests.
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:11>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by collinanderson):
I think the one remaining case is in `ProxyFinder` in
`tests/utils_tests/test_module_loading.py`
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:12>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
Another usage popped up:
`db/migrations/loader.py:
six.moves.reload_module(module)`
This uses `imp.reload()` on Python 3. It should use `importlib.reload()`
on Python 3.4+ to avoid a warning.
[https://bitbucket.org/gutworth/six/issue/112/sixmovesreload_module-uses-
deprecated-imp Created ticket] to see if it can be addressed in six.
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:13>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: closed
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* resolution: => fixed
Comment:
Issue in previous comment will be fixed in the next version of six. I
don't think there are any remaining tasks for this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:14>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* status: closed => new
* resolution: fixed =>
Comment:
Oops, `import imp` still exists in
`tests/utils_tests/test_module_loading.py`.
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:15>
Django
Reporter: Aymeric Augustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* has_patch: 0 => 1
Comment:
The test from #15662 only affected a Python 2 code path. With Python 2
support removed in master, it seems okay to remove it.
[https://github.com/django/django/pull/7968 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:16>
Django
Reporter: Aymeric Augustin | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Claude Paroz):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:17>
Django
Reporter: Aymeric Augustin | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by GitHub <noreply@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"9e9e73735e9aa651b5c8ad7a700f90ccb5d3b18b" 9e9e7373]:
{{{
#!CommitTicketReference repository=""
revision="9e9e73735e9aa651b5c8ad7a700f90ccb5d3b18b"
Refs #23919 -- Removed an obsolete test for a Python 2 code path (refs
#15662).
Fixed #21628 by removing the last usage of the imp module.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21628#comment:18>