Repository navigation
Avoid two importlib copies #58862
Description
| BPO | 14657 |
|---|---|
| Nosy | @malemburg, @brettcannon, @ncoghlan, @pitrou, @ericvsmith, @merwok, @ericsnowcurrently |
| 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 = None
closed_at = <Date 2012-06-17.20:40:03.020>
created_at = <Date 2012-04-23.22:34:03.572>
labels = ['interpreter-core', 'type-bug', 'library']
title = 'Avoid two importlib copies'
updated_at = <Date 2012-06-25.00:40:03.452>
user = 'https://github.com/pitrou'bugs.python.org fields:
activity = <Date 2012-06-25.00:40:03.452>
actor = 'ncoghlan'
assignee = 'none'
closed = True
closed_date = <Date 2012-06-17.20:40:03.020>
closer = 'pitrou'
components = ['Interpreter Core', 'Library (Lib)']
creation = <Date 2012-04-23.22:34:03.572>
creator = 'pitrou'
dependencies = []
files = ['25329', '25330', '25331', '25364', '25403', '25479']
hgrepos = []
issue_num = 14657
keywords = ['patch']
message_count = 75.0
messages = ['159096', '159098', '159100', '159104', '159109', '159116', '159123', '159127', '159128', '159154', '159158', '159173', '159186', '159196', '159198', '159202', '159203', '159204', '159209', '159211', '159214', '159215', '159217', '159218', '159224', '159225', '159247', '159264', '159268', '159269', '159270', '159272', '159273', '159274', '159275', '159292', '159296', '159299', '159302', '159304', '159307', '159308', '159310', '159311', '159316', '159318', '159328', '159348', '159585', '159961', '160053', '160058', '160059', '160060', '160061', '160088', '160091', '160093', '160104', '160120', '160147', '160148', '160149', '160150', '160153', '160154', '160162', '160974', '162984', '163002', '163010', '163083', '163084', '163804', '163878']
nosy_count = 9.0
nosy_names = ['lemburg', 'brett.cannon', 'ncoghlan', 'pitrou', 'eric.smith', 'eric.araujo', 'Arfrever', 'python-dev', 'eric.snow']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue14657'
versions = ['Python 3.3']
Activity
pitrou commentedon Apr 23, 2012
This patch avoids creating a second copy of importlib._bootstrap when a first one exists as _frozen_importlib.
This isn't perfect as it mutates the module when importlib is imported for the first time, but I think it's better than the status quo.
Also, importlib itself could be imported somewhere along the startup phase, so that all this is invisible to the user.
I'm not sure how to test this, since _frozen_importlib is an implementation detail, and changing that module's name would probably defeat the test already.
pitrou commentedon Apr 23, 2012
New patch with tests.
pitrou commentedon Apr 23, 2012
New patch also avoids calling _setup() a second time (which can be annoying since _setup() has a list.append() call somewhere).
brettcannon commentedon Apr 24, 2012
So why the mutation? Are you that worried someone is going to import importlib._bootstrap directly?
This also costs in development complexity because not only do you have to run 'make' to get changes to be testable, but it also leads to difficult debugging situations where if you are not totally sure you got something working you won't find out until you see e.g. that the standard I/O streams were not initialized.
If you really feel the need to hide _frozen_importlib then it would be better to do the minimum required to get import up and running (should be once the encodings are up in Py_Initialize) and then pull in importlib._bootstrap and have that clear out what _frozen_importlib set like __import__(), sys.path_importer_cache(), and eventually sys.meta_path and sys.path_hooks (I wouldn't touch sys.modules, though, thanks to built-ins and extensions not liking to be reloaded).
brettcannon commentedon Apr 24, 2012
I should also mention that all of this becomes much less important once issue bpo-14605 is finished because at that point sys.meta_path and sys.path_hooks will have _frozen_importlib objects and that will be what importlib works off of directly. But I still understand the desire to eliminate _frozen_importlib from being exposed, it's just a matter of coming up with a reasonable solution.
ncoghlan commentedon Apr 24, 2012
My preference would also be for _frozen_importlib._bootstrap to overwrite as much evidence of itself as it can with the "real" one.
This would also mean that changes to importlib._bootstrap would actually take effect for user code almost immediately, *without* rebuilding Python, as the frozen version would *only* be used to get hold of the pure Python version.
pitrou commentedon Apr 24, 2012
Well, importing importlib *does* import importlib._bootstrap, and
creates another copy of the module. importlib.__import__ is then wired
to _bootstrap.__import__, which is different from the built-in
__import__ (potentially using different globals, for example).
I'm worried that two different copies of importlib will lead to its own
difficult debugging situations.
pitrou commentedon Apr 24, 2012
Actually, _io, encodings and friends must be loaded before importlib
gets imported from Python code, so you will still have __loader__
entries referencing the frozen importlib, unless you also rewrite these
attributes.
My desire here is not to hide _frozen_importlib, rather to avoid subtle
issues with two instances of a module living in memory with separate
global states. Whether it's the frozen version or the on-disk Python
version that gets the preference is another question (a less important
one in my mind).
malemburg commentedon Apr 24, 2012
Antoine Pitrou wrote:
Why don't you freeze the whole importlib package to avoid all these
issues ? As side effect, it will also load a little faster.
brettcannon commentedon Apr 24, 2012
To start, I'm *not* going to make the final call on this issue's solution. I'm inches away from importlib burnout and general integration frustration with trying to clean up the implicit behaviour. So to prevent me from making a bad decision I will you guys make the final call.
Anyway, I see two options here. One is the "let _frozen_importlib be *the* implementation, period" argument put forth by Antoine (MAL's "freeze everything" also falls under this since it suffers from the same issues). This is the easiest solution for the issue of not having overlapping implementations and cause potential mix-ups, etc. The issue becomes development difficulty goes up as now you are adding a compile step where if you screw up you can get really bad error messages (e.g. "standard streams could not be created" kind of stuff). This could theoretically be overcome if the importlib tests all used a manually created module directly from the source code to verify things before rebuilding (as well as making sure sys.path_importer_cache was cleaned out). With a restructuring of importlib's tests to use a common TestCase with the proper setUp()/teardown() for keeping things clean along with class and module fixtures to prevent obscene stuff like re-importing for every test method. Another option is we hide the source as _importlib or something to allow direct importation w/o any tricks under a protected name.
Then there is Nick's proposal of using _frozen_importlib to start up and then swap out with a new version created from the source during startup. This keeps development simple since the tests run against the code *almost* all other code will use and thus eliminate the test. The problem here is that startup is a smidgen slower and it requires you blacklist what needs to get swapped out and if you mess up that will be tough to debug as well.
Both get the same outcome but with different approaches, it's just a question of which one is easiest to maintain.
pitrou commentedon Apr 24, 2012
Le mardi 24 avril 2012 à 15:10 +0000, Brett Cannon a écrit :
I don't have any strong preference. Nick's proposal sounds slightly
better but Nick hasn't uploaded a patch yet :-)
malemburg commentedon Apr 24, 2012
test me
thod. Another option is we hide the source as _importlib or something to allow direct importation w/o any tricks under a protected name.
Using the freeze everything approach you make things easier for the
implementation, since you don't have to think about whether certain
pieces of code are already available or not.
For development, you can also have the package load bytecode
or source from an external package instead of running (all of)
the module's bytecode that was compiled into the binary.
This is fairly easy to do, since the needed exec() does not
depend on the import machinery.
The only downside is big if statement to isolate the frozen
version from the loaded one - would be great if we had a
command to stop module execution or code execution for a block to
make that more elegant, e.g. "break" at module scope :-)
50 remaining items
pitrou commentedon May 7, 2012
(not to mention the speedups in import.c)
ericsnowcurrently commentedon May 7, 2012
I'm +1 on Nick's recommendation.
@antoine
+1
Does this mean that the whole test suite should be run under both (whenever _bootstrap.py is modified)? Would that warrant a new flag for the test suite or even an automated check?
That's what I was getting at with this:
Arfrever commentedon May 7, 2012
It might be sufficient to only run tests from the following files with both importlib._bootstrap and _frozen_importlib:
test_imp.py
test_import.py
test_importhooks.py
test_importlib.py
test_pkgimport.py
test_threaded_import.py
test_zipimport.py
test_zipimport_support.py
pitrou commentedon May 7, 2012
I was only thinking about test_importlib myself.
brettcannon commentedon May 7, 2012
I would say test_importlib and test_imp (test_import really should just get folded into test_importlib).
brettcannon commentedon May 17, 2012
Realized that any decorator or context manager that is created for swapping _frozen_importlib/importlib._bootstrap should also verify no module is left in sys.modules with a bad loader and that sys.path_importer_cache doesn't have a bad finder (I would say that this would go into test.support.regrtest's state checks, but that seems overkill for only two tests).
And this might be worth doing as a decorator (method or class) to make it easier to make sure the requisite tests always run with both versions (or copying what test_warnings does). I don't want to do anything in a module's test_main() as that precludes using unittest's test discovery for running tests.
pitrou commentedon Jun 16, 2012
Unless someone plans to do further work on this, I'd like to commit unique_importlib3.patch. A working solution is better than nothing.
ericsnowcurrently commentedon Jun 17, 2012
It's actually at the top of my list, but may still be a week or two until I can get to it.
ncoghlan commentedon Jun 17, 2012
+1 Antoine - your patch is better than the status quo, so it makes sense to apply it.
If Eric can make the other way work in time for 3.3, great, but at least we'll have a solid fallback position without it.
python-dev commentedon Jun 17, 2012
New changeset e3a984076837 by Antoine Pitrou in branch 'default':
Issue bpo-14657: The frozen instance of importlib used for bootstrap is now also the module imported as importlib._bootstrap.
http://hg.python.org/cpython/rev/e3a984076837
pitrou commentedon Jun 17, 2012
Ok, I've committed the patch. I'm closing this issue, but of course potential improvements can be posted under a new issue.
brettcannon commentedon Jun 24, 2012
Do people feel the need to change the importlib tests to run against both the frozen code and importlib/_bootstrap.py? If so either the test_warnings approach (or a new one using class decorators or something) should be used to run the tests twice.
ncoghlan commentedon Jun 25, 2012
Given Antoine's other change to the build process to fix the bootstrapping problem, I'm OK with leaving it up to anyone hacking on _bootstrap.py to remember that they need to run make if they want the interpreter to see any of their changes.
Unlike the C accelerator cases, it's not possible for the two implementations to get out of sync on the buildbots, so running the tests twice would just be a convenience change for anyone hacking on _bootstrap.py rather than being needed for correctness.
That said, if someone created a new issue and posted a patch to run the tests twice, I wouldn't object - a classic case of +0 :)