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 static type checking via Mypy #6381

Merged
merged 127 commits into from
Jan 27, 2021
Merged

Add static type checking via Mypy #6381

merged 127 commits into from
Jan 27, 2021

Conversation

shwina
Copy link
Contributor

@shwina shwina commented Sep 30, 2020

Adds static type checking to cuDF Python via MyPy.

  • An additional mypy style check is enabled in CI
  • mypy is run as part of the pre-commit hook
  • Many parts of the cuDF internal code now have type annotations
  • Any new internal code is expected to be written with type annotations (not public-facing APIs)

@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

@kkraus14 kkraus14 added 2 - In Progress Currently a work in progress Python Affects Python cuDF API. labels Oct 1, 2020
@codecov
Copy link

codecov bot commented Oct 12, 2020

Codecov Report

Merging #6381 (06aef48) into branch-0.18 (8860baf) will increase coverage by 0.08%.
The diff coverage is 100.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #6381      +/-   ##
===============================================
+ Coverage        82.09%   82.17%   +0.08%     
===============================================
  Files               97       99       +2     
  Lines            16474    16805     +331     
===============================================
+ Hits             13524    13810     +286     
- Misses            2950     2995      +45     
Impacted Files Coverage Δ
python/cudf/cudf/_lib/__init__.py 100.00% <ø> (ø)
python/cudf/cudf/_typing.py 91.66% <ø> (ø)
python/cudf/cudf/core/__init__.py 100.00% <ø> (ø)
python/cudf/cudf/core/abc.py 91.48% <ø> (+4.25%) ⬆️
python/cudf/cudf/core/buffer.py 80.00% <ø> (+0.95%) ⬆️
python/cudf/cudf/core/column/__init__.py 100.00% <ø> (ø)
python/cudf/cudf/core/column/categorical.py 92.73% <ø> (-0.62%) ⬇️
python/cudf/cudf/core/column/column.py 87.75% <ø> (-0.39%) ⬇️
python/cudf/cudf/core/column/datetime.py 89.62% <ø> (+0.19%) ⬆️
python/cudf/cudf/core/column/decimal.py 94.87% <ø> (ø)
... and 68 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 6a4c760...06aef48. Read the comment docs.

@shwina shwina changed the base branch from branch-0.16 to branch-0.17 October 12, 2020 21:32
@shwina
Copy link
Contributor Author

shwina commented Oct 12, 2020

rerun tests

@shwina shwina marked this pull request as ready for review January 25, 2021 21:34
@kkraus14 kkraus14 added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Jan 25, 2021
@kkraus14
Copy link
Collaborator

@gpucibot merge

@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Jan 25, 2021
@galipremsagar
Copy link
Contributor

rerun tests

@shwina
Copy link
Contributor Author

shwina commented Jan 26, 2021

Rerun tests

@kkraus14
Copy link
Collaborator

Looks like a flaky test related to the groupby sorting change:

def test_serialize_groupby_external():
        df = cudf.DataFrame()
        df["val"] = np.arange(100, dtype=np.float32)
        gb = df.groupby(cudf.Series(np.random.randint(0, 20, 100)))
        outgb = gb.deserialize(*gb.serialize())
        expect = gb.mean()
        got = outgb.mean()
>       assert_eq(got, expect)

cudf/tests/test_serialize.py:163: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
cudf/tests/utils.py:87: in assert_eq
    tm.assert_frame_equal(left, right, **kwargs)
pandas/_libs/testing.pyx:67: in pandas._libs.testing.assert_almost_equal
    ???
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

>   ???
E   AssertionError: DataFrame.index are different
E   
E   DataFrame.index values are different (10.0 %)
E   [left]:  Int64Index([5, 17, 13, 15, 16, 18, 12, 6, 1, 7, 8, 10, 0, 19, 3, 14, 2, 11, 4,
E               9],
E              dtype='int64')
E   [right]: Int64Index([5, 17, 13, 15, 16, 18, 12, 6, 1, 7, 8, 0, 10, 19, 3, 14, 2, 11, 4,
E               9],
E              dtype='int64')

pandas/_libs/testing.pyx:182: AssertionError

@shwina
Copy link
Contributor Author

shwina commented Jan 26, 2021

Looking into it

Copy link
Contributor

@cwharris cwharris left a comment

Choose a reason for hiding this comment

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

Partial review. This is great. :)

.pre-commit-config.yaml Show resolved Hide resolved
ci/checks/style.sh Show resolved Hide resolved
ci/checks/style.sh Show resolved Hide resolved
conda/environments/cudf_dev_cuda10.1.yml Show resolved Hide resolved
python/cudf/cudf/core/buffer.py Show resolved Hide resolved
python/cudf/cudf/core/buffer.py Outdated Show resolved Hide resolved
@kkraus14
Copy link
Collaborator

@gpucibot merge

@@ -71,7 +71,7 @@ def test_melt(nulls, num_id_vars, num_value_vars, num_rows, dtype):
@pytest.mark.parametrize(
"dtype",
list(NUMERIC_TYPES + DATETIME_TYPES)
+ [pytest.param("str", marks=pytest.mark.xfail())],
+ [pytest.param("str", marks=pytest.mark.xfail())], # type: ignore
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 any reason for tests to provide type annotations? (annotations for test utilities, I can see being helpful, but not limited-scope test code).

@rapids-bot rapids-bot bot merged commit fc40c52 into rapidsai:branch-0.18 Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge 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.

7 participants