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
Another strange Tracebacks with importlib #59630
Comments
Two "easy" ways to get tracebacks which contain useless importlib._bootstrap rows:
$ ./python -c "import foo"
Traceback (most recent call last):
File "<string>", line 1, in <module>
File "<frozen importlib._bootstrap>", line 1421, in _find_and_load
File "<frozen importlib._bootstrap>", line 1388, in _find_and_load_unlocked
File "<frozen importlib._bootstrap>", line 527, in _check_name_wrapper
File "<frozen importlib._bootstrap>", line 899, in load_module
File "<frozen importlib._bootstrap>", line 880, in load_module
File "<frozen importlib._bootstrap>", line 503, in module_for_loader_wrapper
File "<frozen importlib._bootstrap>", line 744, in _load_module
File "<frozen importlib._bootstrap>", line 857, in get_code
File "./foo.py", line 1
invalid syntax
^
SyntaxError: invalid syntax $ ./python
Python 3.3.0b1 (default:afdb0e1a9dac+, Jul 19 2012, 23:55:02)
[GCC 4.4.3] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import _sqlite3
>>> del _sqlite3.register_converter
>>> import sqlite3.dump
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
File "<frozen importlib._bootstrap>", line 1421, in _find_and_load
File "<frozen importlib._bootstrap>", line 1372, in _find_and_load_unlocked
File "/home/amauryfa/python/cpython3.x/Lib/sqlite3/__init__.py", line 23, in <module>
from sqlite3.dbapi2 import *
File "/home/amauryfa/python/cpython3.x/Lib/sqlite3/dbapi2.py", line 83, in <module>
register_adapters_and_converters()
File "/home/amauryfa/python/cpython3.x/Lib/sqlite3/dbapi2.py", line 80, in register_adapters_and_converters
register_converter("date", convert_date)
NameError: global name 'register_converter' is not defined |
IMO we should not ship 3.3 without a fix for this. |
Perhaps this should be split into two issues, as the fix will likely be different for the two. For the first part (SyntaxError), my initial impression is that this will require a similar fix, or extension of it, to the one in bpo-15110. That fix was limited just to the execution of the module, while this problem happens earlier (during the compilation step in SourceLoader.get_code()). For the second part, it is all about failures during the execution of an import that was initiated during the import of another module. That traceback is definitely less than helpful. A better traceback would is doable. For both (and maybe for bpo-15110) we should explore using exception chaining to separate the import portion from the rest. I see about working on a patch later this evening. |
Here's a trivial patch that demonstrates what I mean. Yet, in light of exception chaining, I wonder if we should consider actually making ImportError supercede the inner exception. I'll still try coming up with something similar, but propagates the inside exception. |
Added patch with test cases for the situations Amaury picked up, as well another I noticed (when the from clause refers to a module that fails) |
I actually did start refactoring a lot of this when I was working on the pkgutil changes, but stopped because that patch was already hairy enough. Maybe I should have kept going... |
I think tuning the "what frames to hide" heuristic is a better bet - I remember know that the real reason I stopped messing with which exceptions were thrown was that I ended up confusing pkgutil. |
Fixed test case patch - previous one included a comment from when my test was buggy. |
I like the concept of "exit points" that Antoine introduced with a special method name. Maybe we coule generalize this and have a "execute_and_hide_frames(func, *args, **kwargs)" that is recognized by the caller in import.c. |
OK, to investigate the failures a bit further, I turned off the traceback suppression altogether by sticking an immediate return at the start of remove_importlib_frames. For the three failing tests, this is what I got when adding a traceback.print_tb call: test_broken_from (test.test_import.ImportTracebackTests) ... File "/home/ncoghlan/devel/py3k/Lib/test/test_import.py", line 890, in test_broken_from And here's what I get with my patched version enabled (which also ignores chunks ending with get_code when the failure is a syntax error): test_broken_from (test.test_import.ImportTracebackTests) ... File "/home/ncoghlan/devel/py3k/Lib/test/test_import.py", line 890, in test_broken_from |
I realised that it is my test that was wrong in the syntax error case. Attached patch includes the test cases, and the fix for that case. The weird thing that still requires an explanation is why the chunks in the first two cases are only being partially stripped. Those blocks end with _exec_module, yet a couple of frames are being left in the traceback. Current patch includes my debugging output from those tests. |
Ah, it turns out the three problem children are those which accept an "import_" callback, which is set to builtins.__import__ when accessed via the import statement. This means we end up making a *recursive* call to PyImport_ImportModuleLevelObject, which calls remove_importlib_frames. The inner section of the trace gets stripped away, thus the "_exec_module" frame is already gone by the time the outer frames get added to the traceback. For the moment, I'm just going to adopt the blunt instrument approach and special case those three functions (including them in the "always strip" category). If anyone else comes up with a more elegant mechanism to deal with the recursion, we can switch to it later. |
I'll at least introduce a "_recursive_import" exit point to simplify handling of those three cases. For those, if there isn't another importlib frame on the stack below them, odds are pretty good that it has been stripped already. |
New changeset 75a30a478dc7 by Nick Coghlan in branch 'default': |
I think the implementation I just checked in is close to being as simple as we can get: Blocks ending in _exec_module and _recursive_import are trimmed unconditionally Because the latter two are conditional on the kind of exception thrown, it's only the first two that could be unified and I think keeping the two distinct checks is actually clearer in that case. |
On second thought, I realised having a general purpose "hide this" hook means that any future unconditional cases can easily be hidden just by changing from the normal call to a stripped call. I'll implement that now. |
Seeing as the blocker is dealt with for beta2, see bpo-15486 for the proposed refactoring. |
The unit tests added here are another candidate for the refactoring in bpo-15403 ("Refactor package creation support code into a common location"). I'll note this in that issue. |
For some reason the new tests fail on the XP buildbots. |
For instance, http://buildbot.python.org/all/builders/x86%20XP-4%203.x/builds/7214/steps/test/logs/stdio 4 cases of "ImportError: No module named '_parent_foo'". Failing on "import _parent_foo.bar" and "from _parent_foo import bar". Unfortunately I don't have an XP system set up on which to debug this. |
New changeset 8a0eb395e725 by Nick Coghlan in branch 'default': |
Looks like both XP buildbots are happy now. (one is failing, but not for test_import). |
Indeed, there are still a couple of Windows failures, but they aren't related to this. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: