Repository navigation
The new import system makes it inconvenient to correctly issue a deprecation warning for a module #68493
Description
| BPO | 24305 |
|---|---|
| Nosy | @warsaw, @brettcannon, @ncoghlan, @larryhastings, @jwilk, @njsmith, @ericsnowcurrently, @takluyver, @berkerpeksag, @zware, @serhiy-storchaka, @MojoVampire |
| 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 2015-09-06.07:51:11.529>
created_at = <Date 2015-05-27.23:36:49.752>
labels = ['type-bug', 'library', 'release-blocker']
title = 'The new import system makes it inconvenient to correctly issue a deprecation warning for a module'
updated_at = <Date 2015-09-07.05:38:03.473>
user = 'https://github.com/njsmith'bugs.python.org fields:
activity = <Date 2015-09-07.05:38:03.473>
actor = 'python-dev'
assignee = 'brett.cannon'
closed = True
closed_date = <Date 2015-09-06.07:51:11.529>
closer = 'larry'
components = ['Library (Lib)']
creation = <Date 2015-05-27.23:36:49.752>
creator = 'njs'
dependencies = []
files = ['40187', '40199', '40211', '40233', '40286', '40287']
hgrepos = []
issue_num = 24305
keywords = ['patch', '3.2regression']
message_count = 40.0
messages = ['244225', '244232', '244247', '244259', '244302', '244310', '244390', '244448', '248602', '248603', '248605', '248609', '248617', '248635', '248670', '248750', '248814', '248918', '248937', '249011', '249088', '249089', '249090', '249091', '249092', '249151', '249306', '249622', '249666', '249687', '249907', '249978', '249980', '249981', '249984', '249985', '249986', '249987', '249988', '250069']
nosy_count = 14.0
nosy_names = ['barry', 'brett.cannon', 'ncoghlan', 'larry', 'jwilk', 'Arfrever', 'njs', 'python-dev', 'eric.snow', 'takluyver', 'berker.peksag', 'zach.ware', 'serhiy.storchaka', 'josh.r']
pr_nums = []
priority = 'release blocker'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue24305'
versions = ['Python 3.5', 'Python 3.6']
Activity
njsmith commentedon May 27, 2015
(Noticed while fixing the IPython equivalent of bpo-24294)
The obvious way to deprecate a module is to issue a DeprecationWarning inside the main body of the module, i.e.
But this is problematic, because the resulting message will claim that the problem is in thirdpartymodule.py, not in mymodule.py. And this is especially bad if I am doing things correctly (!) and using a warnings filter that enables display of DeprecationWarnings for mymodule, but not for third-party modules. (This need for correct attribution comes up in the interactive use case cited above, but I actually have packages where the CI infrastructure requires the elimination of DeprecationWarnings triggered by my own code -- for this to work it's crucial that warnings be attributed correctly.)
So the obvious fix is to instead write:
which says "the code that needs fixing is the code that called me".
On Python 2.7, this works, because all the code that executes in between 'import thirdpartymodule' and the call to 'warnings.warn' is C code, so it doesn't create any intermediate stack frames.
On more recent versions of Python, the import system itself is written in Python, so this doesn't work at all.
On Python 3.3, the correct way to deprecate a module is:
and on Python 3.4, the correct way to deprecate a module is:
(See ipython/ipython#8480 (comment) for test code.)
Obviously this is not desireable.
I'm not sure what best solution is. Maybe there should be some collaboration between the import code and the warnings module, so that when the warnings module walks the stack, it skips over stack frames that come from inside the guts of import system?
ncoghlan commentedon May 28, 2015
Somewhat related issues are bpo-16217 (traceback noise when running under python -m) and bpo-23773 (traceback noise when running under custom import hooks)
For 3.4 and 3.5, we may want to consider a brute force solution where the warnings module just straight up ignores _frozen_importlib frames when counting stack levels.
For 3.6+, I've occasionally pondered various ideas for introducing the notion of a "frame debugging level". The most recent variant of that idea would be to have all frames live in level zero by default, but there'd be a way to flag a module at compile time to set it higher for the code objects created by that module.
Displaying those frames in tracebacks would then be tied to both the current frame's debugging level and the interpreter's verbosity level (whichever was higher), while other stack related operations (like the warnings stacklevel and frame retrieval) would only consider frames at the current frame's debugging level and below.
berkerpeksag commentedon May 28, 2015
This looks like a duplicate of bpo-23810 (there is a patch for stdlib usages, but it probably can be changed to a more general solution).
ncoghlan commentedon May 28, 2015
I've made this depend on bpo-23810, rather than duplicating it.
That way, bpo-23810 can cover fixing the stdlib deprecation warnings, while this can cover making it a public API.
brettcannon commentedon May 28, 2015
My personal plan was to get issue bpo-23810 finished, make sure it worked, and then expose a public API for declaring module deprecations which used the private API under the hood. I'm hoping to get bpo-23810 done this Friday and then we can talk about how we may want to expose a function in the warnings module for deprecating modules.
serhiy-storchaka commentedon May 28, 2015
It would be better to skip _frozen_importlib frames automatically instead of forcing end users to use special API.
brettcannon commentedon May 29, 2015
Skipping select frames is a shift in semantics for warnings.warn() (so can't go into 3.5b1), doing it implicitly might be costly on interpreters where getting a frame is expensive, and coming up with a new API allows for a nicer design, e.g.
warnings.deprecate_module(__name__, 'it will be removed in Python 3.6')-> "the formatter module is deprecated; it will be removed in Python 3.6"njsmith commentedon May 30, 2015
A nice new API is certainly a nice thing, but I feel that changing the stack walking code should be considered as fixing a regression introduced in 3.3. Indeed, searching github for code that actually uses the stacklevel= argument:
I observe that:
there isn't a single usage on the first ten pages with stacklevel > 3, so stack walking speed is unlikely to be an issue -- esp. since it will only be slow in the cases where there are spurious import frames on the stack, i.e., you can only make it faster by making the results incorrect, and
in the first ten pages I counted 14 distinct pieces of code (GH search is chock full of duplicates, sigh), and *11* of them are specifically module deprecations that are correctly passing stacklevel=2, and thus working on 2.7 but broken on 3.3+, and
I counted zero examples where someone wrote
which is the only situation where even backporting a fix for this would break code that previously worked correctly.
Basically the overwhelming majority of uses of the stacklevel= argument are broken in 3.3 and 3.4 and 3.5-beta. We should fix it.
brettcannon commentedon Aug 14, 2015
I have merged over issue bpo-23810 because they are aiming to solve the same problem and the conversation is split too much.
Just thinking out loud, this situation is compounded by the fact that importlib itself has some warnings and so automatically stripping out stack frames might make those warnings look odd.
Still not sure if Larry considers this a release blocker for 3.5.0. And any solution will need to be solved in warnings.py and _warnings.c.
Some days I really hate our warnings system.
serhiy-storchaka commentedon Aug 14, 2015
Frames can be skipped only for warnings issued by imported module code, not for warnings issued by import machinery itself.
I propose following algorithm:
Before executing module code push the current frame and the number of frames to skip in global stack in the warnings module, and pop it after execution (public context manager can help). In warnings.warn() skip stacklevel frames and if saved frame was skipped, skip corresponding additional number of frames. Repeat for all saved frames.
brettcannon commentedon Aug 14, 2015
Will that be thread-safe? Plus storing that value in a way that _warnings and warnings can read will require something like a single-item list that can be mutated in-place.
The other option is to teach warnings to skip anything coming from importlib/_bootstrap* unless stacklevel==1 and that frame happens to be in the importlib code.
njsmith commentedon Aug 14, 2015
For 3.4/3.5 purposes, I propose a simpler algorithm: first, define a function which takes a module name and returns true if it is part of the internal warning machinery and false otherwise. This is easy because we know what import machinery we ship.
Then, to walk the stack in warnings.py, do something like:
This produces reasonable behavior for warnings issued by both regular user code and by warnings issued from inside the warning machinery, and it avoids having to explicitly keep track of call depths.
Then we can worry about coming up with an all-singing all-dancing generalized version for 3.6.
29 remaining items
larryhastings commentedon Sep 3, 2015
That *is* easier, thanks. Though the UI for that is baffling. Protip: search for the section where all the "custom" builders are listed all in one section, three-quarters of the way down the page.
larryhastings commentedon Sep 5, 2015
Okay. Right now creating server-side clones is broken. So I have repurposed one of my existing (old, dead) server-side clones for testing this. It's called "ssh://[email protected]/larry/path_error2".
I just fired off this change on all the "custom" buildbots. I'm going to bed. If it passed everything in the morning I'll just check it in to 3.5.0, and hail mary.
larryhastings commentedon Sep 6, 2015
It was basically okay on the buildbots--no worse than cpython would have been before the checkin. (A lot of the buildbots are... wonky.)
I checked it in to cpython350 directly. I'll do the forward merge after 3.5.0rc3 goes out the door.
njsmith commentedon Sep 6, 2015
Hooray! Thanks Larry.
Would it make sense to do a 3.4.x backport, or is that closed now with 3.5 being imminent?
larryhastings commentedon Sep 6, 2015
I don't think it'd be appropriate to backport to 3.4--that ship has sailed. 3.4 requires a stacklevel=8 and that's that.
If we backported it and it shipped in 3.4.4, "correct" code would have to use a stacklevel=8 for 3.4.0 through 3.4.3, and stacklevel=2 for 3.4.4 and above. Ugh!
njsmith commentedon Sep 6, 2015
Some limited code search statistics posted upthread (msg244448) suggest that ~100% of real-world code is using stacklevel=2 unconditionally and thus getting incorrect results on 3.3 and 3.4.
larryhastings commentedon Sep 6, 2015
Yes, I saw that. That doesn't mean we should change the interface they are using (incorrectly) eighteen months after it shipped. We take backwards-compatibility pretty seriously here in the Python world, bugs and all.
njsmith commentedon Sep 6, 2015
Also just checked searchcode.com, which allows more fine-grained queries, and it reports ~6500 hits for "stacklevel=2" and exactly 0 for "stacklevel=8".
<refreshes, sees new post>
Huh. So the official word is that requiring stacklevel=8 on 3.4 is not a bug, rather all those modules are buggy? I've been telling people who asked to just leave the stacklevel=2 thing alone rather than adding version-specific conditionals, but I guess I can pass that on.
larryhastings commentedon Sep 6, 2015
Unless I'm overruled, yes.
njsmith commentedon Sep 6, 2015
ok
python-dev commentedon Sep 7, 2015
New changeset 94966dfd3bd3 by Larry Hastings in branch '3.5':
Issue bpo-24305: Prevent import subsystem stack frames from being counted
https://hg.python.org/cpython/rev/94966dfd3bd3 aka 714e493 in git
Calculate correct stack level for deprecation warnings to show them t…
Calculate correct stack level for deprecation warnings to show them t…