Update to mypy 0.971 #11640

wence- · 2022-09-01T16:17:33Z

Description

Update to current mypy, primarily since on Apple silicon, the previous pinned version of mypy is no longer installable (typed-ast does not build from source). This necessitates some minor updates to the type annotation rules, since the newer mypy version is a bit pickier.

While we're here, exclude directories we were previously just ignoring errors in.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

.pre-commit-config.yaml

setup.cfg

python/cudf/cudf/core/dataframe.py

bdice

We'll also want to update the conda environment:

cudf/conda/environments/cudf_dev_cuda11.5.yml

Line 46 in 8ad0290

- mypy=0.782

I expect this may cause compatibility issues and require a broader update of mypy across RAPIDS. Alternatively, we can remove mypy from the conda environment (so it can't conflict with other RAPIDS envs) and solely rely on pre-commit for managing the version and executing the check.

vyasr

This is a surprisingly minimal changeset. I thought the update required more changes than this, but perhaps that was due to unrelated incompatibilities? If CI passes then that's good enough for me.

vyasr · 2022-09-01T16:26:12Z

We'll also want to update the conda environment:

cudf/conda/environments/cudf_dev_cuda11.5.yml

Line 46 in 8ad0290

- mypy=0.782

I expect this may cause compatibility issues and require a broader update of mypy across RAPIDS. Alternatively, we can remove mypy from the conda environment (so it can't conflict with other RAPIDS envs) and solely rely on pre-commit for managing the version and executing the check.

I believe cudf is the only package in RAPIDS that uses mypy at all, so it shouldn't cause any problems (unless there are incompatibilities with dependencies).

Avoids need to explicit-package-bases

setup.cfg

.pre-commit-config.yaml

python/cudf/cudf/core/column/string.py

python/cudf/cudf/core/dataframe.py

python/cudf/cudf/core/index.py

python/cudf/cudf/core/indexed_frame.py

python/cudf/cudf/core/multiindex.py

bdice · 2022-09-02T20:38:01Z

@wence- Before merging, can you please write a PR description describing the motivation for this change and the scope of the changes? (Ideally we would use the PR template checklist, too.)

conda/environments/cudf_dev_cuda11.5.yml

.pre-commit-config.yaml

setup.cfg

wence- · 2022-09-05T11:37:08Z

@wence- Before merging, can you please write a PR description describing the motivation for this change and the scope of the changes? (Ideally we would use the PR template checklist, too.)

Done.

Co-authored-by: Bradley Dice <[email protected]>

wence- · 2022-09-05T11:38:20Z

Final question to address is what we want to do about missing imports in the pre-commit environment.

ajschmidt8

This will also need to be updated in the integration repository below. Can you update the mypy version and add the types-cachetools package to the rapids-build-env package?

bdice · 2022-09-07T14:24:01Z

@ajschmidt8 @wence- @vyasr Longer term, as the integration repo is phased out, we may not want to keep “developer tools” like mypy, flake8, black in the conda environment if they are being managed by pre-commit for a given repo. Users wanting consistency with CI style checks must run mypy through pre-commit and not as a standalone executable from the conda environment. There’s not much reason to keep mypy in the conda environment unless it helps developers’ IDE settings, but the results will not necessarily be consistent with what pre-commit’s mypy expects. The pre-commit environment is ultimately our source of truth for CI and local pre-commit checks.

ajschmidt8 · 2022-09-07T14:48:22Z

@bdice, so pre-commit is also used in CI? If that's the case, then I'm fine with either of the following:

Removing mypy from our integration repository entirely
Updating it to match the version in this PR

Based on your comment, it seems like it never really needed to be in the integration repository packages in the first place, right?

bdice · 2022-09-07T16:20:29Z

@ajschmidt8 We used the integration repo's copy of mypy until #9412. That changed cudf over to using the pre-commit executable (and its managed versions of various linters/formatters) instead of the versions of black, flake8, mypy, etc. from the user's environment (which is the integration environment on CI).

cudf/ci/checks/style.sh

Line 23 in 66b5a0c

pre-commit run --hook-stage manual --all-files

To my knowledge, the only reason we should keep those tools in the cuDF environment is for developers who want IDE integration (usually for VS Code) with black or clang-format, which is harder to achieve when calling via pre-commit because it doesn't have the expected executable in the PATH.

If no developers want it installed in the development environment for purposes of IDE integration, it would be safe to remove from the cuDF conda environment file. Same for other "developer" tools like flake8, black, pydocstyle. I'd think this is unrelated / unaffected by the integration repo so that pinning could probably be deleted entirely if cuDF is the only RAPIDS package that needs it.

ajschmidt8 · 2022-09-07T18:23:41Z

@bdice, got it. we can remove those packages from integration if you'd like. But we have to be prepared to deal with any unexpected fallout if we do. Seems like the easiest path here would be to just continue to keep the versions in-sync until we truly deprecate that repository. But if you're prepared to address any potential fallout, then feel free to open a PR to remove them 🙂.

bdice · 2022-09-07T19:21:54Z

Seems like the easiest path here would be to just continue to keep the versions in-sync until we truly deprecate that repository.

I'm 100% in favor of that path. Just wanted to clarify that the integration mypy is not used by cuDF CI, so that we can revisit and simplify in the future.

vyasr · 2022-09-07T23:41:03Z

I'm fine with the path outlined above. I agree that we can remove linters from the environment in the long run and just relyin on pre-commit, and I'm also happy to let this happen organically as integration is phased out rather than trying to be proactive in simplifying a repo that is going away anyway.

Also, to answer the question about who uses which linters, AFAIK cudf is the only RAPIDS library using mypy and pydocstyle. I think that it is also the only library using black, but there's been enough interest from other libraries that even if nobody has implemented it yet it's likely that other libraries will start using it eventually.

vyasr

Apologies for the delay in re-reviewing this. Looks mostly fine, there are a couple of minor questions I have left then we should be good to go.

.pre-commit-config.yaml

conda/environments/cudf_dev_cuda11.5.yml

python/cudf/cudf/core/dataframe.py

python/cudf/cudf/core/indexed_frame.py

setup.cfg

wence- · 2022-09-08T08:32:45Z

This will also need to be updated in the integration repository below. Can you update the mypy version and add the types-cachetools package to the rapids-build-env package?

Done here: rapidsai/integration#531

Update mypy version in build env in line with rapidsai/cudf#11640

vyasr · 2022-09-09T17:02:22Z

@gpucibot merge

As part of #11640, there was a missing exclusion for `cudf/cudf/utils/metadata/orc_column_statistics_pb2.py`, that causes the following errors: ```bash (cudfdev) pgali@dt07:/nvme/0/pgali/cudf$ git commit -m "address reviews" [WARNING] Unstaged files detected. [INFO] Stashing unstaged files to /home/nfs/pgali/.cache/pre-commit/patch1662993629-63423. isort....................................................................Passed black....................................................................Passed flake8...................................................................Passed mypy.....................................................................Failed - hook id: mypy - exit code: 1 python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:7: error: Library stubs not installed for "google.protobuf.internal" (or incompatible with Python 3.9) python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:7: note: Hint: "python3 -m pip install types-protobuf" python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:7: note: (or run "mypy --install-types" to install all missing stub packages) python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:7: note: See https://mypy.readthedocs.io/en/stable/running_mypy.html#missing-imports python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:8: error: Library stubs not installed for "google.protobuf" (or incompatible with Python 3.9) python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:25: error: Name "_BUCKETSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:26: error: Name "_BUCKETSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:27: error: Name "_INTEGERSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:28: error: Name "_INTEGERSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:29: error: Name "_DOUBLESTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:30: error: Name "_DOUBLESTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:31: error: Name "_STRINGSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:32: error: Name "_STRINGSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:33: error: Name "_BUCKETSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:34: error: Name "_BUCKETSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:35: error: Name "_DECIMALSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:36: error: Name "_DECIMALSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:37: error: Name "_DATESTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:38: error: Name "_DATESTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:39: error: Name "_TIMESTAMPSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:40: error: Name "_TIMESTAMPSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:41: error: Name "_BINARYSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:42: error: Name "_BINARYSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:43: error: Name "_COLUMNSTATISTICS" is not defined python/cudf/cudf/utils/metadata/orc_column_statistics_pb2.py:44: error: Name "_COLUMNSTATISTICS" is not defined Found 22 errors in 1 file (checked 138 source files) pydocstyle...............................................................Passed clang-format.........................................(no files to check)Skipped no-deprecationwarning....................................................Passed cmake-format.........................................(no files to check)Skipped - hook id: cmake-format cmake-lint...........................................(no files to check)Skipped - hook id: cmake-lint copyright-check..........................................................Passed doxygen-check........................................(no files to check)Skipped - hook id: doxygen-check [INFO] Restored changes from /home/nfs/pgali/.cache/pre-commit/patch1662993629-63423. ``` Authors: - GALI PREM SAGAR (https://github.com/galipremsagar) Approvers: - Lawrence Mitchell (https://github.com/wence-) URL: #11685

Update to mypy 0.971

6a31413

wence- requested a review from a team as a code owner September 1, 2022 16:17

wence- requested review from shwina and galipremsagar September 1, 2022 16:17

github-actions bot added the Python Affects Python cuDF API. label Sep 1, 2022

wence- added 3 - Ready for Review Ready for review by team code quality non-breaking Non-breaking change and removed Python Affects Python cuDF API. labels Sep 1, 2022

vyasr added the improvement Improvement / enhancement to an existing function label Sep 1, 2022

bdice reviewed Sep 1, 2022

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

vyasr reviewed Sep 1, 2022

View reviewed changes

setup.cfg Outdated Show resolved Hide resolved

vyasr reviewed Sep 1, 2022

View reviewed changes

python/cudf/cudf/core/dataframe.py Outdated Show resolved Hide resolved

bdice reviewed Sep 1, 2022

View reviewed changes

vyasr reviewed Sep 1, 2022

View reviewed changes

github-actions bot added the Python Affects Python cuDF API. label Sep 1, 2022

Handle NotImplementedType in Python < 3.10

5c0ab18

wence- force-pushed the wence/mypy-update branch from 1e2cc5a to 5c0ab18 Compare September 1, 2022 17:29

Add excludes to mypy call

114b4b1

Avoids need to explicit-package-bases

wence- commented Sep 1, 2022

View reviewed changes

setup.cfg Show resolved Hide resolved

bdice reviewed Sep 1, 2022

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

bdice reviewed Sep 1, 2022

View reviewed changes

python/cudf/cudf/core/column/string.py Show resolved Hide resolved

bdice reviewed Sep 1, 2022

View reviewed changes

python/cudf/cudf/core/dataframe.py Outdated Show resolved Hide resolved

bdice reviewed Sep 1, 2022

View reviewed changes

python/cudf/cudf/core/index.py Show resolved Hide resolved

bdice reviewed Sep 1, 2022

View reviewed changes

python/cudf/cudf/core/indexed_frame.py Outdated Show resolved Hide resolved

bdice reviewed Sep 1, 2022

View reviewed changes

python/cudf/cudf/core/multiindex.py Show resolved Hide resolved

Excludes in config file

d534174

wence- force-pushed the wence/mypy-update branch from 6a2cdd5 to d534174 Compare September 2, 2022 10:04

Some review updates

d0fe054

github-actions bot added the conda label Sep 2, 2022

bdice approved these changes Sep 2, 2022

View reviewed changes

conda/environments/cudf_dev_cuda11.5.yml Outdated Show resolved Hide resolved

.pre-commit-config.yaml Outdated Show resolved Hide resolved

setup.cfg Show resolved Hide resolved

Don't pin types-cachetools

cee96b7

Co-authored-by: Bradley Dice <[email protected]>

wence- mentioned this pull request Sep 7, 2022

[ENH] More type-stubs in the mypy pre-commit environment? #11661

Open

jjacobelli approved these changes Sep 7, 2022

View reviewed changes

ajschmidt8 requested changes Sep 7, 2022

View reviewed changes

vyasr requested changes Sep 7, 2022

View reviewed changes

wence- added 2 commits September 8, 2022 09:23

Factor NotImplementedType out into _typing

1848a42

Fix grammar

b24cc23

wence- added a commit to wence-/rapids-integration that referenced this pull request Sep 8, 2022

Update mypy version in build env in line with rapidsai/cudf#11640

839ba51

Fix version note in comment

ce55a73

vyasr requested a review from ajschmidt8 September 9, 2022 16:59

vyasr approved these changes Sep 9, 2022

View reviewed changes

ajschmidt8 approved these changes Sep 9, 2022

View reviewed changes

ajschmidt8 added a commit to rapidsai/integration that referenced this pull request Sep 9, 2022

Merge pull request #531 from wence-/wence/mypy-update

f0d2f7d

Update mypy version in build env in line with rapidsai/cudf#11640

rapids-bot bot merged commit f485667 into rapidsai:branch-22.10 Sep 9, 2022

wence- deleted the wence/mypy-update branch September 9, 2022 17:03

galipremsagar mentioned this pull request Sep 12, 2022

[REVIEW] Ignore protobuf generated files in mypy checks #11685

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to mypy 0.971 #11640

Update to mypy 0.971 #11640

wence- commented Sep 1, 2022 •

edited

Loading

bdice left a comment •

edited

Loading

vyasr left a comment

vyasr commented Sep 1, 2022

bdice commented Sep 2, 2022

wence- commented Sep 5, 2022

wence- commented Sep 5, 2022

ajschmidt8 left a comment

bdice commented Sep 7, 2022

ajschmidt8 commented Sep 7, 2022

bdice commented Sep 7, 2022 •

edited

Loading

ajschmidt8 commented Sep 7, 2022

bdice commented Sep 7, 2022

vyasr commented Sep 7, 2022 •

edited

Loading

vyasr left a comment

wence- commented Sep 8, 2022

vyasr commented Sep 9, 2022

Update to mypy 0.971 #11640

Update to mypy 0.971 #11640

Conversation

wence- commented Sep 1, 2022 • edited Loading

Description

Checklist

bdice left a comment • edited Loading

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

vyasr commented Sep 1, 2022

bdice commented Sep 2, 2022

wence- commented Sep 5, 2022

wence- commented Sep 5, 2022

ajschmidt8 left a comment

Choose a reason for hiding this comment

bdice commented Sep 7, 2022

ajschmidt8 commented Sep 7, 2022

bdice commented Sep 7, 2022 • edited Loading

ajschmidt8 commented Sep 7, 2022

bdice commented Sep 7, 2022

vyasr commented Sep 7, 2022 • edited Loading

vyasr left a comment

Choose a reason for hiding this comment

wence- commented Sep 8, 2022

vyasr commented Sep 9, 2022

wence- commented Sep 1, 2022 •

edited

Loading

bdice left a comment •

edited

Loading

bdice commented Sep 7, 2022 •

edited

Loading

vyasr commented Sep 7, 2022 •

edited

Loading