classification
Title: Standardised mechanism for stripping importlib frames from tracebacks
Type: behavior Stage: resolved
Components: Interpreter Core, Library (Lib) Versions: Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: georg.brandl Nosy List: Arfrever, amaury.forgeotdarc, brett.cannon, chris.jerdonek, eric.snow, georg.brandl, ishimoto, jcea, meador.inge, ncoghlan, pitrou, python-dev
Priority: release blocker Keywords: patch

Created on 2012-07-29 10:46 by ncoghlan, last changed 2012-08-03 15:38 by jcea. This issue is now closed.

Files
File name Uploaded Description Edit
issue15486_simplify_importlib_frame_removal_no_new_test.diff ncoghlan, 2012-07-30 11:23 Simplified approach to frame removal review
Messages (11)
msg166745 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-07-29 10:46
Thanks to issue #15425, the number of special cases in the importlib frame stripping is growing. This patch tweaks the frame stripping mechanism to make it easy to selectively strip frames in importlib._bootstrap by creating a _call_with_frames_removed helper function.

The rules in remove_importlib_frames then change to remove any sequence of importlib frames which ends with that helper and all importlib frames in the ImportError case.
msg166748 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-07-29 11:28
In trying to find a new case that wasn't already covered by the test suite, I found an error which I'm not even sure should be an error. Doesn't 3.2 suppress failures that occur when attempting to implicitly cache the .pyc?

Anyway, given the rest of the patch, trimming this traceback will be easy if we decide that's the right thing to do.
msg166760 - (view) Author: Chris Jerdonek (chris.jerdonek) * (Python committer) Date: 2012-07-29 13:44
The test cases uploaded to issue 7559 ("TestLoader.loadTestsFromName swallows import errors") a while back contain a number of distinct cases (four, I think) in which an import error can be raised.  IIRC, these include a couple recursive scenarios (cyclical import from within __init__.py, etc).

Those test cases were designed specifically to cover different scenarios in which unittest's loader was swallowing import errors.  Might be worth taking a look.
msg166770 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-29 14:50
This is an elegant solution, +1 from me.
msg166798 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2012-07-29 17:45
That said, given the nature of the patch (a cleanup without any functional impact), I don't think it should be a release blocker.
msg166846 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-07-30 01:28
The "release blocker" status comes from the test case I added in order to demonstrate the ability to strip a new frame sequence without needing to modify the C code.

Currently that test (failing to write the PYC file) fails with an IsADirectory traceback that includes a lot of importlib frames.

There are two possible solutions:
- redirect the affected call through "_call_with_frames_removed" but otherwise leave the failure intact
- change it so this particular case isn't a fatal error

I need to check if failing to write the .pyc was a fatal error in 3.2 before I decide how to fix it (unless someone else remembers off the top of their head)
msg166875 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-07-30 11:06
OK, after a bit of experimentation, it appears both 3.2 and 3.3 eventually get annoyed if you mess about too much with __pycache__.

1. They're both fine if __pycache__ is entirely unwritable (they just silently skip caching the bytecode)

2. 3.2 throws EOFError if you replace the cache entry with an empty file, 3.3 silently rewrites it with a valid cached version

3. 3.2 throws EOFError if you replace the cache entry with a directory, 3.3 throws a more accurate IsADirectory error

That means my chosen test case is a valid one, and I can just update the offending call in importlib._bootrap to use the new frame stripping hook as I originally planned.
msg166880 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2012-07-30 11:23
Looking at the actual code - I don't think it actually makes sense to strip the frames in this case. Unlike the previous examples, it only comes up if someone is doing something deliberately pathological, at which point a little noise in the traceback is the least of your worries.

Updated patch just drops that test case entirely and only has the changes that simplify the frame removal code. Since Georg isn't on IRC right now, assigning him to decide if he wants this version in beta 2 rather than the current "multiple exit points" approach.
msg166913 - (view) Author: Georg Brandl (georg.brandl) * (Python committer) Date: 2012-07-30 17:06
Looks good and simplifies things.  Green light :)
msg166947 - (view) Author: Eric Snow (eric.snow) * (Python committer) Date: 2012-07-31 02:08
patch LGTM.  Nice and clean.
msg166988 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2012-07-31 11:14
New changeset 62033490ca0f by Nick Coghlan in branch 'default':
Close #15486: Simplify the mechanism used to remove importlib frames from tracebacks when they just introduce irrelevant noise
http://hg.python.org/cpython/rev/62033490ca0f
History
Date User Action Args
2012-08-03 15:38:37jceasetnosy: + jcea
2012-07-31 11:14:32python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg166988

resolution: fixed
stage: patch review -> resolved
2012-07-31 02:22:00ishimotosetnosy: + ishimoto
2012-07-31 02:08:45eric.snowsetmessages: + msg166947
2012-07-30 17:06:00georg.brandlsetmessages: + msg166913
2012-07-30 11:23:56ncoghlansetfiles: - issue15486_simplify_importlib_frame_removal.diff
2012-07-30 11:23:41ncoghlansetfiles: + issue15486_simplify_importlib_frame_removal_no_new_test.diff
assignee: georg.brandl
messages: + msg166880
2012-07-30 11:06:33ncoghlansetmessages: + msg166875
2012-07-30 01:28:12ncoghlansetmessages: + msg166846
2012-07-29 17:45:01pitrousetmessages: + msg166798
2012-07-29 16:35:16meador.ingesetnosy: + meador.inge
2012-07-29 14:50:31pitrousetnosy: + pitrou
messages: + msg166770
2012-07-29 13:44:31chris.jerdoneksetnosy: + chris.jerdonek
messages: + msg166760
2012-07-29 11:54:19Arfreversetnosy: + Arfrever
2012-07-29 11:28:01ncoghlansetfiles: + issue15486_simplify_importlib_frame_removal.diff


keywords: + patch
stage: patch review
title: Standardise the mechanisms for stripping importlib frames -> Standardised mechanism for stripping importlib frames from tracebacks
nosy: + brett.cannon, georg.brandl, amaury.forgeotdarc, eric.snow
versions: + Python 3.3
messages: + msg166748
priority: normal -> release blocker
components: + Interpreter Core, Library (Lib)
type: behavior
2012-07-29 10:46:43ncoghlancreate