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

[REVIEW] Fix codecov in CI #10347

Merged
merged 15 commits into from
Mar 8, 2022
Merged

Conversation

galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Feb 23, 2022

This PR fixes codecov reports generation when used with pytest-xdist.

@galipremsagar galipremsagar added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Feb 23, 2022
@github-actions github-actions bot added the gpuCI label Feb 23, 2022
@codecov
Copy link

codecov bot commented Feb 23, 2022

Codecov Report

Merging #10347 (84ae7a4) into branch-22.04 (a7d88cd) will increase coverage by 75.71%.
The diff coverage is n/a.

❗ Current head 84ae7a4 differs from pull request most recent head cc78c48. Consider uploading reports for the commit cc78c48 to get more accurate results

Impacted file tree graph

@@                Coverage Diff                @@
##           branch-22.04   #10347       +/-   ##
=================================================
+ Coverage         10.42%   86.13%   +75.71%     
=================================================
  Files               119      139       +20     
  Lines             20603    22433     +1830     
=================================================
+ Hits               2148    19323    +17175     
+ Misses            18455     3110    -15345     
Impacted Files Coverage Δ
...ython/custreamz/custreamz/tests/test_dataframes.py 99.39% <0.00%> (-0.01%) ⬇️
python/custreamz/custreamz/_version.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/_version.py 0.00% <0.00%> (ø)
...ython/dask_cudf/dask_cudf/io/tests/test_parquet.py 100.00% <0.00%> (ø)
python/cudf/cudf/core/udf/pipeline.py
...thon/dask_cudf/dask_cudf/tests/test_distributed.py 86.79% <0.00%> (ø)
python/dask_cudf/dask_cudf/tests/test_sort.py 100.00% <0.00%> (ø)
python/cudf/cudf/core/udf/utils.py 98.63% <0.00%> (ø)
python/dask_cudf/dask_cudf/tests/test_onehot.py 100.00% <0.00%> (ø)
...ython/dask_cudf/dask_cudf/tests/test_delayed_io.py 99.03% <0.00%> (ø)
... and 105 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 e610108...cc78c48. Read the comment docs.

@github-actions github-actions bot removed the gpuCI label Feb 24, 2022
@github-actions github-actions bot added the gpuCI label Feb 24, 2022
ci/gpu/build.sh Outdated Show resolved Hide resolved
python/cudf/.coveragerc Outdated Show resolved Hide resolved
@galipremsagar galipremsagar changed the title [WIP] Fix codecov [REVIEW] Fix codecov in CI Mar 7, 2022
@galipremsagar galipremsagar self-assigned this Mar 7, 2022
@galipremsagar galipremsagar marked this pull request as ready for review March 7, 2022 21:32
@galipremsagar galipremsagar requested review from a team as code owners March 7, 2022 21:32
Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

This looks great to me! It may also be nice at some point to enable the cython.Coverage plugin so that we can get coverage results for our Cython code, but the pending switch to scikit-build will bork that anyway so let's not bother with enabling coverage until we can improve scikit-build/coverage compatibility.

ci/gpu/build.sh Outdated
@@ -239,9 +239,10 @@ fi
# TEST - Run py.test, notebooks
################################################################################

cd "$WORKSPACE/python/cudf"
cd "$WORKSPACE/python/cudf/cudf"
# It is essential to cd into $WORKSPACE/python/cudf/cudf as `pytest-xdist` + `codecov` seem to work only at this directory level.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, coverage.py is extremely sensitive to file paths unfortunately.

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Are any changes needed to measure coverage for dask-cudf and custreamz, or are those already working? Let's try to align coverage for all the Python package tests in this PR if possible.

ci/gpu/build.sh Outdated Show resolved Hide resolved
source = cudf
omit = cudf/tests/*
Copy link
Contributor

@bdice bdice Mar 7, 2022

Choose a reason for hiding this comment

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

Do we really want to omit our tests? Unused functions in our tests should be flagged as "not covered."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know why it was omitted previously, but I'll enable it.

Co-authored-by: Bradley Dice <[email protected]>
@galipremsagar
Copy link
Contributor Author

Are any changes needed to measure coverage for dask-cudf and custreamz, or are those already working?

Yea, they seem to be already working.

@bdice
Copy link
Contributor

bdice commented Mar 7, 2022

For this PR, I see https://github.com/rapidsai/cudf/blob/branch-22.04/python/dask_cudf/.coveragerc is skipping coverage for dask_cudf tests. I would recommend the same change we made for cudf, don't ignore coverage in test files.

For a follow-up, it looks like cudf/_fuzz_testing is not covered (not executed?) in our CI. Should we be running the fuzz testing code (very briefly?) to ensure it works and gives us a coverage measurement during our CI runs? I am not familiar with this part of cudf so feel free to ignore if I'm missing something here (e.g. if the code is not stable or can't be executed on CI).

python/dask_cudf/.coveragerc Outdated Show resolved Hide resolved
@galipremsagar galipremsagar requested a review from a team as a code owner March 7, 2022 22:22
@galipremsagar
Copy link
Contributor Author

For this PR, I see https://github.com/rapidsai/cudf/blob/branch-22.04/python/dask_cudf/.coveragerc is skipping coverage for dask_cudf tests. I would recommend the same change we made for cudf, don't ignore coverage in test files.

Made the changes.

For a follow-up, it looks like cudf/_fuzz_testing is not covered (not executed?) in our CI. Should we be running the fuzz testing code (very briefly?) to ensure it works and gives us a coverage measurement during our CI runs? I am not familiar with this part of cudf so feel free to ignore if I'm missing something here (e.g. if the code is not stable or can't be executed on CI).

Those are very long-running tests as they target at the machine limits dataset sizes. Hence they are not intended to be run per PR. So we are planning on running them on benchmark machines soon.

@bdice
Copy link
Contributor

bdice commented Mar 7, 2022

Those are very long-running tests as they target at the machine limits dataset sizes. Hence they are not intended to be run per PR. So we are planning on running them on benchmark machines soon.

Right, I know fuzz testing is typically large-scale. I was wondering if there are options to use "toy" dataset sizes that can run quickly, to permit CI testing that ensures the code works and has reasonable coverage.

edit: A related problem is that we don't build or run libcudf benchmarks in CI, and sometimes PRs are merged that introduce problems in the benchmarks because we don't build them in CI (even if they didn't execute, proving they compile would be beneficial). Being able to run "CI-friendly" versions of long-running code could be helpful. Regardless, it's out of scope for this PR. Very nice job on this. 👍

@galipremsagar
Copy link
Contributor Author

Those are very long-running tests as they target at the machine limits dataset sizes. Hence they are not intended to be run per PR. So we are planning on running them on benchmark machines soon.

Right, I know fuzz testing is typically large-scale. I was wondering if there are options to use "toy" dataset sizes that can run quickly, to permit CI testing that ensures the code works and has reasonable coverage.

Yeah, I think that is reasonable and possible, I'll try this in a follow-up PR.

@galipremsagar galipremsagar added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer labels Mar 8, 2022
@galipremsagar
Copy link
Contributor Author

@gpucibot merge

1 similar comment
@quasiben
Copy link
Member

quasiben commented Mar 8, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit a9b6cb1 into rapidsai:branch-22.04 Mar 8, 2022
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants