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

Reenable stream identification library in CI #12714

Merged

Conversation

vyasr
Copy link
Contributor

@vyasr vyasr commented Feb 7, 2023

Description

This PR reenables the preload library introduced for verifying stream usage in libcudf in #11875. This library was disabled during the GitHub Actions migration.

Checklist

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

@vyasr vyasr added feature request New feature or request 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Feb 7, 2023
@vyasr vyasr self-assigned this Feb 7, 2023
@github-actions github-actions bot added ci CMake CMake build issue labels Feb 7, 2023
@github-actions github-actions bot added the Python Affects Python cuDF API. label Feb 7, 2023
@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

❗ No coverage uploaded for pull request base (branch-23.04@c931d5a). Click here to learn what that means.
Patch has no changes to coverable lines.

❗ Current head 352a93c differs from pull request most recent head 176e55b. Consider uploading reports for the commit 176e55b to get more accurate results

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-23.04   #12714   +/-   ##
===============================================
  Coverage                ?   85.81%           
===============================================
  Files                   ?      158           
  Lines                   ?    25154           
  Branches                ?        0           
===============================================
  Hits                    ?    21587           
  Misses                  ?     3567           
  Partials                ?        0           

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.

@vyasr vyasr added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Feb 7, 2023
@vyasr vyasr marked this pull request as ready for review February 7, 2023 06:36
@vyasr vyasr requested review from a team as code owners February 7, 2023 06:36
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
cpp/CMakeLists.txt Outdated Show resolved Hide resolved
@vyasr vyasr requested a review from bdice February 8, 2023 00:32
ci/test_cpp.sh Outdated Show resolved Hide resolved
@vyasr vyasr force-pushed the feat/reenable_libidentify_streams branch from 5c598e0 to 9071a48 Compare February 9, 2023 00:49
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.

All good to me. I checked the CI logs and I think it looks correct.

@davidwendt
Copy link
Contributor

Just curious how long this takes to run?
Our cpp-test matrix is very large. I wonder if running this under "nightly" would be better since I don't the entire test matrix is necessary to verify stream usage.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 10, 2023

Just curious how long this takes to run? Our cpp-test matrix is very large. I wonder if running this under "nightly" would be better since I don't the entire test matrix is necessary to verify stream usage.

This doesn't really add measureable overhead because I'm not running any new tests, I'm just running all of the existing tests with stream validation on. For reference, the conda-cpp-tests jobs take 7-10 minutes from what I see on other PRs, and that's the same in this PR.

In the future if the tests change in such a way that we want to run both with and without, then yes we may want to reconsider so that we don't rerun tests multiple times per push just for stream validation.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 10, 2023

/merge

@rapids-bot rapids-bot bot merged commit 1c0224f into rapidsai:branch-23.04 Feb 10, 2023
@vyasr vyasr deleted the feat/reenable_libidentify_streams branch February 10, 2023 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team CMake CMake build issue feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants