Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow variable stacklevel for deprecation warnings. #6138

Closed
wants to merge 1 commit into from

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Dec 22, 2021

Each decoration needs to be manually checked, I have ideas on how to
make that better, by inspecting the above frames and look whether there
is a sentinel asking to add 1 to the stack number, but not tonight.

closes #6137

Alternative would be to use something like:

def extra_stack():
     import sys
     f = sys._getframe()
     fback = f.f_back
     count = 0
     while '__DEPRECATION_SKIP_STACK__' in fback.f_locals.keys():
         count +=1
         fback = fback.f_back
     return count

And mark corresponding decorators with __DEPRECATION_SKIP_STACK__ = 1.
or

def extra_stack(module_name='skimage'):
     import sys
     f = sys._getframe()
     fback = f.f_back
     count = 0
     while module_name in fback.f_global['__name__']:
         count +=1
         fback = fback.f_back
     return count

Which would always compute stacklevels so that deprecation warnings are emitted outside of skimage, potentially with a global toggle for skimage own CI Suite.

Description

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.

Each decoration needs to be manually checked, I have ideas on how to
make that better, by inspecting the above frames and look whether there
is a sentinel asking to add 1 to the stack number, but not tonight.

closes scikit-image#6137
@pep8speaks
Copy link

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

Line 252:80: E501 line too long (94 > 79 characters)
Line 273:43: E226 missing whitespace around arithmetic operator
Line 287:73: E226 missing whitespace around arithmetic operator
Line 287:80: E501 line too long (95 > 79 characters)

Line 49:80: E501 line too long (80 > 79 characters)

Line 19:80: E501 line too long (80 > 79 characters)

Line 51:80: E501 line too long (80 > 79 characters)

Line 96:80: E501 line too long (80 > 79 characters)
Line 435:80: E501 line too long (80 > 79 characters)
Line 754:80: E501 line too long (80 > 79 characters)
Line 936:80: E501 line too long (80 > 79 characters)

Line 193:80: E501 line too long (80 > 79 characters)

Line 13:80: E501 line too long (80 > 79 characters)

Line 112:80: E501 line too long (80 > 79 characters)

Line 104:80: E501 line too long (80 > 79 characters)
Line 175:80: E501 line too long (80 > 79 characters)

@hmaarrfk
Copy link
Member

i like the explicit parameter approach.

Adding additional global variables is tricky to get by correct, and I feel like it won't scale well with additional support for other interpreters.

i don't have time to read the code now in full but i do think it is an interesting approach.

the other thing i would say, i often as a keyword only argument in some of my functions called:

  • _stacklevel_increment
    to allow one to forward warnings an arbitrary amount up the call stack.

@utils.deprecate_kwarg({'max_iter': 'max_num_iter'}, removed_version="1.0",
deprecated_version="0.19")
deprecated_version="0.19", extra_stacklevel=1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
deprecated_version="0.19", extra_stacklevel=1)
deprecated_version="0.19")

This is causing the CI failures: extra_stacklevel is defined for deprecate_kawarg

@rfezzani
Copy link
Member

@Carreau, as an alternative, please see #6183 that defines a decorator's base class for storing the callstack length of each decorated function to automatically compute the correct warning stacklevel.

@@ -208,7 +208,7 @@ def __init__(self, kwarg_mapping, deprecated_version, warning_msg=None,
"for `{func_name}`. ")
if removed_version is not None:
self.warning_msg += (f'It will be removed in '
f'version {removed_version}.')
f'version {removed_version}. ')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rfezzani please cherry-pick this for #6183 😉

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thank you 😉

rfezzani pushed a commit to rfezzani/scikit-image that referenced this pull request Jan 17, 2022
@grlee77
Copy link
Contributor

grlee77 commented Feb 8, 2022

closing this as we ended up going with the approach in #6183. Thanks for initiating this work, @Carreau

@grlee77 grlee77 closed this Feb 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect stacklevel for deprecate_multichannel_kwarg deprecation warnings.
6 participants