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

Configured pydocstyle for the trainer, callbacks, loggers, optimizers, and utils. #1089

Merged
merged 21 commits into from
Jun 2, 2022

Conversation

ravi-mosaicml
Copy link
Contributor

@ravi-mosaicml ravi-mosaicml commented May 25, 2022

This PR enables the pydocstyle pre-commit hook for most of the codebase. Docstrings were brought up to compliance so they would pass the pydocstyle check.

  • The main changes involved ensuring summary docstring lines were indeed just one line, that they ended with periods, and that there were no blank lines between the end of the docstring and the start of the function.
  • Added docstrings for missing arguments that were identified.
  • (Coming along for the ride): Remove calls to trainer.engine.close, as now close() is invoked automatically in __del__ as part of Multiple calls to Trainer.fit() #948

Model, dataset, and algorithm docstrings will be addressed in future PRs.

…rs, and utils.

This PR enables the `pydocstyle` pre-commit hook for most of the codebase. Docstrings were brought up to compliance so they would pass the pydocstyle check. Model, dataset, and algorithm docstrings will be addressed in future PRs.
@ravi-mosaicml ravi-mosaicml requested a review from a team as a code owner May 25, 2022 21:52
@eracah
Copy link
Contributor

eracah commented Jun 1, 2022

This PR is obv very large and mostly contains routine docstring edits. I won't look at each one; I will trust that pydoctest does what it claims to do. Any diffs that do other things that we should take special look at in this PR? Like the pre-commit config or anything else?

@ravi-mosaicml
Copy link
Contributor Author

This PR is obv very large and mostly contains routine docstring edits. I won't look at each one; I will trust that pydoctest does what it claims to do. Any diffs that do other things that we should take special look at in this PR? Like the pre-commit config or anything else?

I know it's a bunch of files, but it would be super helpful if you could double check that there aren't any unintentional changes from a sloppy merge that I might have done (e.g. remove new functionality). Let me update it against the latest dev.

Copy link
Contributor

@A-Jacobson A-Jacobson left a comment

Choose a reason for hiding this comment

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

there are a few changes in here that are unrelated to docs. I've flagged them so you can confirm you intend them to be in here.

Also fixed a few typos and made some grammar suggestions. Most are a matter of opinion, you don't have to accept them.

Other than that LGTM, thanks for taking the time to clean this stuff up!

ravi-mosaicml and others added 2 commits June 1, 2022 20:49
Co-authored-by: Austin <A-Jacobson@users.noreply.github.com>
@ravi-mosaicml ravi-mosaicml enabled auto-merge (squash) June 2, 2022 01:00
Copy link
Contributor

@Averylamp Averylamp left a comment

Choose a reason for hiding this comment

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

lgtm, small nits

@ravi-mosaicml ravi-mosaicml merged commit 0685f35 into mosaicml:dev Jun 2, 2022
@ravi-mosaicml ravi-mosaicml deleted the pydocstyle branch June 2, 2022 19:12
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.

4 participants