CLN: Improve setting of resample deprecation warnings' stacklevel
State: closed

- [N/A] closes #xxxx (see https://github.com/pandas-dev/pandas/pull/36369#issuecomment-698763910)
- [x] tests added / passed
- [x] passes `black pandas`
- [x] passes `git diff upstream/master -u -- "*.py" | flake8 --diff`
- [N/A] whatsnew entry

The essence of the problem is that the tests for deprecated resample kwargs require that the warning is raised exactly where the user called the resample functionality—i.e. with the appropriate `stacklevel`— (I guess so that the warning message is more user-friendly?), but there are multiple entry points that use these deprecated parameters, so this can't be easily solved by just adjusting the stacklevel by a constant amount.

The old solution was to centralize the management of these warnings in the greatest-common-denominator call site (which was `Grouper.__new__`) and "guess" the correct value of `stacklevel` by relying on the current call graph (and hoping it never changes). This of course is poorly maintainable. (For a more detailed explanation of what is wrong with this, see https://github.com/pandas-dev/pandas/pull/36369#issuecomment-694808777.)

The essence of my proposed solution (this PR) is to add a call to the deprecated-parameter-checking code to each of these entry points—this allows instead to use a constant `stacklevel` value (2) in each of these. Now since some of these public call sites appear in the call graph of others, we have to ensure this code is only called once within any call path starting at any of these entry points. This is what the `call_once` context manager takes care of.

Labels:
name: Resample
description: resample method
name: Deprecate
description: Functionality to remove in pandas

Comments:
Can you comment as to what problem this is solving?
> Can you comment as to what problem this is solving?

It's explained here: https://github.com/pandas-dev/pandas/pull/36369#issuecomment-694808777. @MarcoGorelli asked me to extract this into a separate PR.

**TL;DR:** The tests in `pandas.tests.resample.test_deprecated` required that any warnings about deprecated `resample` kwargs be emitted at (i.e. with the `stacklevel` corresponding to) the call site of `resample` and related methods. Previously, these warnings were all managed by `Grouper.__new__`, which all "call paths" happened to end up in. The correct `stacklevel` was set by relying on the call graph (assuming it would never change); this was very brittle, as demonstrated by #36369. Now the warnings are managed at each public call site, ensuring that the correct `stacklevel` is always set even if changes are made to the call graph.
This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.
Sorry for the delay here, I just found this PR to be pretty difficult to understand (e.g. I don't yet know what `asyncio` does) so I don't think I'm the right person to review it

Hopefully someone else will take this else on - otherwise, is there a way to simplify?
> This pull request is stale because it has been open for thirty days with no activity. Please update or respond to this comment if you're still interested in working on this.

Waiting for review.

> Sorry for the delay here, I just found this PR to be pretty difficult to understand (e.g. I don't yet know what `asyncio` does) so I don't think I'm the right person to review it

The essence of the problem is that the tests for deprecated resample kwargs require that the warning is raised exactly where the user called the resample functionality—i.e. with the appropriate `stacklevel`— (I guess so that the warning message is more user-friendly?), but there are multiple entry points that use these deprecated parameters, so this can't be easily solved by just adjusting the stacklevel by a constant amount.

The old solution was to centralize the management of these warnings in the greatest-common-denominator call site (which was `Groupby.__new`) and "guess" the correct value of `stacklevel` by relying on the current call graph (and hoping it never changes). This of course is poorly maintainable.

The essence of my proposed solution (this PR) is to add a call to the deprecated-parameter-checking code to each of these entry points—this allows instead to use a constant `stacklevel` value (2) in each of these. Now since some of these public call sites appear in the call graph of others, we have to ensure this code is only called once within any call path starting at any of these entry points. This is what the `call_once` context manager takes care of.

The `asyncio` stuff only comes in due to the nature of this context manager: whenever it's called, `call_once` needs to know, *for the current call stack*, whether there is an active `call_once` context for the given callback—which would mean that the callback has been called already—to know whether to call the callback when it's entered or not. The key point is that this state is *local to the current call stack*. 

For example, suppose we kept track of this state as a global variable, and suppose we had two threads T1 T2 running the same code. Suppose T1 enters an outermost `call_once` context for callback C (so C gets called), but before T1 exits this context, T2 tries to enter its own [outermost] `call_once` context for C: since this information is stored globally, the context manager implementation would think that there already was an active context for C, so C wouldn't be called (when it should have been).

So, long story short, this state needs to be *at least* thread-local. But, more specifically, it has to be *call-stack*-local (or what the Python docs call *context*-local). This is where `contextvars` comes in; it offers context-local storage. The only other main, "standard" mechanism that deals with multiple call stacks simultaneously besides threads is `asyncio`: coroutines run concurrently in an interleaved manner. So that's why I mention it in the PR and added tests for it.

> Hopefully someone else will take this else on - otherwise, is there a way to simplify?

A way to simplify this would be not to require the warnings to be raised with stacklevel corresponding to the public call site—then none of this would be needed 🙂 
ok so you are trying to make less brittle tests but this doesn't actually change any user facing behavior? eg the warning that is show might be at the right stack level or not right now?

you went to a lot of effort here
but if this is only for the purpose of making the tests works i would just not check the stack level 

i want to remove this all of these depreciation warnings anyhow (we should have done for 1.0'actually)

so want to make this simpler 
> ok so you are trying to make less brittle tests but this doesn't actually change any user facing behavior? eg the warning that is show might be at the right stack level or not right now?

Yes, exactly. I encountered this problem while changing some docs in #36369, and since this evolved into its own change (and was unrelated to the docs themselves), Marco asked me to extract this into a separate PR.

> but if this is only for the purpose of making the tests works i would just not check the stack level 

Yes that's definitely a better solution, makes everything simpler.

> you went to a lot of effort here

I had fun trying to come up with this in any case :) 

> i want to remove this all of these depreciation warnings anyhow (we should have done for 1.0'actually)
> 
> so want to make this simpler 

Nice, is there a PR or issue I can follow to know when that's done?

> Nice, is there a PR or issue I can follow to know when that's done?

when we do 2.0 :-<
> > Nice, is there a PR or issue I can follow to know when that's done?
> 
> when we do 2.0 :-<

Meanwhile, can this or an equivalent fix be merged so that stuff that depends on this working properly (e.g. #36369) can go ahead? Once the tests are removed / deprecated kwargs are gone this can all be deleted.

Or if you prefer I can make a PR to remove the unnecessary stacklevel tests.
> > > Nice, is there a PR or issue I can follow to know when that's done?
> > 
> > 
> > when we do 2.0 :-<
> 
> Meanwhile, can this or an equivalent fix be merged so that stuff that depends on this working properly (e.g. #36369) can go ahead? Once the tests are removed / deprecated kwargs are gone this can all be deleted.
> 
> Or if you prefer I can make a PR to remove the unnecessary stacklevel tests.

yes I would just remove the uncessary checking of stacklevel (pref not to make this too complicated); your idea is good but i would rather just fix the tests here).
This has been superseded by #37603 (which has now been closed); I'm closing this as well. I won't be deleting the branch though as IMHO this was quite an elegant solution, the code will be available in case anyone finds it useful :)