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

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.

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

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.

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.

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

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"

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.

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.

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.

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.

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.

Nathaniel's solution is basically what I came up with in issue bpo-23810 except I simply skipped subtracting from the stack level if it was found to be an internal import frame instead of swapping in and out a callable.

Yeah, yours is probably better in fact, I was just trying to make the semantics as obvious as explicit as possible for a comment :-)

Here is a pure Python patch that skips any frames that mentions 'importlib' and '_bootstrap' in the filename (it also skips _warnings.warn for easy testing since I didn't implement the C part). The only side-effect is that negative stack levels won't be the same because of the change in line numbers, but in that instance I'm not going to worry about it. It also causes test_venv and test_warnings to fail but I have not looked into why.

And still need a call by Larry as to whether this will go into 3.5.0.

Here is a new version of the patch with Nathaniel's and and Berker's comments addressed.

Here is an approach that somewhat merges Nathaniel's proposed solution and mine.

Could someone do a quick code review of my latest patch? If the design looks sane I might be able to steal some time tomorrow to start on the C implementation.

"Hybrid Nathaniel/Brett approach" LGTM

Here is a patch that adds the C version of the frame skipping. Unfortunately it fails under test_threading, test_subprocess, test_multiprocessing_spawn. It's due to is_internal_frame() somehow although setting a breakpoint in gdb in that function never triggers. The return value is -11 from the interpreter and I don't remember what that represents.

The C implementation is making me nervous. My gut feeling is the Python implementation would be easier to get right.

I still don't quite understand: what is the user-perceived result of this change? Module authors issuing a DeprecationWarning can now use stacklevel=2 instead of stacklevel=10?

Is it really *impossible* to "correctly issue a deprecation warning for a module", as the title asserts? Or does the new import system simply make it *tiresome*?

if sys.version_info.major == 3 and sys.version_info.minor == 4:
  stacklevel = 8
elif  sys.version_info.major == 3 and sys.version_info.minor == 4:
  stacklevel = 10
else:
  stacklevel = 2 # I bet they fixed it in 3.6!

warnings.warn("{} is deprecated".format(name), DeprecationWarning,
stacklevel=stacklevel)

That's Python for you, doing six "impossible" things before breakfast.

I still don't quite understand: what is the user-perceived result of this change? Module authors issuing a DeprecationWarning can now use stacklevel=2 instead of stacklevel=10?

Exactly. There are a lot of deprecated modules in the wild, and the correct way to mark a module as deprecated is by writing

warnings.warn("This module is deprecated!", stacklevel=2)

at the top-level of your module. Except that in Python 3.3 you have to use stacklevel=10, and in Python 3.4 you have to use stacklevel=8, and I haven't checked what you need in Python 3.5 but it may well be different again, because it depends on internal implementation details of the import system. Fortunately AFAICT from some code searches no-one is actually depending on this though; instead everyone just writes stacklevel=2 and gets the wrong result.

(This is made more urgent b/c people are increasingly relying on the stacklevel information to filter whether they even see the DeprecationWarning. E.g. I've seen multiple test suites which register a warnings filter that displays DeprecationWarnings iff they are attributed to the package-under-test, and IPython now shows deprecation warnings by default if the warning is attributed to interactive code -- see bpo-24294. So there's a good chance that users will only find out that the module they are using is deprecated if the stacklevel= is set correctly.)

If this has been broken since 3.3, I don't think it's a release blocker for 3.5. I'm willing to consider it a "bug" and accept a fix, but I'd prefer it to be as low-risk as possible (aka the Python version). Can someone fix the regressions?

And, if the C fix is better somehow, let's definitely get that into 3.6.

You're right, "impossible" is a slight exaggeration :-). As an alternative, every package could indeed carry around a table containing the details of importlib's call stack in every version of Python.

(I also haven't checked whether it's consistent within a single stable release series. I guess we could add a rule that 3.5.x -> 3.5.(x+1) cannot change the number of function calls inside the importlib callstack, because that is part of the public API, but I have less than perfect confidence that this rule has been enforced strictly in the past, and I don't think I'd want to be the one in charge of enforcing it in the future.)

So there are two approaches I see to solving this whole thing. One is for there to be a slight divergence between the C code and the Python code. For _warnings.warn(), nothing changes. For warnings.warn(), though, it does the expected frame skipping. This would mean that _warnings.warn() continues to exist for startup purposes but then user-level code and non-startup code uses the Python version that has the expected semantics. This requires figuring out why not importing _warnings.warn in warnings.py causes test failures.

The other option is someone helps me figure out why the C code is causing its test failures by triggering a -11 return in threaded/subprocess tests and prevent the divergence.

I have opened issue bpo-24938 to actually re-evaluate one of the key points of _warnings.c which was startup performance. This was done about 7 years ago which pre-dates freezing code and providing simple C wrappers as necessary for internal use in Python like we do with importlib.

I figured out what was causing the threading/subprocess problems (something in frame->f_code->co-filename was NULL), so the attached patch covers Python, C, and adds a test (as well as cleaning up the test_warnings file structure since it was old-school spread out in Lib/test).

I don't think Larry wants this going into 3.5 based on his previous comments (period, not just 3.5.0). Is that true, Larry? If so, would you accept a 3.5.0 patch to fix the warnings in formatter and imp to a more accurate stacklevel?

Well, this is making me nervous to apply during the RCs. But... I'm willing to risk it.

My price: I want to see this run on a bunch of otherwise-healthy buildbots to make sure it doesn't break any platforms.

In case you've never done such a thing, here's how: create a "server-side clone" on hg.python.org, apply the patch to that tree, and push to the server-side clone. Then grab a fistful of URLs to "custom" buildbots from here:

http://buildbot.python.org/all/waterfall

and visit each individual page, e.g.

http://buildbot.python.org/all/builders/AMD64%20Snow%20Leop%20custom

and use the "Force Build" GUI at the bottom to kick off a build using the server-side clone with the patch applied.

Nathaniel: If Brett doesn't have the time to deal with this, I can make the server-side clone and check in the patch onto it. You can then kick off the builds and report back the results.

Just to note, there's an easier way to run a custom build on multiple bots: go to http://buildbot.python.org/all/builders/ and scroll (way) down to the section for forcing a build on custom builders (you can search for 'Repo path:' (with the colon)), check the box next to all the builders you want, then fill out the Repo path, your name, reason, and revision. You can also force the build on *all* custom builders; search for the second hit on 'Repo path:'.

...might not hurt if we document that somewhere.

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.

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://hg@hg.python.org/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.

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.

Hooray! Thanks Larry.

Would it make sense to do a 3.4.x backport, or is that closed now with 3.5 being imminent?

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!

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.

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.

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.

Unless I'm overruled, yes.

ok

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