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

Add support for pydocstyle and test on abc.py #7985

Merged
merged 8 commits into from
Apr 29, 2021

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Apr 16, 2021

This PR is related to #202 and adds support for using pydocstyle to lint docstrings. It adds flake8 configuration, a pre-commit hook, and sets up the flake8 config to enable incremental application of docstring styling. As an added bonus, because of the way that pydocstyle chooses not to lint docstrings on internal objects (those prefixed with an underscore), as more files are linted we can evaluate what constitutes public API for cudf. Once a file has been linted and is added to the regex filter in the config, ongoing CI checks will prevent regressions in the documentation.

As an initial test, this PR lints the abc.py file containing the Serializable class (and updates the docstrings accordingly). We may want to mark that entire class as internal in any case, but it's important enough that it needs to be documented in any case (and perhaps we could settle on a standard docstring style for internal-facing code as well while we're at it).

@vyasr vyasr self-assigned this Apr 16, 2021
@github-actions github-actions bot added Python Affects Python cuDF API. gpuCI labels Apr 16, 2021
@vyasr vyasr added 2 - In Progress Currently a work in progress Python Affects Python cuDF API. gpuCI improvement Improvement / enhancement to an existing function non-breaking Non-breaking change and removed Python Affects Python cuDF API. gpuCI labels Apr 16, 2021
@vyasr
Copy link
Contributor Author

vyasr commented Apr 16, 2021

@kkraus14 @galipremsagar would appreciate thoughts here before I go any further. I started this because I was rewriting docstrings anyway in the process of other work being done in some of my recent PRs before I found #202. A wholesale update of our docstrings is a big task and too much time investment to be worth prioritizing, but I think following this approach we can improve docs incrementally without dedicating too much time.

I assume that ops will need to update gpuCI containers to include pydocstyle before the style check will pass, that's something I can request from them if we want to move forward.

@vyasr
Copy link
Contributor Author

vyasr commented Apr 22, 2021

@kkraus14 mentioned that it would be great if we could reduce duplication between docstrings and type annotations. The main related tool I was previously aware was darglint, which is a linter that validates consistency between docstring types and actually accepted arguments. It works well, but has some limitations I've run into that prevented me from adopting it for previous projects and it can be a bit slow. numpydoc doesn't have this yet, but we could push for or contribute to making it happen in the longer term if we think it's worthwhile. Looks like napoleon has actually implemented support for docstrings where types are pulled from annotations instead of being required in the docstring, but I don't know if we'd be interested in switching from numpydoc to napoleon. Note that this wouldn't require any change to our docstring style since napoleon supports both Google and numpy styles, and I've had good experiences with it in the past, but of course that's no guarantee that it'll work great for us here. There's this Sphinx plugin, which seems to work with napoleon for this purpose, and I'm not sure exactly how these two interact right now. The READMEs seem a bit out of date, but they're actively developed and I could test them.

I don't think any of this should block us from moving forward with adding docstring validation, but it's something we should keep in mind to reduce duplication in the future.

@vyasr
Copy link
Contributor Author

vyasr commented Apr 27, 2021

ok to test

@vyasr vyasr marked this pull request as ready for review April 27, 2021 16:57
@vyasr vyasr requested review from a team as code owners April 27, 2021 16:57
@vyasr vyasr requested review from cwharris and shwina April 27, 2021 16:57
@vyasr
Copy link
Contributor Author

vyasr commented Apr 27, 2021

This PR is ready for initial review. rapidsai/integration#258 was just merged, so it may take a while for CI to pick up the new images with pydocstyle installed, but pydocstyle passes locally (as do all other style checks) so review can move forward and I'll retrigger tests as needed.

@cwharris
Copy link
Contributor

cwharris commented Apr 27, 2021

Looks like pydocstyle isn't in the path, or isn't installed correctly. Probably need to add it to the gdf conda env?

ci/checks/style.sh: line 37: pydocstyle: command not found

@vyasr
Copy link
Contributor Author

vyasr commented Apr 27, 2021

I added it to the rapids integration metapackage and that change got merged, so I think we'll just have to wait for that conda package to get built. @ajschmidt8 may have a better idea of when that might be.

For reference, I think the relevant conda package is here and we should be able to see when it's been updated (last update is from 5 days ago, which is before pydocstyle was added).

@ajschmidt8
Copy link
Member

I added it to the rapids integration metapackage and that change got merged, so I think we'll just have to wait for that conda package to get built. @ajschmidt8 may have a better idea of when that might be.

For reference, I think the relevant conda package is here and we should be able to see when it's been updated (last update is from 5 days ago, which is before pydocstyle was added).

The nightly conda package location is here: https://anaconda.org/rapidsai-nightly/rapids-build-env

There was an issue with CI that I just fixed that prevented the package from getting re-deployed after your changes. Everything is getting re-built and re-deployed now. After the package is updated, the Docker container that installs that package will also have to be updated and re-deployed (this one). After both of those are complete, this PR should have pydocstyle included.

@vyasr
Copy link
Contributor Author

vyasr commented Apr 27, 2021

rerun tests

@vyasr
Copy link
Contributor Author

vyasr commented Apr 27, 2021

@cwharris CI images are updated and the style check passes now.

@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #7985 (d6a4777) into branch-0.20 (51336df) will decrease coverage by 0.03%.
The diff coverage is 85.26%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.20    #7985      +/-   ##
===============================================
- Coverage        82.88%   82.85%   -0.04%     
===============================================
  Files              103      103              
  Lines            17668    17818     +150     
===============================================
+ Hits             14645    14763     +118     
- Misses            3023     3055      +32     
Impacted Files Coverage Δ
python/cudf/cudf/core/column/__init__.py 100.00% <ø> (ø)
python/cudf/cudf/io/orc.py 86.89% <ø> (ø)
python/cudf/cudf/utils/cudautils.py 57.75% <25.00%> (ø)
python/cudf/cudf/utils/dtypes.py 81.87% <41.66%> (-1.57%) ⬇️
python/cudf/cudf/core/column/lists.py 86.98% <66.66%> (-0.43%) ⬇️
python/cudf/cudf/core/column/struct.py 94.73% <66.66%> (-1.56%) ⬇️
python/cudf/cudf/core/column/numerical.py 94.43% <72.72%> (ø)
python/cudf/cudf/core/tools/datetimes.py 80.42% <75.29%> (-4.11%) ⬇️
python/cudf/cudf/core/groupby/groupby.py 91.55% <76.92%> (+0.11%) ⬆️
python/cudf/cudf/core/column/column.py 88.64% <77.77%> (ø)
... and 29 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 4d359c2...d6a4777. Read the comment docs.

@vyasr vyasr added 3 - Ready for Review Ready for review by team Python Affects Python cuDF API. and removed Python Affects Python cuDF API. 2 - In Progress Currently a work in progress labels Apr 28, 2021
@kkraus14
Copy link
Collaborator

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 7f0ad1d into rapidsai:branch-0.20 Apr 29, 2021
@vyasr vyasr added this to the cuDF Python Refactoring milestone Jul 22, 2021
@vyasr vyasr deleted the prototype_pydocstyle branch January 14, 2022 18:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants