Opened 11 years ago

Closed 8 years ago

#21628 closed Cleanup/optimization (fixed)

Stop using the `imp` module

Reported by: Aymeric Augustin Owned by: nobody
Component: Core (Other) Version: dev
Severity: Normal Keywords:
Cc: merb Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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?

Change History (18)

comment:1 by Aymeric Augustin, 11 years ago

We'll most likely have to write some wrappers in django.utils.importlib to cover all Python versions.

comment:2 by loic84, 11 years ago

As @aaugustin pointed out 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.

Version 1, edited 11 years ago by loic84 (previous) (next) (diff)

comment:3 by merb, 11 years ago

@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. (btw. imp.acquire_lock() does nothing when you are working on master)

Last edited 11 years ago by merb (previous) (diff)

comment:4 by merb, 11 years ago

Cc: merb added

comment:5 by Aymeric Augustin <aymeric.augustin@…>, 11 years ago

In 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.

comment:6 by Aymeric Augustin, 11 years ago

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.

comment:7 by Marc Tamlyn, 11 years ago

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+

comment:8 by Alex Gaynor <alex.gaynor@…>, 11 years ago

In 9d487ae2a16d1efd501e65c78e6885c8cdcd4421:

Refs #21628 -- removed one usage of the imp module in the tests. It is deprecated in Python 3.4

comment:9 by Claude Paroz, 10 years ago

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).

comment:10 by Aymeric Augustin <aymeric.augustin@…>, 10 years ago

In d7f1f316bcb21853597515a25e6e43639b9ed021:

Simplified module_has_submodule on Python >= 3.3.

Stopped using the imp module on Python >= 3.3. Refs #21628.

comment:11 by Aymeric Augustin, 10 years ago

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.

comment:12 by Collin Anderson, 10 years ago

I think the one remaining case is in ProxyFinder in tests/utils_tests/test_module_loading.py

comment:13 by Tim Graham, 10 years ago

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. Created ticket to see if it can be addressed in six.

comment:14 by Tim Graham, 9 years ago

Resolution: fixed
Status: newclosed

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.

comment:15 by Tim Graham, 9 years ago

Resolution: fixed
Status: closednew

Oops, import imp still exists in tests/utils_tests/test_module_loading.py.

comment:16 by Tim Graham, 8 years ago

Has patch: set

The test from #15662 only affected a Python 2 code path. With Python 2 support removed in master, it seems okay to remove it. PR

comment:17 by Claude Paroz, 8 years ago

Triage Stage: AcceptedReady for checkin

comment:18 by GitHub <noreply@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 9e9e7373:

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.

Note: See TracTickets for help on using tickets.
Back to Top