[REVIEW] Decorator to generate docstrings with autodetection of parameters #2635

dantegd · 2020-08-03T15:58:19Z

This PR will close the following issues:

closes #1674
closes #2481
closes #2243

It is done via 2 decorators to generate common docstrings in the codebase. It'll greatly reduce the maintenance cost of the most common docstrings and reduce the amount of discrepancies, typos and outdated types in the relatively high number of estimators and methods we have now.

The docstrings are generated at import time, and the impact in import time is in the hundredths of seconds as far as I've measured, which is very much within the margin of error of cuml module import times. For functions that use either of the decorators:

generate_docstring: Meant to be used by fit/predict/et.al methods that have the typical signatures (i.e. fit(x,y) or predict(x)). It detects the parameters and default values and generates the appropriate docstring, with som econfigurable for shapes and formats.
insert_into_docstring: More flexible but less automatic method, meant to be used by functions that use our common dense or sparse datatypes, but have many more custom parameters that are particular to the class(es) as opposed to being common in the codebase. Allows to keep our documentation up to date and correct with minimal changes by keeping our common datatypes concentrated here. NearestNeigbors is a good example of this use case.

Docstrings for the functions look like this:

cuml.dask datatype version of the docstrings will come in a future update.

GPUtester · 2020-08-03T15:58:45Z

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

docs/source/index.rst

…deco2

JohnZed

I like the approach! Haven't reviewed every line in detail, but just a few high level comments - simplifying that decorator would help, and I think the insert_into_docstring could be a little more explicit (see below). generate_docstring already seems to work great.

python/cuml/cluster/kmeans.pyx

python/cuml/common/doc_utils.py

JohnZed · 2020-08-06T19:04:05Z

python/cuml/common/doc_utils.py

+                       parameters=False,
+                       return_values=False):
+    def deco(func):
+        @wraps(func)


phew, this function got a little complex... maybe there is room to break it up?

I added comments to make it more clear. Didn't want to break it into further functions to avoid any potential overheads (even if small)

python/cuml/ensemble/randomforestclassifier.pyx

python/cuml/svm/svr.pyx

…ltinomialNB before (which needs to be skipped)

…deco2

ajschmidt8

I didn't see any ops-codeowner files that were changed in this PR, so I'm assuming we were tagged for a review for awareness for doc builds. is that right, @dantegd? if so, this looks good. let me know if there's anything in particular you want us to look at here.

JohnZed

Michael covered the more substantive stuff... Just some small changes in there. Additionally, this could use some tests (which will be kind of interesting to write!). Otherwise, looks very far along.

python/cuml/svm/svr.pyx

python/cuml/common/doc_utils.py

JohnZed · 2020-08-12T22:09:25Z

python/cuml/common/doc_utils.py

+        @wraps(func)
+        def docstring_wrapper(*args, **kwargs):
+            return func(*args, **kwargs)
+


I'm on board with this suggestion too. Function wrapping adds more runtime complexity, so the __doc__ modification seems desirable.

JohnZed · 2020-08-12T22:10:02Z

python/cuml/common/doc_utils.py

+                )
+
+        if(len(to_add) > 0):
+            docstring_wrapper.__doc__ = str(func.__doc__).format(*to_add)


Good catch.

…deco2

dantegd · 2020-08-19T05:20:51Z

rerun tests

…d pydocs

…docstring

dantegd · 2020-08-19T15:30:28Z

@JohnZed @mdemoret PR is ready for re-review, hopefully merge before code freeze. I opened issue #2714 for the 2 features I haven't had time to implement: automatic indentation detection and named parameters for insert_into_docstring.

The autodetection of indentation is not particularly important at this moment since this PR has 0 sphinx warnings at its current status (except for /home/galahad/miniconda3/envs/ns0812/lib/python3.8/site-packages/numpydoc/docscrape.py:418: UserWarning: Unknown section Output in the docstring of <functools._lru_cache_wrapper object at 0x7f2d7d758c10> in None. which is outside our code). That said I think its a good idea for a future improvement that'll make adding new docstrings more robust and easier.

JohnZed

LGTM except for those two small vestiges of conditional generation that could be removed

JohnZed · 2020-08-19T16:56:53Z

python/cuml/common/doc_utils.py

 from inspect import signature

+# if docs need to be autogeneretad in other environments, add checks here
+try:
+    from IPython import get_ipython


What about interactive python? Or Colab?

Ok, in Colab, get_ipython actually works fine... still I'm not sure about making this conditional. Seems less error-prone to just generate all the time to me.

JohnZed · 2020-08-19T17:29:55Z

docs/source/conf.py

@@ -20,6 +20,8 @@
 import sys
 sys.path.insert(0, os.path.abspath('../../python'))

+os.environ["BUILD_CUML_DOCSTRINGS"] = "True"


Maybe no longer necessary?

JohnZed · 2020-08-19T17:33:56Z

python/cuml/__init__.py

@@ -85,6 +87,10 @@
 from cuml.common.memory_utils import set_global_output_type, using_output_type


+# docstring generation
+
+_generate_pydocstrings = False


Is this still used?

JohnZed

Looks great! I'd like to wait on another look from MD, but good to go from my perspectvie

mdemoret-nv

Everything looks good to me minus a couple small comments and suggestions. For the sorting and prepending suggestions, I can add them to #2714 and push from 0.15.

Looks good.

mdemoret-nv · 2020-08-19T17:53:37Z

python/cuml/common/doc_utils.py

+def generate_docstring(X='dense',
+                       X_shape='(n_samples, n_features)',
+                       y='dense',
+                       y_shape='(n_samples, 1)',
+                       convert_dtype_cast=False,
+                       skip_parameters=[],
+                       skip_parameters_heading=False,
+                       parameters=False,
+                       return_values=False):


I still really wish there was a way to sort the parameters by the order they are listed in the function signature. However, since most of the auto generated parameters will be at the front, maybe we can add a prepend_to_parameters=True argument which would insert items at the front of the parameter list instead of the back? Seems like a quick compromise and will work for 90% of cases.

That's a great idea indeed and if I'm not mistaken it would work for all current instances we have currently, so will quickly add it

mdemoret-nv · 2020-08-19T17:55:19Z

python/cuml/common/doc_utils.py

+        if(('X' in params or 'y' in params or parameters) and not
+                skip_parameters_heading):
+
+            func.__doc__ += \
+                '\nParameters\n----------\n'


Is it possible to just auto detect the parameters heading and only insert it if its not found? Should always be:

Parameters ----------

Was thinking about it and wanted to avoid having to look for the heading in the string for every occurrence of the decorator in the __doc__ string. So I left it to dev control with the skip_parameters_heading, particularly since the (pretty big) majority of use cases don't skip generating the heading, what do you think?

python/cuml/test/test_fit_function.py

FEA New version of docstring decorator with autodetection of parameters

83b3cd7

dantegd added 2 - In Progress Currenty a work in progress doc Documentation Cython / Python Cython or Python issue labels Aug 3, 2020

dantegd requested a review from a team as a code owner August 3, 2020 15:58

dantegd added 4 commits August 4, 2020 13:54

FEA Change all linear_models to use decorator

848eb68

FEA Add capability of decorator to skip generating parameters header

4b1532b

FEA Change cluster estimators to use decorator

5991996

FEA Add new insert_to_docstring decorator and multiple updates

6446a37

dantegd commented Aug 5, 2020

View reviewed changes

docs/source/index.rst Outdated Show resolved Hide resolved

dantegd added 2 commits August 5, 2020 15:46

Merge remote-tracking branch 'original/branch-0.15' into 015-fea-doc-…

97e74c7

…deco2

FEA More improvements and more classes migrated

b362e68

dantegd requested a review from a team as a code owner August 5, 2020 22:58

dantegd added 2 commits August 5, 2020 18:01

FIX Undo temporary changes

9874510

DOC Added entry to changelog

6ec313f

dantegd changed the title ~~[WIP] Decorator to generate docstrings with autodetection of parameters~~ [REVIEW] Decorator to generate docstrings with autodetection of parameters Aug 6, 2020

dantegd added the 3 - Ready for Review Ready for review by team label Aug 6, 2020

JohnZed reviewed Aug 6, 2020

View reviewed changes

FIX Small fixes for test_fit, for some reason it was not detecting mu…

534884f

…ltinomialNB before (which needs to be skipped)

mdemoret-nv mentioned this pull request Aug 7, 2020

[REVIEW] Cleanup sphinx doc warnings for 0.15 #2649

Merged

Merge remote-tracking branch 'original/branch-0.15' into 015-fea-doc-…

05d67c6

…deco2

ajschmidt8 approved these changes Aug 11, 2020

View reviewed changes

dantegd added 3 commits August 12, 2020 08:37

Merge branch-0.15 into 015-fea-doc-deco2

416e22c

FIX Multiple fixes and improvements from PR feedback

de5d5e4

DOC Added further clarification to decorator docstring

6ea94b5

dantegd requested a review from JohnZed August 12, 2020 15:52

dantegd added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 3 - Ready for Review Ready for review by team labels Aug 12, 2020

mdemoret-nv self-requested a review August 12, 2020 17:07

JohnZed requested changes Aug 12, 2020

View reviewed changes

dantegd added 4 - Waiting on Author Waiting for author to respond to review and removed 4 - Waiting on Reviewer Waiting for reviewer to review or respond labels Aug 12, 2020

Merge remote-tracking branch 'original/branch-0.15' into 015-fea-doc-…

2aaf10a

…deco2

dantegd mentioned this pull request Aug 14, 2020

[DOC] Convert all remaining Python docstrings to pydoc and examples to doctest #2415

Closed

dantegd added 3 commits August 19, 2020 00:11

FIX Incorporate multiple fixes from review to remove sphinx warnings

f8cfc61

Merge branch-0.15 into 015-fea-doc-deco2

46f1c04

Merge remote into local

c9c73de

dantegd changed the title ~~[REVIEW] Decorator to generate docstrings with autodetection of parameters [skip ci]~~ [REVIEW] Decorator to generate docstrings with autodetection of parameters Aug 19, 2020

dantegd added 3 commits August 19, 2020 09:55

ENH Add env variable and autodetection of ipython to see when to buil…

86ee107

…d pydocs

ENH Add pickled models to gitignore and autogenerate check to insert …

b815082

…docstring

FIX PEP8 fixes

a788a91

dantegd added 4 - Waiting on Reviewer Waiting for reviewer to review or respond and removed 2 - In Progress Currenty a work in progress 4 - Waiting on Author Waiting for author to respond to review labels Aug 19, 2020

dantegd mentioned this pull request Aug 19, 2020

[FEA] Add named parameters to insert docstring decorator and indentation detection to generate docstring decorator #2714

Open

dantegd added 4 commits August 19, 2020 10:36

FIX DOC Fix example of KMeans

e705acb

FIX Remove check for generating docstrings based on feedback

e3a605f

Merge branch-0.15 into 015-fea-doc-deco2

75e228a

FIX remove accidental line break

02e4c08

JohnZed requested changes Aug 19, 2020

View reviewed changes

dantegd added 2 commits August 19, 2020 12:36

FIX Remove stray variable in init

5b4af21

FIX Remove stray variable in conf.py

2202aa4

JohnZed approved these changes Aug 19, 2020

View reviewed changes

mdemoret-nv approved these changes Aug 19, 2020

View reviewed changes

ENH Add prepend parameters decorator option

0e11828

dantegd merged commit 681d326 into rapidsai:branch-0.15 Aug 19, 2020

dantegd mentioned this pull request Feb 19, 2021

[VERIFY] Have consistent format for accepting targets across cuML algos #3532

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[REVIEW] Decorator to generate docstrings with autodetection of parameters #2635

[REVIEW] Decorator to generate docstrings with autodetection of parameters #2635

dantegd commented Aug 3, 2020 •

edited

Loading

GPUtester commented Aug 3, 2020

JohnZed left a comment

JohnZed Aug 6, 2020

dantegd Aug 12, 2020

ajschmidt8 left a comment

JohnZed left a comment

JohnZed Aug 12, 2020

JohnZed Aug 12, 2020

dantegd commented Aug 19, 2020

dantegd commented Aug 19, 2020

JohnZed left a comment

JohnZed Aug 19, 2020

JohnZed Aug 19, 2020

JohnZed Aug 19, 2020

JohnZed Aug 19, 2020

JohnZed left a comment

mdemoret-nv left a comment

mdemoret-nv Aug 19, 2020

dantegd Aug 19, 2020

mdemoret-nv Aug 19, 2020

dantegd Aug 19, 2020 •

edited

Loading

[REVIEW] Decorator to generate docstrings with autodetection of parameters #2635

[REVIEW] Decorator to generate docstrings with autodetection of parameters #2635

Conversation

dantegd commented Aug 3, 2020 • edited Loading

GPUtester commented Aug 3, 2020

JohnZed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ajschmidt8 left a comment

Choose a reason for hiding this comment

JohnZed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantegd commented Aug 19, 2020

dantegd commented Aug 19, 2020

JohnZed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JohnZed left a comment

Choose a reason for hiding this comment

mdemoret-nv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantegd Aug 19, 2020 • edited Loading

Choose a reason for hiding this comment

dantegd commented Aug 3, 2020 •

edited

Loading

dantegd Aug 19, 2020 •

edited

Loading