[REVIEW] Converting all Estimator Constructors to Keyword Arguments

[mdemoret-nv]

Closes #3373

This mimics sklearn's positional argument deprecation message (and is named differently to prevent mixups) but is automatically added to any class that inherits from cuml.Base (similar to other CumlArray decorators that are automatically added to Base class). When the descriptor is automatically added, it assumes all arguments should be keyword-only (to catch accidentally forgetting to specify keyword only arguments). This can easily be overridden by manually adding the decorator if some arguments need to be positional and some keyword. For example, the OneVsRestClassifier estimator takes a positional estimator to wrap so the __init__ function looks like:

@_deprecate_pos_args(version="0.20")
def __init__(self,
             estimator,
             *args,
             handle=None,
             verbose=False,
             output_type=None):

Notes:

1. This only updates __init__ functions but could be applied to any of the public functions in the API.
2. No tests were added because validation is performed on module load and will generate an assert error if developers have used it incorrectly

[JohnZed]

Looks very cool! I had only minor questions and comments, can wrap up quickly. If this is ready to go, can you remove the WIP label?

[JohnZed]

Can you benchmark a class initialization with and without this change? (just %timeit x = Foo() in Ipython) I think it'll be very small but just want to make sure there is not a meaningful cost to the tricks.

[mdemoret-nv]

Here are the numbers. Before this PR:

>>> timeit.repeat('cuml.DBSCAN()', 'import cuml', number=100000)
[1.6759078549985134, 0.41760462100137374, 0.4198731740034418, 0.4204189170013706, 0.4161926310007402]

After this PR:

>>> timeit.repeat('cuml.DBSCAN()', 'import cuml', number=100000)
[2.557721577999473, 0.7288761550007621, 0.7315405129993451, 0.7273198049988423, 0.7297004659994855]

I was surprised that the impact would be so big (75% longer) but at the same time this is only adding 3 usec per instantiation which is incredibly small. I'm not sure what we could change in the function to make it run faster.

[dantegd]

Just had a quick comment

[codecov-io]

Codecov Report

Merging #3636 (8c110a0) into branch-0.19 (fd9ec89) will increase coverage by 1.67%.
The diff coverage is 86.22%.

@@               Coverage Diff               @@
##           branch-0.19    #3636      +/-   ##
===============================================
+ Coverage        80.70%   82.38%   +1.67%     
===============================================
  Files              227      227              
  Lines            17615    17652      +37     
===============================================
+ Hits             14217    14542     +325     
+ Misses            3398     3110     -288     

Flag	Coverage Δ
dask	46.45% <0.59%> (+1.46%)	⬆️
non-dask	74.40% <86.22%> (+1.49%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...cuml/_thirdparty/sklearn/preprocessing/__init__.py	100.00% <ø> (ø)
...l/_thirdparty/sklearn/preprocessing/_imputation.py	64.54% <ø> (+1.74%)	⬆️
python/cuml/_thirdparty/sklearn/utils/extmath.py	56.89% <ø> (ø)
...cuml/_thirdparty/sklearn/utils/skl_dependencies.py	80.00% <ø> (+26.07%)	⬆️
...ython/cuml/_thirdparty/sklearn/utils/validation.py	18.41% <ø> (-4.04%)	⬇️
python/cuml/cluster/__init__.py	100.00% <ø> (ø)
python/cuml/cluster/agglomerative.pyx	96.47% <ø> (ø)
python/cuml/cluster/dbscan.pyx	98.23% <ø> (-1.77%)	⬇️
python/cuml/cluster/dbscan_mg.pyx	100.00% <ø> (ø)
python/cuml/cluster/kmeans.pyx	92.03% <ø> (+0.07%)	⬆️
... and 169 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 668aea1...8c110a0. Read the comment docs.

[mdemoret-nv]

@dantegd and @JohnZed I've incorporated your feedback into this PR and merged upstream. However, with the changes to the BaseEstimator class in preprocessing, the number of updates I had to make was pretty large. Probably warrants a quick re-review to make sure I didnt miss anything.

To address both of your comments about the local import, I've moved the _deprecate_pos_args class to cuml.internal. So this local import isnt needed anymore.

[JohnZed]

Looks good to me. I like the new namings and appreciate the move to internals. I'll wait a bit before merging in case @dantegd wants to take a look at the changes

[JohnZed]

@gpucibot merge