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

Fixing BUG, get_next_chunk() should use the blocking function device_read() #12584

Merged
merged 8 commits into from
Jan 20, 2023
Merged

Fixing BUG, get_next_chunk() should use the blocking function device_read() #12584

merged 8 commits into from
Jan 20, 2023

Conversation

madsbk
Copy link
Member

@madsbk madsbk commented Jan 20, 2023 •
edited
Loading

datasource_chunk_reader.get_next_chunk() called device_read_async() without waiting on the returned Future.

Should fix the CI fail in #12574

cc. @vuule

Checklist

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

@madsbk madsbk added bug Something isn't working non-breaking Non-breaking change labels Jan 20, 2023
@github-actions github-actions bot added ci libcudf Affects libcudf (C++/CUDA) code. labels Jan 20, 2023
@madsbk madsbk changed the title Fixing BUG, get_next_chunk() to use the blocking function device_read() Fixing BUG, get_next_chunk() should use the blocking function device_read() Jan 20, 2023
@madsbk madsbk force-pushed the get_next_chunk_use_blocking_device_read branch from 53d9d79 to 846a642 Compare January 20, 2023 09:28
wence-
wence- reviewed Jan 20, 2023
View reviewed changes
build.sh Outdated Show resolved Hide resolved
@madsbk madsbk marked this pull request as ready for review January 20, 2023 11:22
@madsbk madsbk requested review from a team as code owners January 20, 2023 11:22
@codecov
Copy link

codecov bot commented Jan 20, 2023 •
edited
Loading

Codecov Report

Base: 86.58% // Head: 85.69% // Decreases project coverage by -0.89% ⚠️

Coverage data is based on head (86c8588) compared to base (b6dccb3).
Patch has no changes to coverable lines.

Additional details and impacted files 
@@               Coverage Diff                @@
##           branch-23.02   #12584      +/-   ##
================================================
- Coverage         86.58%   85.69%   -0.89%     
================================================
  Files               155      155              
  Lines             24368    24858     +490     
================================================
+ Hits              21098    21303     +205     
- Misses             3270     3555     +285
Impacted Files Coverage Δ
python/cudf/cudf/_version.py 1.41% <0.00%> (-98.59%) ⬇️
python/cudf/cudf/core/buffer/spill_manager.py 72.50% <0.00%> (-7.50%) ⬇️
python/cudf/cudf/core/buffer/spillable_buffer.py 91.07% <0.00%> (-1.78%) ⬇️
python/cudf/cudf/utils/dtypes.py 77.77% <0.00%> (-1.69%) ⬇️
python/cudf/cudf/options.py 86.11% <0.00%> (-1.59%) ⬇️
python/cudf/cudf/core/single_column_frame.py 94.30% <0.00%> (-1.27%) ⬇️
...ython/custreamz/custreamz/tests/test_dataframes.py 98.38% <0.00%> (-1.01%) ⬇️
python/dask_cudf/dask_cudf/io/csv.py 96.34% <0.00%> (-1.00%) ⬇️
python/dask_cudf/dask_cudf/io/parquet.py 91.81% <0.00%> (-0.59%) ⬇️
python/cudf/cudf/core/multiindex.py 91.66% <0.00%> (-0.51%) ⬇️
... and 39 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

ajschmidt8
ajschmidt8 reviewed Jan 20, 2023
View reviewed changes
ci/gpu/build.sh
@@ -169,7 +169,7 @@ if [[ -z "$PROJECT_FLASH" || "$PROJECT_FLASH" == "0" ]]; then
done

# Test libcudf (csv, orc, and parquet) with `LIBCUDF_CUFILE_POLICY=KVIKIO`
for test_name in "CSV_TEST" "ORC_TEST" "PARQUET_TEST"; do
for test_name in "CSV_TEST" "ORC_TEST" "PARQUET_TEST" "DATA_CHUNK_SOURCE_TEST"; do
Copy link
Member

@ajschmidt8 ajschmidt8 Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This file is no longer used since we've switched to GitHub Actions.

We will be opening PRs to remove all of these dead files from the repositories after the 23.02 release.

The file below is where we test the C++ now. These changes should be moved to there

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, added it to test_cpp.sh. When #12574 get merged, we don't need this anymore.

ajschmidt8
ajschmidt8 approved these changes Jan 20, 2023
View reviewed changes
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

Approving ops-codeowner file changes

vuule
vuule approved these changes Jan 20, 2023
View reviewed changes
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

Nice catch! Beat me to it :)

@vuule
Copy link
Contributor

vuule commented Jan 20, 2023

/merge

@rapids-bot rapids-bot bot merged commit 90d60cb into rapidsai:branch-23.02 Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wence- wence- wence- left review comments

@vuule vuule vuule approved these changes

@PointKernel PointKernel PointKernel approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@hyperbolic2346 hyperbolic2346 Awaiting requested review from hyperbolic2346 hyperbolic2346 is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

@nvdbaranec nvdbaranec Awaiting requested review from nvdbaranec nvdbaranec is a code owner automatically assigned from rapidsai/cudf-cpp-codeowners

Assignees
No one assigned
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@madsbk @vuule @wence- @ajschmidt8 @PointKernel