Add new `deprecate_parameter` helper by lagru · Pull Request #7256 · scikit-image/scikit-image

lagru · 2023-12-04T14:27:39Z

Description

The plan is for deprecate_parameter to be a complete drop-in for the former remove_arg and deprecate_kwarg decorators (which I'll remove in a follow-up PR). It's mean to address several concerns I have with our current solution:

Detecting stacklevel for deprecation warnings is broken #7257
Deprecating a parameter that is reachable as a positional one in favor of a keyword one isn't currently covered as a use case. remove_arg doesn't account for this, deprecate_kwarg ignores *args.
Using remove_arg, the deprecated parameter must be left in the signature during the deprecation cycle, for deprecate_kwarg it's meant to be removed. The new approach forces the deprecated parameter to a default value DEPRECATED that makes sure that the parameter is no longer in use and its status is immediately visible in the signature. Some linters such as PyCharm even highlight using the deprecated parameter as a potential error.
If both old and new parameter of a function wrapped with deprecate_kwarg are passed, only a warning is shown. In my opinion this should be an error, as it's totally ambiguous which passed value takes precedence.

This is work to make deprecations for skimage2 less work. See also

Checklist

A descriptive but concise pull request title
Docstrings for all functions
Unit tests
A gallery example in ./doc/examples for new features
Contribution guide is followed

Release note

Summarize the introduced changes in the code block below in one or a few sentences. The
summary will be included in the next release notes automatically:

...

The plan is for deprecate_parameter to be a complete drop-in for the former remove_arg and deprecate_kwarg decorators.

lagru · 2023-12-04T14:29:22Z

Cc you @rfezzani, as you were heavily involved with the implementation I am building on. Please let me know if I am stepping on your toes or unintentionally removing any of the nice perks of the previous iteration.

lagru · 2023-12-04T14:31:31Z

Still working on #7257.

skimage/_shared/tests/test_utils.py

skimage/_shared/utils.py

The old way of detecting the appropriate stacklevel for deprecation warnings was broken. The new approach should be simpler and raises a RuntimeError if the stacklevel can't be determined reliably.

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

lagru · 2023-12-08T10:50:00Z

Thanks @mkcor. I've incorporated all your suggestions. I'll mark this as ready for review, since this completes the new decorator deprecate_parameter. I'll remove the old remove_arg and deprecate_kwarg in a follow-up PR to keep this easier to review.

I find this name clearer and easier to understand than the more abstract "stack rank" which doesn't make the connection to decorators and wrappers obvious.

mkcor

Thanks for your patience, @lagru! I have some pending questions which I'm submitting now, while I read the series of tests you've added...

mkcor · 2023-12-18T15:27:02Z

skimage/_shared/tests/test_utils.py

+
+        .. versionadded:: 0.10
+    arg1 : int, optional
+        Second unchanged parameter.


Hmm... Since arg1 is added in version 0.10, I understand that no change happens to it (at version 0.12, in the context of this mock function), but I guess its description shouldn't be exactly parallel to arg0's, which was 'always' there, i.e., wasn't added in version 0.10.

Suggested change

Second unchanged parameter.

Parameter added (not replacing any other parameter).

I am not following. In this test function arg1 is a parameter that is unaffected by the decorators. At least to the extend that it was there before. Adding the decorators only affects it in the sense that it's no longer reachable as a positional parameter because that would require passing deprecated parameters before, e.g.:

_func_deprecated_params(0, 1, 2, 3)

3 is passed to arg1 but 1, 2 are triggering deprecation warnings.

This is a quirk of deprecating positional arguments. Positional args can only be reached by triggering a deprecation warning if there are deprecated positional args in front. But that's also very useful because at the end of the deprecation we can switch the remaining parameters to keyword-only args!

Oops, sorry, because of the blank line before

.. versionadded:: 0.10

my eyes associated it with what's below (arg1 : int, optional instead of above (new1 : int, optional).

But that's also very useful because at the end of the deprecation we can switch the remaining parameters to keyword-only args!

Nice! So, at the end of the deprecation cycle, we'll have the following function signature

def _func_replace_params( arg0, new0=None, new1=None, *, arg1=None ):

? Or even

def _func_replace_params( arg0, *, new0=None, new1=None, arg1=None ):

? Maybe I'll find the answer as I finish reviewing... 🙏

Correct. After the deprecation cycle we can be sure that users always got a warning if they used any parameter following old0 as a positional one. So we can move over to def _func_replace_params(arg0, *, new0=None, new1=None, arg1=None) 🎉.

Thinking about this, there is some case that is somewhat tricky to deprecate with a grace period where both APIs work at the same time. When changing

def foo(a, b, c): # old API # ??? def foo(a, c): # new API

foo(1, 2) throws a warning because b is deprecated. But if users still want to use c while avoiding the warning and being future proof, they have to use foo(1, c=2). So b needs a default parameter in the meantime, which also makes c need a default parameter, at least temporarily. Do you follow? I find this hard to explain. 😅

Right. Yes, I'm following... Isn't this case covered by line 487 of skimage/_shared/tests/test_utils.py, when you

assert _func_deprecated_params(1, arg1=3) == (1, DEPRECATED, DEPRECATED, 3)

?

Oh, sorry, maybe I'm getting only now what you really(?) meant: The utilities you're providing with this PR will let us handle the deprecation (any deprecation) on a 'formal' ('external') level but, in cases such as what you're describing, we'll have to adapt the actual code (inside the function) to handle all cases during the deprecation period: if function call is foo(1, 2, 3) elif foo(1, 2) elif foo(1, c=2) else... error? So, yes, it will be more work that just decorating the function with the new deprecate_parameter helper. Is that what you meant?

Yes, your second guess is correct. Though, I argue that in cases I'm describing, it's very difficult to do a proper deprecation path because a new and old positional API can't be valid at the same time.

skimage/_shared/tests/test_utils.py

mkcor · 2023-12-18T16:07:00Z

@lagru I really like 55f4bf6 because, even if 'stack rank' actually makes sense, it's so much easier/faster to grasp the meaning of 'count wrappers!'

Along these lines... I've noticed you used 'replace' (replace parameters, replace docstring, replace warning, ...) where I would rather expect 'update' -- I find 'replace' a bit non-standard in a computing context. I feel bad for asking you for a change affecting so many lines of code, since you've put in a lot of effort in this very comprehensive PR! 😬

mkcor · 2023-12-18T16:11:39Z

PS: Personally, statistics comes to mind first when I read 'replace' in a computing context.

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

lagru · 2023-12-22T12:34:25Z

Re #7256 (comment): This is semantics and may depend on which specific cases you are referring to but I feel like it should be "replace parameter".

It may just be my mental model but I find "update parameter" misleading. In my mind we are replacing the deprecated parameter with a successor. It's not a modified version of the former which "update" would suggest.

I am not sure if you are getting at the test_replace_docstrings and test_replace_warning with the other references. I intended those names to say test the docstring / warning for the "replace" case as opposed to the "remove" case. See test_remove_docstrings and test_remove_warning which don't test that docstring and warning are removed. I am open to clearer names of course! :)

lagru · 2023-12-22T12:52:07Z

About deprecate_kwarg: I originally intended to replace all occurrences with deprecate_parameter but now I am not so sure if the work is worth it. We could just remove deprecate_kwarg after the last deprecation using that function is complete. The deprecation experience may not be as smooth or "correct" but it still works. Though, we would miss a few opportunities to introduce new args as "keyword-only". What do you think?

lagru · 2023-12-22T13:33:10Z

Failure is unrelated and addressed in #7275.

skimage/_shared/tests/test_utils.py

mkcor · 2023-12-23T18:15:08Z

Ok, I'm fine with the term 'replace.'

See test_remove_docstrings and test_remove_warning which don't test that docstring and warning are removed.

Yes... Naively speaking, I would find it more consistent if these were named test_deprecate_docstrings and test_deprecate_warning -- what do you think?

lagru · 2024-01-05T12:45:27Z

Happy new year @mkcor! :) It makes sense to use clearer names for the tests. Let me know if you are happy with 5556066. I still decided to keep the tests for both cases separated. I dropped the deprecate part as it is already referenced in the test class name.

lagru · 2024-01-05T13:22:05Z

Weird, I can't explain the test failure in the pre-release job yet but it seems unrelated to this PR. I can reproduce the unexpected warning if I install pytest==8.0.0rc1..

mkcor · 2024-01-09T14:24:20Z

About deprecate_kwarg: I originally intended to replace all occurrences with deprecate_parameter but now I am not so sure if the work is worth it. We could just remove deprecate_kwarg after the last deprecation using that function is complete. The deprecation experience may not be as smooth or "correct" but it still works. Though, we would miss a few opportunities to introduce new args as "keyword-only". What do you think?

Very good question. I would tend to be pragmatic (lazy?) and keep deprecate_kwarg where it's currently used: The deprecation experience isn't really consistent then, indeed, but I would consider the situation 'good enough.' If we want to make new args 'keyword-only,' we'll make them so subsequently (as was done in #4187).

mkcor · 2024-01-09T14:28:00Z

Weird, I can't explain the test failure in the pre-release job yet but it seems unrelated to this PR.

Yes, this warning (treated as error) shows up in the other recent PRs as well.

mkcor

Thanks for all this work, @lagru! My suggestions are minor wording tweaks, nothing blocking.

skimage/_shared/utils.py

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

lagru · 2024-01-10T12:46:28Z

🎉 Thank you for the reviews @mkcor! Excited to hack away at skimage2 now.

Add new deprecate_parameter

ea278ce

The plan is for deprecate_parameter to be a complete drop-in for the former remove_arg and deprecate_kwarg decorators.

lagru added the 🔧 type: Maintenance Refactoring and maintenance of internals label Dec 4, 2023

lagru mentioned this pull request Dec 4, 2023

Detecting stacklevel for deprecation warnings is broken #7257

Closed

lagru mentioned this pull request Dec 4, 2023

Deprecate function plot_matches in favor of plot_matched_features #7255

Merged

stefanv approved these changes Dec 5, 2023

View reviewed changes

mkcor reviewed Dec 7, 2023

View reviewed changes

skimage/_shared/tests/test_utils.py Outdated Show resolved Hide resolved

mkcor reviewed Dec 7, 2023

View reviewed changes

skimage/_shared/utils.py Outdated Show resolved Hide resolved

skimage/_shared/utils.py Outdated Show resolved Hide resolved

skimage/_shared/utils.py Outdated Show resolved Hide resolved

lagru and others added 4 commits December 8, 2023 11:36

Add transparent automatic stacklevel detection

3028020

The old way of detecting the appropriate stacklevel for deprecation warnings was broken. The new approach should be simpler and raises a RuntimeError if the stacklevel can't be determined reliably.

Clean-up remaining tests for deprecate_parameter

17710fd

Apply suggestions from code review

2d9ff7b

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

Use consistent quotes for strings

2a61ad4

lagru marked this pull request as ready for review December 8, 2023 10:50

Refactor to more expressive _count_wrappers

55f4bf6

I find this name clearer and easier to understand than the more abstract "stack rank" which doesn't make the connection to decorators and wrappers obvious.

mkcor reviewed Dec 18, 2023

View reviewed changes

Fix grammar

dcee85c

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

lagru mentioned this pull request Dec 22, 2023

Fix warningfilter for deprecation in SciPy 1.12.0rc1 #7275

Merged

Merge branch 'main' into deprecate-parameter

5492bdd

mkcor reviewed Dec 23, 2023

View reviewed changes

skimage/_shared/tests/test_utils.py Show resolved Hide resolved

mkcor reviewed Dec 23, 2023

View reviewed changes

skimage/_shared/tests/test_utils.py Show resolved Hide resolved

lagru mentioned this pull request Jan 5, 2024

Align arguments for draw.ellipse() and draw.ellipse_perimeter() #7263

Open

Use clearer names for docstring and warning tests

5556066

lagru requested a review from mkcor January 5, 2024 12:45

mkcor approved these changes Jan 9, 2024

View reviewed changes

lagru and others added 2 commits January 9, 2024 17:54

Apply suggestions from code review

0b32e05

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

Use full sentences in longer error message

1585ae8

Co-authored-by: Marianne Corvellec <marianne.corvellec@ens-lyon.org>

mkcor merged commit bd20f1f into scikit-image:main Jan 10, 2024

stefanv added this to the 0.23 milestone Jan 10, 2024

lagru deleted the deprecate-parameter branch January 10, 2024 12:45

mkcor mentioned this pull request Jan 12, 2024

Complete deprecations and TODOs for 0.23 #7290

Merged

lagru added the 🥾 Path to skimage2 A step towards the new "API 2.0" label Jan 18, 2024

lagru mentioned this pull request Feb 22, 2024

Standardize function signature of compare_images. #7322

Merged

lagru mentioned this pull request Apr 9, 2024

0.23 release #7374

Closed

	Second unchanged parameter.
	Parameter added (not replacing any other parameter).

Uh oh!

Conversation

lagru commented Dec 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Release note

Uh oh!

lagru commented Dec 4, 2023

Uh oh!

lagru commented Dec 4, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lagru commented Dec 8, 2023

Uh oh!

mkcor left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mkcor commented Dec 18, 2023

Uh oh!

mkcor commented Dec 18, 2023

Uh oh!

lagru commented Dec 22, 2023

Uh oh!

lagru commented Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lagru commented Dec 22, 2023

Uh oh!

Uh oh!

Uh oh!

mkcor commented Dec 23, 2023

Uh oh!

lagru commented Jan 5, 2024

Uh oh!

lagru commented Jan 5, 2024

Uh oh!

mkcor commented Jan 9, 2024

Uh oh!

mkcor commented Jan 9, 2024

Uh oh!

mkcor left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lagru commented Jan 10, 2024

Uh oh!

Reviewers

Assignees

Labels

lagru commented Dec 4, 2023 •

edited

Loading

lagru commented Dec 22, 2023 •

edited

Loading