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.

Sign up for GitHub

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

Jump to bottom

[REVIEW] Add pytest-xdist to dev environment.yml #6958

Merged
merged 5 commits into from
Dec 16, 2020
Merged

[REVIEW] Add pytest-xdist to dev environment.yml #6958

merged 5 commits into from
Dec 16, 2020

Conversation

galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Dec 9, 2020 •
edited
Loading

Resolves: #6370

This PR enables the parallel execution of pytests of cudf, dask_cudf & custreamz in CI. The changes also include adding pytest-xdist to dev environments.

With these changes, here is the change in pytest execution times in CI:

module without pytest-xdist with pytest-xdist(n=6)
cudf 1 hr 14 min
dask_cudf 4 min 1 min
custreamz 6 min 2 min

Related Integration changes: rapidsai/integration#188

@galipremsagar galipremsagar added 3 - Ready for Review Ready for review by team gpuCI Python Affects Python cuDF API. labels Dec 9, 2020
@galipremsagar galipremsagar self-assigned this Dec 9, 2020
@galipremsagar galipremsagar requested a review from a team as a code owner December 9, 2020 21:53
@galipremsagar galipremsagar added feature request New feature or request non-breaking Non-breaking change labels Dec 9, 2020
@kkraus14
Copy link
Collaborator

kkraus14 commented Dec 9, 2020

@galipremsagar I think we'd need to hook up pytest-xdist into CI in this PR as well: https://github.com/rapidsai/cudf/blob/branch-0.18/ci/gpu/build.sh#L202-L222

@galipremsagar
Copy link
Contributor Author

@galipremsagar I think we'd need to hook up pytest-xdist into CI in this PR as well: https://github.com/rapidsai/cudf/blob/branch-0.18/ci/gpu/build.sh#L202-L222

Thanks @kkraus14, I was under the impression integration repo was the place pytests were being triggered. Added it here aswell.

kkraus14
kkraus14 requested changes Dec 9, 2020
View reviewed changes
ci/gpu/build.sh
@@ -205,11 +205,11 @@ fi

cd $WORKSPACE/python/cudf
gpuci_logger "Python py.test for cuDF"
py.test --cache-clear --basetemp=${WORKSPACE}/cudf-cuda-tmp --junitxml=${WORKSPACE}/junit-cudf.xml -v --cov-config=.coveragerc --cov=cudf --cov-report=xml:${WORKSPACE}/python/cudf/cudf-coverage.xml --cov-report term
py.test -n 6 --cache-clear --basetemp=${WORKSPACE}/cudf-cuda-tmp --junitxml=${WORKSPACE}/junit-cudf.xml -v --cov-config=.coveragerc --cov=cudf --cov-report=xml:${WORKSPACE}/python/cudf/cudf-coverage.xml --cov-report term
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the reasoning behind 6 here? Should we set this programmatically based on something?

Copy link
Contributor Author

@galipremsagar galipremsagar Dec 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

CI test jobs have 8vCPUS, but only set PARALLEL_LEVEL to 4 to have a stable build. But for pytests we can easily use 6 to 7 cores. That was the reason I didn't set it programmatically here, and the reason to not use all 8 cores is to prevent choking of pytest execution.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@rapidsai/ops any guidance / thoughts here? This looks good to go otherwise.

Copy link
Member

Choose a reason for hiding this comment

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

looks good to me.

ci/gpu/build.sh Show resolved Hide resolved
@galipremsagar galipremsagar added DO NOT MERGE Hold off on merging; see PR for details and removed DO NOT MERGE Hold off on merging; see PR for details labels Dec 10, 2020
@galipremsagar
Copy link
Contributor Author

rerun tests

1 similar comment
@galipremsagar
Copy link
Contributor Author

rerun tests

@codecov
Copy link

codecov bot commented Dec 15, 2020 •
edited
Loading

Codecov Report

Merging #6958 (d52b183) into branch-0.18 (f117b68) will increase coverage by 0.42%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #6958      +/-   ##
===============================================
+ Coverage        81.59%   82.02%   +0.42%     
===============================================
  Files               96       96              
  Lines            15905    16340     +435     
===============================================
+ Hits             12978    13403     +425     
- Misses            2927     2937      +10
Impacted Files Coverage Δ
python/cudf/cudf/core/dataframe.py 90.70% <0.00%> (-0.26%) ⬇️
python/cudf/cudf/io/feather.py 100.00% <0.00%> (ø)
python/cudf/cudf/comm/serialize.py 0.00% <0.00%> (ø)
python/cudf/cudf/_fuzz_testing/io.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/_version.py 0.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_csv.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_orc.py 100.00% <0.00%> (ø)
python/dask_cudf/dask_cudf/io/tests/test_json.py 100.00% <0.00%> (ø)
...ython/dask_cudf/dask_cudf/io/tests/test_parquet.py 100.00% <0.00%> (ø)
python/cudf/cudf/utils/applyutils.py 98.74% <0.00%> (+0.02%) ⬆️
... and 37 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 f117b68...d52b183. Read the comment docs.

@galipremsagar galipremsagar added DO NOT MERGE Hold off on merging; see PR for details 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer DO NOT MERGE Hold off on merging; see PR for details labels Dec 15, 2020
@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge 6 - Okay to Auto-Merge and removed 3 - Ready for Review Ready for review by team 4 - Needs cuDF (Python) Reviewer labels Dec 16, 2020
@rapids-bot rapids-bot bot merged commit 8c1f01e into rapidsai:branch-0.18 Dec 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@kkraus14 kkraus14 kkraus14 approved these changes

Assignees

@galipremsagar galipremsagar

Labels
5 - Ready to Merge Testing and reviews complete, ready to merge feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEA] Explore using pytest-xdist in CI
3 participants
@galipremsagar @kkraus14 @ajschmidt8