The new import system makes it inconvenient to correctly issue a deprecation warning for a module

[njsmith]

BPO	24305
Nosy	@warsaw, @brettcannon, @ncoghlan, @larryhastings, @jwilk, @njsmith, @ericsnowcurrently, @takluyver, @berkerpeksag, @zware, @serhiy-storchaka, @MojoVampire
Files	issue24305.diff: Skip internal CPython implementation frames issue24305.diff issue24305.diff: Hybrid Nathaniel/Brett approach issue24305.diff: Subtly buggy C implementation issue24305.diff issue24305.diff: Passes all tests, Python and C

^{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']

[njsmith]

(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.

# thirdpartymodule.py
import warnings
warnings.warn("{} is deprecated".format(__name__), DeprecationWarning)

# mymodule.py
import thirdpartymodule

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:

# thirdpartymodule.py
import warnings
warnings.warn("{} is deprecated".format(__name__), DeprecationWarning,
              stacklevel=2)

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:

warnings.warn("this module is deprecated", DeprecationWarning,
              stacklevel=10)

and on Python 3.4, the correct way to deprecate a module is:

warnings.warn("this module is deprecated", DeprecationWarning,
              stacklevel=8)

(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]

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]

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]

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]

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]

It would be better to skip _frozen_importlib frames automatically instead of forcing end users to use special API.

[brettcannon]

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]

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:

https://github.com/search?p=2&q=warnings.warn+DeprecationWarning+stacklevel&ref=searchresults&type=Code&utf8=%E2%9C%93

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

if sys.version_info[:2] == (3, 3):
    stacklevel = 10
elif sys.version_info[:2] == (3, 4):
    stacklevel = 8
else:
    stacklevel = 2
warnings.warn("{} is deprecated".format(__name__), DeprecationWarning, stacklevel=stacklevel)

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]

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]

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]

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]

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:

frame = sys._get frame(1)
if is_import_machinery(frame.module_name):
  skip_frame = lambda modname: False
else:
  skip_frame = is_import_machinery
def next_unskipped_frame(f):
  new = f
  while new is f or skip_frame(new.module_name):
    new = new.caller
for i in range(stacklevel - 1):
  frame = next_unskipped_frame(frame)

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.