ENH Add Deprecating Position Arguments Helper #13311

thomasjpfan · 2019-02-27T14:47:39Z

Reference Issues/PRs

Addresses #12805

What does this implement/fix? Explain your changes.

Adds a function decorator, warn_args , that can wraps a function and issues a warning when any of the positional arguments passed after * will issue a warning.

For functions:

@_deprecate_positional_args
def dbscan(X, eps=0.5, *, min_samples=4, metric='minkowski'):
    pass

Calling dbscan(X, 0.1, 5) will result with a DeprecationWarning:

DeprecationWarning: Should use keyword args: min_samples=5

For classes

class LogisticRegression:
    
    @_deprecate_positional_args
    def __init__(self, penalty='l2', *, dual=False):

        self.penalty = penalty
        self.dual = dual

Calling LogisticRegression('l2', True) will result with a DeprecationWarning:

Should use keyword args: dual=True

Any other comments?

_deprecate_positional_args uses the fact that the first element of a class method is traditionally named self to determine if the function is called in a instance method like __init__.

jnothman · 2019-02-28T09:00:43Z

Your agility across different kinds of development problems is impressive, @thomasjpfan. Thanks for the good work.

GaelVaroquaux · 2019-02-28T14:25:23Z

Is the idea that after two releases, we remove the "*" that gets added in functions? (I find that it really looks ugly. I could see it confusing beginners.

jnothman

We need to make sure that this warning raises in our test suite and doc building.

We should also document this is what's new and perhaps more clearly somewhere in the user guide??

The only problem I see at the moment is that the * doesn't show in the API reference documentation. I don't think it's a big problem, since almost all args can be passed as kwargs.

Do we need to test support for non-__init__ methods?

jnothman · 2019-02-28T14:42:45Z

sklearn/linear_model/tests/test_logistic.py

@@ -1788,3 +1788,10 @@ def test_penalty_none(solver):
        "LogisticRegressionCV",
        lr.fit, X, y
    )
+
+
+def test_logistic_warns_with_args():


remove this. it's not practical or helpful to have this for everything.

jnothman · 2019-02-28T14:48:12Z

sklearn/utils/validation.py

+            args_msg = ['{}={}'.format(name, arg)
+                        for name, arg in zip(kwonlyargs[:extra_args],
+                                             err_args[-extra_args:])]
+            warnings.warn("Should use keyword args: "


Something more forceful. "Pass xxxx as keyword args (e.g. xxxx=...). From version 0.24 passing these as positional arguments will result in an error."

Then we need to obligate ourselves to finish all the decorating by 0.22.

jnothman · 2019-02-28T14:48:48Z

sklearn/utils/validation.py

@@ -936,3 +937,38 @@ def check_non_negative(X, whom):

    if X_min < 0:
        raise ValueError("Negative values in data passed to %s" % whom)
+
+
+def warn_args(f):


Maybe call it deprecate_positional_args?

jnothman · 2019-02-28T14:53:17Z

sklearn/utils/validation.py

+    def inner_f(*args, **kwargs):
+        extra_args = len(args) - len(orig_spec)
+        if extra_args > 0:
+            # ignore first 'self' argument for class methods


class -> instance

jnothman · 2019-02-28T14:59:23Z

sklearn/utils/validation.py

+    orig_spec = argspec.args
+
+    # Assumes class method has 'self' as first argument
+    is_class_method = orig_spec and 'self' == orig_spec[0]


Is inspect.ismethod applicable here? Maybe it's not bound yet.

is_class_method -> is_method

…ments

jnothman · 2019-03-01T06:19:14Z

@GaelVaroquaux

Is the idea that after two releases, we remove the "*" that gets added in functions? (I find that it really looks ugly. I could see it confusing beginners.

No, the idea is that that * is what makes this a hard constraint rather than a cultural convention, after which we are:

assured users are passing all but a few key params by name
able to reorder parameters as we see fit and hence introduce params
able to document params in any order (ideally under multiple section headings if we could get that introduced in numpydoc) so that we can order them either alphabetically or by their information / clustering (though this is hard) rather than ordering them roughly by date of addition!

…ments

codecov · 2019-05-31T16:59:58Z

Codecov Report

Merging #13311 into master will decrease coverage by 4.31%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master   #13311      +/-   ##
==========================================
- Coverage    96.8%   92.49%   -4.32%     
==========================================
  Files         393      420      +27     
  Lines       71859    74830    +2971     
  Branches     7866        0    -7866     
==========================================
- Hits        69564    69211     -353     
- Misses       2272     5619    +3347     
+ Partials       23        0      -23

Impacted Files	Coverage Δ
sklearn/utils/tests/test_validation.py	`98.55% <100%> (+0.02%)`	⬆️
sklearn/utils/validation.py	`99.64% <100%> (+0.01%)`	⬆️
sklearn/linear_model/logistic.py	`98.92% <100%> (+0.01%)`	⬆️
sklearn/ensemble/partial_dependence.py	`32.91% <0%> (-62.77%)`	⬇️
sklearn/ensemble/tests/test_partial_dependence.py	`61.85% <0%> (-38.15%)`	⬇️
sklearn/tree/export.py	`79.51% <0%> (-15.37%)`	⬇️
sklearn/tree/tests/test_export.py	`94.16% <0%> (-5.84%)`	⬇️
sklearn/cluster/mean_shift_.py	`95.14% <0%> (-3.89%)`	⬇️
sklearn/tests/test_site_joblib.py	`96.66% <0%> (-3.34%)`	⬇️
sklearn/covariance/elliptic_envelope.py	`97.22% <0%> (-2.78%)`	⬇️
... and 293 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5e0d1a1...2796868. Read the comment docs.

amueller · 2019-05-31T18:18:48Z

Does this need a slep/discussion/vote?

rth · 2019-07-12T20:33:00Z

I wonder what happens with auto-completion e.g. in Jupyter etc. Would it still work even with the decorator?

adrinjalali · 2019-07-13T12:11:46Z

@amueller

Does this need a slep/discussion/vote?

Here's the SLEP: scikit-learn/enhancement_proposals#19

@rth

I wonder what happens with auto-completion e.g. in Jupyter etc. Would it still work even with the decorator?

I put the output for ipython and VSCode in the SLEP

adrinjalali · 2019-07-13T12:12:49Z

@thomasjpfan as I pasted in the SLEP, the order of items in the hint shown by ipython seem different with the decorator, any chance we could fix/change that?

thomasjpfan · 2019-07-14T22:06:32Z

@adrinjalali Are you using the deprecate_positional_args version of this PR? In iPython and jupyter lab, the auto complete seems to be working for me. (ipython=7.6.1, jupyterlab=0.35.5)

adrinjalali · 2019-07-15T14:28:32Z

Yeah I took the decorator from this PR. I haven't tried jupyterlab though.

So you're saying you see something else than the one I pasted in the SLEP?

thomasjpfan · 2019-07-15T21:12:18Z

So you're saying you see something else than the one I pasted in the SLEP?

Yup. I get the same auto complete results for the wrapped and unwrapped version of the function. My implementation calls decorates the wrapped function with @wrap(f) which places the original function in f.__wrapped__. autocomplete should be using this for the original function signature.

amueller · 2019-07-17T17:32:37Z

for reference:
https://github.com/matplotlib/matplotlib/blob/507bd38add3b199ffc9148177e0781d9b3406793/lib/matplotlib/cbook/deprecation.py#L258-L400

adrinjalali · 2019-07-17T20:29:50Z

I quite like their idea of rename_parameter, delete_parameter, deprecate_parameter, etc, as decorators. It's so neat!

amueller · 2019-07-17T21:45:26Z

@adrinjalali I once started writing a library to do that:
https://github.com/amueller/futurepast
but I never had time to go through with it.

…ments

jnothman · 2019-09-17T23:20:39Z

SLEP009 is all but accepted. Please resolve conflicts and update for review.

thomasjpfan · 2019-09-20T00:41:52Z

Okay lets see how this looks like with #15005 (comment)

thomasjpfan · 2019-09-20T03:02:56Z

sklearn/manifold/tests/test_isomap.py

    noise_scale = 0.01

    # Create S-curve dataset
    X, y = datasets.samples_generator.make_s_curve(n_samples, random_state=0)

    # Compute isomap embedding
-    iso = manifold.Isomap(n_components, 2)
+    iso = manifold.Isomap(n_neighbors, n_components=2)


This really makes me want to do: Isomap(*, n_neighbors=5, n_components=2,...)

I'm happy with that

jnothman · 2019-09-20T04:36:50Z

Do you have a way of systematically considering all the signatures?

jnothman · 2019-09-20T05:26:25Z

Do you have a way of systematically considering all the signatures?

thomasjpfan · 2019-09-20T16:44:25Z

For working on this PR, I had a code snippet that checked for keyword only arguments based on order:

from inspect import signature, Parameter
from sklearn.utils.testing import all_estimators
from collections import defaultdict

whitelist = set([
    'n_components', 'estimator', 'base_estimator', 'n_clusters', 'n_neighbors', 'steps',
    'regressor', 'transformers', 'n_estimators', 'transformer_list', 'score_func', 'func',
    'degree', 'estimators'])

public_estimators = [tup for tup in all_estimators() if not tup[0].startswith("_")]
estimators_by_module = defaultdict(list)
for name, est in public_estimators:
    module = est.__module__
    estimators_by_module[module].append((name, est))
    
for module, estimators in estimators_by_module.items():
    print(module)
    for name, est in estimators:
        sig = signature(est)
        if sig.parameters:
            params = list(sig.parameters.items())
            first_name, first_param = params[0]
            if first_name in whitelist:
                if first_param.kind == Parameter.KEYWORD_ONLY:
                    print("*", name, first_param, "INCORRECT KEYWORD_ONLY")
                params = params[1:]
            # remove parameters without defaults
            params = [p for p in params if p[1].default != Parameter.empty]
            # the rest must be keywords only
            if not all(p[1].kind in [Parameter.KEYWORD_ONLY]
                       for p in params):
                print("*", name, "THE REST SHOULD BE KEYWORD_ONLY")

I added a test, test_estimators_keyword_only, to the whitelist and that the rest of the parameters are keyword only.

thomasjpfan · 2019-09-20T16:46:18Z

sklearn/tests/test_common.py

+    "name, Estimator",
+    [(name, Estimator)
+     for name, Estimator in all_estimators() if not name.startswith("_")])
+def test_estimators_keyword_only(name, Estimator):


This test makes sure that the whitelist is POSITION_OR_KEYWORD and that the rest of the parameters (without defaults) is KEYWORD_ONLY.

NicolasHug · 2019-09-25T19:36:05Z

I agree with #13311 (comment), can we first merge a simpler PR where only the decorator is introduced?

qinhanmin2014 · 2019-10-04T13:06:52Z

Here is the current whitelist:

Honestly I'm still uncomfortable about the long whitelist (e.g., PCA(2))). I think we introduce keyword only agrs because we want to force users to sprcify the name of the parameter, so the whitelist should be as short as possible. It's true that we'll break user's code, but we have a deprecation cycle so I think that's acceptable.

thomasjpfan · 2019-10-04T14:46:03Z

I agree with #13311 (comment), can we first merge a simpler PR where only the decorator is introduced?

After we merge this decorator, and we go through each module group by group. We would need to wait till we go through every module before we can release. Otherwise there will be some estimators that have this warning and others that do not.

Honestly I'm still uncomfortable about the long whitelist

@qinhanmin2014 What do you think the whitelist should be?

qinhanmin2014 · 2019-10-05T07:59:25Z

@qinhanmin2014 What do you think the whitelist should be?

Not sure, but shorter than current version, e.g., perhaps n_components, n_clusters and n_neighbors should not be in the list.

But as I've said in gitter, if this is the final decision, I won't oppose.

This reverts commit 24625fc.

This reverts commit d7f5228.

This reverts commit c83dddd.

This reverts commit 209e957.

This reverts commit cb0e2a5.

This reverts commit c826ba8.

…ments

adrinjalali

This looks good now. We need probably another PR to explain keyword only args and how we use them in sklearn I think. (Another example of where a blog would be nice)

thomasjpfan · 2019-10-06T12:54:49Z

This PR was stripped down to only include the decorator. It doesn’t use it anywhere.

rth

LGTM, given that this only add the helper function. Thanks @thomasjpfan !

thomasjpfan added 3 commits February 27, 2019 15:12

ENH Adds warn_args

d6f288d

ENH Updates deprecation message

e35a7c1

DOC Adds docstring to warn_args

ce7812a

jnothman reviewed Feb 28, 2019

View reviewed changes

thomasjpfan added 3 commits February 28, 2019 17:51

CLN Address comments

9afe110

Merge remote-tracking branch 'upstream/master' into warn_keyword_argu…

d2d1439

…ments

Trigger

489d3fd

thomasjpfan added 2 commits May 31, 2019 12:55

Merge remote-tracking branch 'upstream/master' into warn_keyword_argu…

5c660c5

…ments

CLN Address comments

2796868

adrinjalali mentioned this pull request Jul 13, 2019

SLEP009: keyword only arguments scikit-learn/enhancement_proposals#19

Merged

thomasjpfan added the API label Aug 6, 2019

thomasjpfan added 2 commits August 7, 2019 11:39

Merge remote-tracking branch 'upstream/master' into warn_keyword_argu…

b24617c

…ments

CLN Updates name

bf2633f

jnothman mentioned this pull request Sep 17, 2019

Implement SLEP009: keyword-only arguments #15005

Closed

40 tasks

TST Fix

d7f5228

thomasjpfan commented Sep 20, 2019

View reviewed changes

thomasjpfan added 3 commits September 20, 2019 11:42

CLN Uses signature instead of fullargspec

f1005a9

CLN Fix doctest

24625fc

TST Adds test to check keyword arguments

b944f59

thomasjpfan commented Sep 20, 2019

View reviewed changes

thomasjpfan added 9 commits October 5, 2019 13:33

Revert "CLN Fix doctest"

2e79984

This reverts commit 24625fc.

Revert "TST Fix"

251d8b8

This reverts commit d7f5228.

Revert "BUG Fix"

261ef35

This reverts commit c83dddd.

Revert "BUG Fix"

3b5ddfe

This reverts commit 209e957.

Revert "STY Fix"

001ba4e

This reverts commit cb0e2a5.

Revert "ENH Adds positional deprecations"

5a0504e

This reverts commit c826ba8.

REV Removes deprecation

bfa3af7

Merge remote-tracking branch 'upstream/master' into warn_keyword_argu…

a4d23e0

…ments

STY Fix

02239ab

adrinjalali approved these changes Oct 6, 2019

View reviewed changes

rth approved these changes Oct 7, 2019

View reviewed changes

rth changed the title ~~[MRG] Adds Deprecating Position Arguments Helper~~ ENH Add Deprecating Position Arguments Helper Oct 7, 2019

rth merged commit f450173 into scikit-learn:master Oct 7, 2019

hcho3 mentioned this pull request Sep 27, 2020

[RFC] Deprecate position arguments and encourage users to use keyword arguments only dmlc/xgboost#6172

Closed

adrinjalali mentioned this pull request Mar 2, 2022

Deprecate passing positional args to most of the public API huggingface/huggingface_hub#732

Closed

mathause mentioned this pull request Aug 10, 2022

Keyword only args for arguments like "drop" pydata/xarray#5531

Closed

Uh oh!

ENH Add Deprecating Position Arguments Helper #13311

ENH Add Deprecating Position Arguments Helper #13311

Uh oh!

Conversation

thomasjpfan commented Feb 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Uh oh!

jnothman commented Feb 28, 2019 via email

Uh oh!

GaelVaroquaux commented Feb 28, 2019

Uh oh!

jnothman 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!

jnothman commented Mar 1, 2019

Uh oh!

codecov bot commented May 31, 2019

Codecov Report

Uh oh!

amueller commented May 31, 2019

Uh oh!

rth commented Jul 12, 2019

Uh oh!

adrinjalali commented Jul 13, 2019

Uh oh!

adrinjalali commented Jul 13, 2019

Uh oh!

thomasjpfan commented Jul 14, 2019

Uh oh!

adrinjalali commented Jul 15, 2019

Uh oh!

thomasjpfan commented Jul 15, 2019

Uh oh!

amueller commented Jul 17, 2019

Uh oh!

adrinjalali commented Jul 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amueller commented Jul 17, 2019

Uh oh!

jnothman commented Sep 17, 2019

Uh oh!

thomasjpfan commented Sep 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnothman commented Sep 20, 2019

Uh oh!

jnothman commented Sep 20, 2019

Uh oh!

thomasjpfan commented Sep 20, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasHug commented Sep 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qinhanmin2014 commented Oct 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomasjpfan commented Oct 4, 2019

Uh oh!

thomasjpfan commented Feb 27, 2019 •

edited

Loading

adrinjalali commented Jul 17, 2019 •

edited

Loading

NicolasHug commented Sep 25, 2019 •

edited

Loading

qinhanmin2014 commented Oct 4, 2019 •

edited

Loading