Repository navigation
Replace __import__ w/ importlib.__import__ #46630
Description
| BPO | 2377 |
|---|---|
| Nosy | @brettcannon, @ncoghlan, @pitrou, @vstinner, @ericvsmith, @benjaminp, @serwy, @merwok, @alex, @Trundle, @ericsnowcurrently |
| Dependencies | |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/brettcannon'
closed_at = <Date 2012-05-04.17:44:03.273>
created_at = <Date 2008-03-18.02:40:12.363>
labels = ['interpreter-core', 'type-bug', 'release-blocker']
title = 'Replace __import__ w/ importlib.__import__'
updated_at = <Date 2012-05-04.17:44:03.272>
user = 'https://github.com/brettcannon'bugs.python.org fields:
activity = <Date 2012-05-04.17:44:03.272>
actor = 'brett.cannon'
assignee = 'brett.cannon'
closed = True
closed_date = <Date 2012-05-04.17:44:03.273>
closer = 'brett.cannon'
components = ['Interpreter Core']
creation = <Date 2008-03-18.02:40:12.363>
creator = 'brett.cannon'
dependencies = ['14605', '1559549']
files = ['24447', '24632', '24700', '25104', '25218']
hgrepos = ['34']
issue_num = 2377
keywords = ['patch']
message_count = 58.0
messages = ['63851', '139233', '147625', '149518', '152631', '152785', '152788', '152814', '152822', '153510', '153511', '153957', '153999', '154022', '154070', '154072', '154103', '154106', '154143', '154145', '154149', '154152', '154157', '154179', '154716', '154717', '154912', '154915', '154917', '155108', '155268', '155269', '155481', '155485', '155852', '155895', '155963', '156049', '156099', '157392', '157675', '157683', '157742', '158001', '158267', '158268', '158270', '158271', '158297', '158298', '158300', '158301', '158302', '158304', '158305', '158307', '158573', '158967']
nosy_count = 13.0
nosy_names = ['brett.cannon', 'ncoghlan', 'pitrou', 'vstinner', 'eric.smith', 'benjamin.peterson', 'roger.serwy', 'eric.araujo', 'Arfrever', 'alex', 'Trundle', 'python-dev', 'eric.snow']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue2377'
versions = ['Python 3.3']
Activity
brettcannon commentedon Mar 18, 2008
Python/import.c should be replaced by the implementation under
development contained in the py3k-importlib branch.
[-]Replace import.c with a pure Python implementation[/-][+]Replace __import__ w/ importlib.__import__[/+]brettcannon commentedon Jun 27, 2011
OK, so first step is to simply replace __import__ w/ importlib.__import__ using builtins.__import__ in order to make sure that all tests pass as expected. Can probably do this by doing the switch in Py_Initialize() somewhere.
Next step after that will be seeing if _io can be used by importlib w/ the import of os postponed until after importlib is bootstrapped.
brettcannon commentedon Nov 14, 2011
Just some notes on the branch:
The FAILING file contains tests known to fail. There is a comment explaining the failure and possible fixes. If something makes it here and stays for a while it probably means upstream changes are needed (eg. test_pydoc needs ImportError to grow an attribute as to what import failed).
Python/pythonrun.c contains XXX markers of what is left to be done (although currently the zipimport comment is out-of-date and there needs to be one for exposing some APIs created in importlib.__init__). If you are looking for something to do, just search for XXX and try to tackle one of the comments.
ericsnowcurrently commentedon Dec 15, 2011
AFAICT, bpo-1559549 is the ImportError attribute ticket.
brettcannon commentedon Feb 4, 2012
OK, I'm down to a single bug to be solved to call this work a "success" (there are other test failures, but they are not overtly difficult to solve).
At this point the bootstrapping is failing in the face of sub-interpreters. Specifically, when I try to load the frozen importlib code in Py_NewInterpreter() it leads to an assertion failure in the GC code when handling references in a GC generation. I have zero experience with sub-interpreters and my GC experience is rusty, so if anyone can have a look at the code I would appreciate it to see if they can figure out why loading a frozen module (while in marshal) is leading to a GC assertion error.
brettcannon commentedon Feb 6, 2012
Thanks to Benjamin, the test_capi assert failure is fixed. At this point the only failures are listed in the FAILING file and are (probably) minor.
vstinner commentedon Feb 6, 2012
Is there a benchmark for import? How slow is importlib? :)
brettcannon commentedon Feb 7, 2012
On Mon, Feb 6, 2012 at 17:06, STINNER Victor <report@bugs.python.org> wrote:
importlib.test.benchmark
vstinner commentedon Feb 8, 2012
bench_startup.py: short script to compute the best startup time (I wrote the original script for the hash collision issue, bpo-13703). Result on my PC:
So importlib adds an overhead of 25.7% in startup time.
brettcannon commentedon Feb 16, 2012
Just a quick update. I have refactored importlib in the cpython repo to allow for implementing bits of importlib.__import__() and importlib._gcd_import() in C. This means that the built-in __import__() is now calling importlib underneath the covers. Eventually what is in importlib.__import__() and _gcd_import() directly will be translated into the equivalent C code. This will speed up accessing sys.modules. After that whatever is deemed on the critical path and worth rewriting in C can be done function by function. That, though, should wait for Python-level profiling and optimization before wasting one's time.
brettcannon commentedon Feb 16, 2012
Add Nick since the refactoring of importlib.__import__() into functions was his idea.
35 remaining items
brettcannon commentedon Apr 14, 2012
Notes on what to mention:
brettcannon commentedon Apr 14, 2012
More notes:
5% startup loss according to normal_startup; within realm of compiler optimizations.
serwy commentedon Apr 14, 2012
Brett, your latest commit breaks IDLE. Here's the error message:
Same error occurs for ZoomHeight, ScriptBinding, CallTips, ParenMatch, and AutoComplete.
I reverted to e730da0cd489, recompiled, and these errors went away. Just to be sure, I updated to tip, recompiled, and the errors reappeared.
brettcannon commentedon Apr 14, 2012
Another thing to note:
index does not default to -1 anymore but to 0; bug that should have gone away in Python 2.7.
brettcannon commentedon Apr 14, 2012
So IDLE broke because it was relying on buggy behaviour accidentally left in Python 2.7 and carried forward (plus it was not updated to use best practices like importlib.import_module()). Roger, can you try the patch I have uploaded and see if that fixes things?
serwy commentedon Apr 14, 2012
I tested update_idle.diff and it corrects the issue.
While IDLE's use of __import__ may be "buggy", I don't see anything in the documentation about deprecation or other warnings about this usage. This is a backwards-incompatible change.
serwy commentedon Apr 15, 2012
I caused a segmentation fault with the following (on Linux):
brettcannon commentedon Apr 15, 2012
I committed the fix. Thanks for testing, Roger.
As for the change in semantics, I'm fully aware it is not backwards-compatible. Unfortunately the incorrect usage was not even discovered until I started my bootstrap work because the import statement does the right thing but __import__() itself was never updated so it is only noticeable if you never updated your code to use importlib.import_module() which has been the preferred way to programmatically import code since Python 2.7/3.1. Plus the correct semantics are documented in PEP-328 (http://python.org/dev/peps/pep-0328/) and referenced in the language spec (http://docs.python.org/py3k/reference/simple_stmts.html#the-import-statement) so I'm not going to change it back since this brings the function more in line with expectations. And since the fix is as simple as a try/except and two import calls then it falls within the realm of having to fix code for any new Python release.
And as for the crash, I will have a look.
serwy commentedon Apr 15, 2012
Brett, I see your point.
The docs for __import__ should be updated to include your two-import fix as well as reference PEP-328.
http://docs.python.org/dev/library/functions.html?highlight=__import__#__import__
brettcannon commentedon Apr 15, 2012
OK, crasher is fixed (as is importlib as it failed on the test case as well thanks to the slicing of [:-0] returning the empty string instead of the entire string).
And I will update the docs to be a bit more clear about things (at least those docs have the right default value for level).
brettcannon commentedon Apr 17, 2012
Note:
__import__('sys', level=1) no longer works; raises KeyError instead.
brettcannon commentedon Apr 22, 2012
To note:
_frozen_importlib is a CPython implementation detail