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] enable per-thread default stream in gpu ci #5673

Merged
merged 2 commits into from
Jul 13, 2020

Conversation

rongou
Copy link
Contributor

@rongou rongou commented Jul 10, 2020

Always build and test with per-thread default stream enabled, as discussed in #5614.

@harrism @jrhemstad @kkraus14

@rongou rongou requested a review from a team as a code owner July 10, 2020 23:56
@GPUtester
Copy link
Collaborator

Can one of the admins verify this patch?

@GPUtester
Copy link
Collaborator

Please update the changelog in order to start CI tests.

View the gpuCI docs here.

Comment on lines 94 to 98
if [[ ${BUILD_MODE} == "pull-request" ]]; then
$WORKSPACE/build.sh clean libcudf cudf dask_cudf benchmarks tests
$WORKSPACE/build.sh clean libcudf cudf dask_cudf benchmarks tests --ptds
else
$WORKSPACE/build.sh clean libcudf cudf dask_cudf benchmarks tests -l
$WORKSPACE/build.sh clean libcudf cudf dask_cudf benchmarks tests -l --ptds
fi
Copy link
Collaborator

Choose a reason for hiding this comment

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

@dillon-cullinan I assume the build mode is "pull-request" when CI is running against a PR. What is the else clause here used?

Copy link
Contributor

Choose a reason for hiding this comment

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

For branch builds (merge commits + nightlies), we make sure to build the legacy code as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. Legacy code is gone so we can likely remove that option, but seems like this is good to go then. Thanks!

@codecov
Copy link

codecov bot commented Jul 11, 2020

Codecov Report

Merging #5673 into branch-0.15 will increase coverage by 1.65%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.15    #5673      +/-   ##
===============================================
+ Coverage        86.16%   87.82%   +1.65%     
===============================================
  Files               72       72              
  Lines            12485    15477    +2992     
===============================================
+ Hits             10758    13592    +2834     
- Misses            1727     1885     +158     
Impacted Files Coverage Δ
python/cudf/cudf/core/multiindex.py 80.63% <0.00%> (-0.04%) ⬇️
python/cudf/cudf/utils/dtypes.py 87.58% <0.00%> (+0.16%) ⬆️
python/cudf/cudf/core/column/string.py 86.68% <0.00%> (+0.18%) ⬆️
python/cudf/cudf/core/column/categorical.py 94.22% <0.00%> (+0.23%) ⬆️
python/cudf/cudf/core/series.py 91.62% <0.00%> (+0.39%) ⬆️
python/cudf/cudf/core/frame.py 92.64% <0.00%> (+1.20%) ⬆️
python/cudf/cudf/core/index.py 92.58% <0.00%> (+1.51%) ⬆️
python/cudf/cudf/core/reshape.py 89.93% <0.00%> (+1.99%) ⬆️
python/cudf/cudf/core/dataframe.py 92.26% <0.00%> (+3.07%) ⬆️

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 542e4e4...9b8b462. Read the comment docs.

@kkraus14 kkraus14 added 3 - Ready for Review Ready for review by team gpuCI labels Jul 13, 2020
@kkraus14 kkraus14 added 5 - Merge After Dependencies and removed 3 - Ready for Review Ready for review by team labels Jul 13, 2020
@kkraus14
Copy link
Collaborator

Waiting to merge until tests have been run with RMM with the new PTDS enabled

Copy link
Collaborator

@kkraus14 kkraus14 left a comment

Choose a reason for hiding this comment

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

Blocking until testing with the new RMM with PTDS is done in CI

@kkraus14
Copy link
Collaborator

rerun tests

@kkraus14 kkraus14 added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 5 - Merge After Dependencies labels Jul 13, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants