Fix decorators warnings stacklevel

[rfezzani]

Description

Fixes #6137.

This PR introduces the DecoratorBaseClass base class that stores the callstack length for each decorated function and allows the computation of the warnings stacklevel automatically, unlike #6138 that introduces the extra_stacklevel argument for each decorator.

Checklist

• Docstrings for all functions
• Gallery example in ./doc/examples (new features only)
• Benchmark in ./benchmarks, if your changes aren't covered by an
  existing benchmark
• Unit tests
• Clean style in the spirit of PEP8
• Descriptive commit messages (see below)

For reviewers

• Check that the PR title is short, concise, and will make sense 1 year
  later.
• Check that new functions are imported in corresponding __init__.py.
• Check that new features, API changes, and deprecations are mentioned in
  doc/release/release_dev.rst.

[rfezzani]

As a demonstration, running the following script

import numpy as np

from skimage.feature import hog
from skimage.transform import pyramid_gaussian

img = np.random.rand(100, 100, 3)

x = hog(img, multichannel=True)
pyramid_gaussian(None, multichannel=True)

in main branch gives

/.../skimage/_shared/utils.py:338: FutureWarning: multichannel is a deprecated argument name for hog. It will be removed in version 1.0.Please use channel_axis instead.
return func(*args, **kwargs)
/tmp/test.py:9: FutureWarning: multichannel is a deprecated argument name for pyramid_gaussian. It will be removed in version 1.0.Please use channel_axis instead.
pyramid_gaussian(None, multichannel=True)

and in current branch gives

/tmp/test.py:8: FutureWarning: multichannel is a deprecated argument name for hog. It will be removed in version 1.0.Please use channel_axis instead.
x = hog(img, multichannel=True)
/tmp/test.py:9: FutureWarning: multichannel is a deprecated argument name for pyramid_gaussian. It will be removed in version 1.0.Please use channel_axis instead.
pyramid_gaussian(None, multichannel=True)

Another example

import functools
import warnings
from skimage._shared.utils import DecoratorBaseClass

class DumbDecorator(DecoratorBaseClass):

    def __init__(self, idx):
        self.idx = idx

    def __call__(self, func):
        stack_rank = self.get_stack_length(func)
        self.update_stack_length(func)
        
        @functools.wraps(func)
        def fixed_func(*args, **kwargs):
            stacklevel = 1 + self.get_stack_length(func) - stack_rank
            warnings.warn(
                f"Message from {self.idx} with stacklevel = {stacklevel}",
                FutureWarning, stacklevel=stacklevel)
            return func(*args, **kwargs)

        return fixed_func


@DumbDecorator(3)
@DumbDecorator(2)
@DumbDecorator(1)
@DumbDecorator(0)
def do_nothing():
    return

do_nothing()

gives

/tmp/test.py:32: FutureWarning: Message from 3 with stacklevel = 2
do_nothing()
/tmp/test.py:32: FutureWarning: Message from 2 with stacklevel = 3
do_nothing()
/tmp/test.py:32: FutureWarning: Message from 1 with stacklevel = 4
do_nothing()
/tmp/test.py:32: FutureWarning: Message from 0 with stacklevel = 5
do_nothing()

[pep8speaks]

Hello @rfezzani! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2022-02-08 14:44:53 UTC

[mkcor]

@Carreau @hmaarrfk what do you think of this approach?

[rfezzani]

Thank you @mkcor for your review 😉

[hmaarrfk]

I have to say i'm not a super big fan of adding a variable to the function.

I was adding a keyword argument to my functions in my own libraries that acts like _stacklevel_increment. My decorators were already inspecting the function signature, so they would detect this argument, and add "1" to it.

This allows other people to build up on your library. For example, maybe they want to pass warnings through to the end user. They can also use _stacklevel_increment (if you choose to document it), and then ensure that the warnings go to the appropriate user.

The advantage is that it might be a little more explicit, and less "magical". I didn't even know you could add variables to functions like that. Are there some implementations of python that don't let you do that?

All that said, I think that this does solve a problem, and I don't particularly have the bandwidth to solve it in a different way myself.

[Carreau]

I don't have particular skin in the game, but I think at some point it should be special value handled by CPython itself.

[rfezzani]

Thank you @hmaarrfk for your comments.

The advantage is that it might be a little more explicit, and less "magical". I didn't even know you could add variables to functions like that. Are there some implementations of python that don't let you do that?

I am not sure to understand here, sorry :/ are you talking about DecoratorBaseClass._stack_length?

By the way, thinking a little bit more of it, this approach has the drawback of not managing decorators not inheriting from DecoratorBaseClass 🙁, but this can be fixed using the __wrapped__ attribute of a decorated functions 😉.

[rfezzani]

With my last modifications:

• stack_rank is obtained using the __wrapped__ attribute to support decorators not inheriting from _DecoratorBaseClass
• stack_length is obtained using the stack_rank of the "final" decorated function obtained from func.__globals__ (here I assume that the decorated function is imported <edit> globally , which is obviously true </edit>)
• _DecoratorBaseClass is now only used to store functions' stack_length to save some overhead.

[rfezzani]

A demo for current PR

import numpy as np
from skimage.feature import hog
from skimage.transform import pyramid_gaussian

img = np.random.rand(100, 100, 3)


def func():
    hog(img, multichannel=True)  # L9


func()

hog(img, multichannel=True)  # L14

pyramid_gaussian(None, multichannel=True)  # L16

Gives

$ python /tmp/test.py 
/tmp/test.py:9: FutureWarning: `multichannel` is a deprecated argument name for `hog`. It will be removed in version 1.0. Please use `channel_axis` instead.
  hog(img, multichannel=True)  # L9
/tmp/test.py:14: FutureWarning: `multichannel` is a deprecated argument name for `hog`. It will be removed in version 1.0. Please use `channel_axis` instead.
  hog(img, multichannel=True)  # L14
/tmp/test.py:16: FutureWarning: `multichannel` is a deprecated argument name for `pyramid_gaussian`. It will be removed in version 1.0. Please use `channel_axis` instead.
  pyramid_gaussian(None, multichannel=True)  # L16

VS main branch

$ python /tmp/test.py 
/home/rfez/Projects/Misc/skimage/skimage/_shared/utils.py:338: FutureWarning: `multichannel` is a deprecated argument name for `hog`. It will be removed in version 1.0.Please use `channel_axis` instead.
  return func(*args, **kwargs)
/tmp/test.py:16: FutureWarning: `multichannel` is a deprecated argument name for `pyramid_gaussian`. It will be removed in version 1.0.Please use `channel_axis` instead.
  pyramid_gaussian(None, multichannel=True)  # L16

[rfezzani]

Should this PR be backported to v0.19?

[rfezzani]

Thank you @mkcor 😉

[mkcor]

@hmaarrfk thank you very much for your feedback!

I have to say i'm not a super big fan of adding a variable to the function.

So you would +1 this PR here (#6183) over #6138, right?

I was adding a keyword argument to my functions in my own libraries that acts like _stacklevel_increment. My decorators were already inspecting the function signature, so they would detect this argument, and add "1" to it.

Same question as @rfezzani, I guess... By _stacklevel_increment, did you mean _stack_length?

This allows other people to build up on your library. For example, maybe they want to pass warnings through to the end user. They can also use _stacklevel_increment (if you choose to document it), and then ensure that the warnings go to the appropriate user.

I like this idea. 🙂

[mkcor]

Should this PR be backported to v0.19?

@rfezzani good question.

@grlee77 are you planning on releasing v0.19.2 very soon? What's your bandwidth like? Thank you!

[mkcor]

I don't have particular skin in the game, but I think at some point it should be special value handled by CPython itself.

@Carreau ok, thanks for all your inputs.

[grlee77]

@grlee77 are you planning on releasing v0.19.2 very soon? What's your bandwidth like? Thank you!

Yeah, we could do a new release later this week. It looks like #5262 is the only PR remaining to review. It seems reasonable to backport this as well, so I will add it to the milestone.

[alexdesiqueira]

@scikit-image/core I wonder what is wrong on AppVeyor 🤔

[grlee77]

Thanks @rfezzani, I think this will be very helpful to users and plan to backport it to v0.19.x as well.

I just kicked off CI again but think this should be ready to merge.

[grlee77]

Overall I prefer this to #6138 in that it avoids the potential for mistakes by contributors in either forgetting to provide the extra_stacklevel argument or setting it incorrectly. Thank you to both @rfezzani and @Carreau for working on resolving this issue!

[alexdesiqueira]

Thank you everyone!

[grlee77]

@meeseeksdev backport to v0.19.x

[rfezzani]

Thank you all for the reviews, and thank you @Carreau for the bug report and your initial work 😉