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

Docstring formatting and linting using pydocstyle and darglint #2670

Merged
merged 81 commits into from
Sep 14, 2021

Conversation

angela97lin
Copy link
Contributor

@angela97lin angela97lin commented Aug 23, 2021

Closes #878

I introduced two packages in this PR: pydocstyle and darglint.

Some noteable changes via pydocstyle:

  • Catches missing arguments: D417: Missing argument descriptions in the docstring (argument(s) X are missing descriptions in 'validate' docstring)--unfortunately, this doesn't always catch everything :(
  • Consistent spacing between sections of multi-line docstrings
  • Consistent punctuation (ex: must end in period for description / summary), unfortunately not for arguments :'(
  • Use "Args" instead of "Arguments"; I think before we tried to conform to "Arguments" but since "Args" is more Google-style (https://google.github.io/styleguide/pyguide.html) and recognized as a valid section header, updating to use Args.
    If there's any formatting that we don't like, we can choose to ignore it, if there's any formatting we want to include on top of the Google style, we can add it! I chose to add D400 because it felt more consistent :)

Pydoc error codes: http://www.pydocstyle.org/en/5.0.1/error_codes.html


Noteable changes via darglint:

  • Detects when "Returns" is missing for multiline docstrings or misformatted.
  • Detects missing arguments even if misformatted sections.
  • Requires "Raises" with the appropriate exception if the method raises an exception.
  • If method does not return, delete Returns section rather than use Returns: None.
  • Detects excess docstrings (when parameters are deleted but docstrings are not)

** However, a strong con for darglint is that it takes a noticeable amount of time (1-2 minutes?)--this increases our lint job from ~2 minutes to ~5-6 minutes on github. **
I wonder if adding this as a pre-commit hook could help with this time, but could be worth looking into as well.


I found https://pypi.org/project/docformatter/ as an automatic docformatter but in practice, it didn't work extremely well--for example, it likes to break docstrings up by character count by Google convention is that the first line of a docstring must fit in one line. Given that this PR makes the adjustments necessary for the repo at large, future PRs should only need to worry about what code lines have been modified, which is not as big of a lift.

@angela97lin angela97lin self-assigned this Aug 23, 2021
@codecov
Copy link

codecov bot commented Aug 23, 2021

Codecov Report

Merging #2670 (8f714bf) into main (b239dbc) will not change coverage.
The diff coverage is 100.0%.

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #2670   +/-   ##
=====================================
  Coverage   99.8%   99.8%           
=====================================
  Files        301     301           
  Lines      27904   27904           
=====================================
  Hits       27841   27841           
  Misses        63      63           
Impacted Files Coverage Δ
evalml/__init__.py 100.0% <ø> (ø)
evalml/__main__.py 100.0% <ø> (ø)
evalml/automl/__init__.py 100.0% <ø> (ø)
evalml/automl/automl_algorithm/__init__.py 100.0% <ø> (ø)
evalml/automl/automl_algorithm/automl_algorithm.py 100.0% <ø> (ø)
...valml/automl/automl_algorithm/default_algorithm.py 100.0% <ø> (ø)
...lml/automl/automl_algorithm/iterative_algorithm.py 100.0% <ø> (ø)
evalml/automl/automl_search.py 99.9% <ø> (ø)
evalml/automl/callbacks.py 100.0% <ø> (ø)
evalml/automl/engine/__init__.py 100.0% <ø> (ø)
... and 171 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 b239dbc...8f714bf. Read the comment docs.

@freddyaboulton
Copy link
Contributor

Can't wait for this pr to be ready 🥳

@angela97lin angela97lin changed the title Docstring formatting Docstring formatting and linting using pydocstyle Aug 30, 2021
@@ -11,6 +11,7 @@ exclude = docs/*
ignore = E501,W504,W503
per-file-ignores =
**/__init__.py:F401
**/tests/*:D
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What this means: Ignore all docstring linting errors for test files

skip=__init__.py
[darglint]
ignore=DAR402
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DAR402: When a docstring describes an exception not explicitly raised.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also skip 401? Looks like darglint only checks if the current method raises an exception and not any methods called within that method.

Just starting a discussion. I don't think we need to resolve this to merge but curious what everyone thinks.

def _raise_if_a_is_odd(a):
    """Raises a Value error if a is odd.

    Raises:
        ValueError if a is odd
    """
    if a % 2 == 1:
        raise ValueError("A is odd!")


def my_func(a, b):
    """Does my_func.

    Args:
        a (int): A number.
        b (int): B number.

    Returns:
        Sum of a and b
    """
    _raise_if_a_is_odd(a)
    return a + b

@angela97lin angela97lin marked this pull request as ready for review September 13, 2021 02:54
Copy link
Contributor

@chukarsten chukarsten left a comment

Choose a reason for hiding this comment

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

I feel super vindicated by the reintroduction of "Raises". That was the hill I was ready to die on. I definitely feel able to accept the risk of a longer lint time on the server CI. I don't think that's a huge deal and is particularly mitigated by the pre-commit hook you linked to. I also feel like there's gotta be a way to do the lint and lint fix locally but with just a diff of files between the current commit and HEAD to minimize local lint time. But we can try it and see whether we get irritated. Great work!!!

Copy link
Contributor

@ParthivNaresh ParthivNaresh left a comment

Choose a reason for hiding this comment

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

Now that was a workout. Excellent work!

@@ -10,7 +11,7 @@
class SklearnStackedEnsembleBase(Estimator):
"""Stacked Ensemble Base Class.

Arguments:
Args:
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there supposed to be a "Raises" part of the docstring here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, though I think these linting packages aren't smart enough to pick up that a class docstring should be associated with the init docstring 😅

"""Prints information about the system, evalml, and dependencies of evalml.

Returns:
None
Copy link
Contributor

Choose a reason for hiding this comment

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

Good riddance of the returns

Copy link
Contributor

@freddyaboulton freddyaboulton left a comment

Choose a reason for hiding this comment

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

@angela97lin Thank you for doing this! This is awesome. I'm glad we're making sure that all of our functions/methods have docstrings and that all of the arguments are properly formatted.

I am ok with the increase in the lint job time. Locally, I tend to run that only once before I push up and I don't think we'd notice on GH since the other jobs run longer than lint.

I left a couple of questions about the changes in this PR, like some docstrings got condensed into one line and None got removed from the type list for some arguments.

What style choices are not captured by pydocstyle/darglint that we'll want to still keep an eye out during PR review? I have the following:

  • First letter of argument description is capitalized.
  • Docstring sentences end in periods.
  • Types are in lower-case bool vs Bool and dict vs Dict.

Let's add these and any others to our contributing guide?

skip=__init__.py
[darglint]
ignore=DAR402
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also skip 401? Looks like darglint only checks if the current method raises an exception and not any methods called within that method.

Just starting a discussion. I don't think we need to resolve this to merge but curious what everyone thinks.

def _raise_if_a_is_odd(a):
    """Raises a Value error if a is odd.

    Raises:
        ValueError if a is odd
    """
    if a % 2 == 1:
        raise ValueError("A is odd!")


def my_func(a, b):
    """Does my_func.

    Args:
        a (int): A number.
        b (int): B number.

    Returns:
        Sum of a and b
    """
    _raise_if_a_is_odd(a)
    return a + b

@@ -1042,8 +1045,7 @@ def search(self, show_iteration_plot=True):
self._searched = True

def _find_best_pipeline(self):
"""Finds the best pipeline in the rankings
If self._best_pipeline already exists, check to make sure it is different from the current best pipeline before training and thresholding"""
"""Finds the best pipeline in the rankings If self._best_pipeline already exists, check to make sure it is different from the current best pipeline before training and thresholding."""
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

pickle_protocol (int): the pickle data stream format.
Args:
file_path (str): Location to save file.
pickle_type ({"pickle", "cloudpickle"}): The pickling library to use.
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting that ("pickle", "cloudpickle") passes the darglint check.


Returns:
Dict[str, Dict[str, float]]: Dictionary keyed by pipeline name that maps to a dictionary of scores.
dict[str, Dict[str, float]]: Dictionary keyed by pipeline name that maps to a dictionary of scores.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think Dict raises an error with flake8/pydocstyle?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, you're correct! Just a consistency thing--we could choose either caps or not :)

@@ -132,36 +129,37 @@ def submit_evaluation_job(self, automl_config, pipeline, X, y) -> EngineComputat
)
return CFComputation(future)

def submit_training_job(self, automl_config, pipeline, X, y) -> EngineComputation:
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

provides a seventh order Taylor series approximation to the two true
functional relationships, and was estimated using least squares
regression.
"""Calculate the probability that there are no true outliers in a numeric (integer or float) column. It is based on creating 100,000 samples consisting of a given number of records, and then repeating this over a grid of sample sizes. Each value in a sample is drawn from a log normal distribution, and then the number of potential outliers in the data is determined using the skew adjusted box plot approach based on the medcouple statistic. It was observed that the distribution of the percentage of outliers could be described by a gamma distribution, with the shape and scale parameters changing with the sample size. For each sample size, the shape and scale parameters of the gamma distriubtion were estimated using maximum likelihood methods. The set of estimate shape and scale parameters for different sample size were then used to fit equations that relate these two parameters to the sample size. These equations use a transendental logrithmic functional form that provides a seventh order Taylor series approximation to the two true functional relationships, and was estimated using least squares regression.
Copy link
Contributor

Choose a reason for hiding this comment

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

Also @eccabay, private methods/functions are not shown in the docs.


Raises:
ValueError: if any pipeline names are duplicated.
ValueError: If any pipeline names are duplicated.
Copy link
Contributor

Choose a reason for hiding this comment

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

The capital If is because we care about starting with a capital letter. darglint/pydocstyle don't care right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup yup 😅 Definitely more of a stylistic thing, but I think it looks a tad bit cleaner!

y (pd.Series): Future target of shape [n_samples].
X_train (pd.DataFrame): Data the pipeline was trained on of shape [n_samples_train, n_feautures].
y_train (pd.Series): Targets used to train the pipeline of shape [n_samples_train].
objective (ObjectiveBase, str): Objective used to threshold predicted probabilities, optional.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did you get rid of None? The parameter can be None in this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This could be a stylistic thing but following Google's example, it seemed like even if None was an acceptable parameter, it wasn't listed as one. Instead, they used "optional" and the "Defaults as None" to indicate that!

@freddyaboulton
Copy link
Contributor

@chukarsten @angela97lin Looks like scikit-learn lints only on the diff but it looks kinda hairy. Maybe it's cause I don't know how to bash script lol

https://github.com/scikit-learn/scikit-learn/blob/main/build_tools/circle/linting.sh

@angela97lin
Copy link
Contributor Author

angela97lin commented Sep 14, 2021

@freddyaboulton Really cool, I had no idea! I filed #2779 with a link to your comment 😁

Also, thanks for the suggestions! I think another stylistic thing is adding the default value to the argument string. Updated the contributing guide with all of these: https://github.com/alteryx/evalml/blob/878_docstring_formatting/contributing.md#code-style-guide

@angela97lin angela97lin merged commit 2719871 into main Sep 14, 2021
@angela97lin angela97lin deleted the 878_docstring_formatting branch September 14, 2021 20:28
@chukarsten chukarsten mentioned this pull request Sep 15, 2021
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.

API ref: Consistent Docstring formatting
5 participants