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

[BUG] gpuCI is not building cuDF with PER_THREAD_DEFAULT_STREAM #9165

Closed
harrism opened this issue Sep 1, 2021 · 4 comments
Closed

[BUG] gpuCI is not building cuDF with PER_THREAD_DEFAULT_STREAM #9165

harrism opened this issue Sep 1, 2021 · 4 comments
Assignees
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code.

Comments

@harrism
Copy link
Member

harrism commented Sep 1, 2021

Describe the bug
#5673 enabled building with PER_THREAD_DEFAULT_STREAM for all gpuCI test builds of libcudf. This seems to have stopped working. This log of a recent PR build shows that the strings PER_THREAD_DEFAULT_STREAM and ptds do not appear in any of the compiler command lines.

We had a pretty bad RMM bug that would have been caught if PTDS was still being tested.

I'm not sure why this is happening, as the build.sh scripts in cuDF are still passing the -ptds option.

Steps/Code to reproduce bug
Look at recent gpuCI libcudf compilation.

Expected behavior
PTDS should be enabled for libcudf test builds.

Environment overview (please complete the following information)

  • Environment location: gpuCI
@harrism harrism added bug Something isn't working gpuCI Needs Triage Need team to review and classify libcudf Affects libcudf (C++/CUDA) code. labels Sep 1, 2021
@beckernick beckernick removed the Needs Triage Need team to review and classify label Sep 7, 2021
@github-actions
Copy link

This issue has been labeled inactive-30d due to no recent activity in the past 30 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed. This issue will be labeled inactive-90d if there is no activity in the next 60 days.

@github-actions
Copy link

This issue has been labeled inactive-90d due to no recent activity in the past 90 days. Please close this issue if no further response or action is needed. Otherwise, please respond with a comment indicating any updates or changes to the original issue and/or confirm this issue still needs to be addressed.

@vyasr
Copy link
Contributor

vyasr commented Jul 15, 2022

I suspect that this is due to the fact that we use conda builds and we don't specify CUDF_USE_PER_THREAD_DEFAULT_STREAM in our conda build.sh scripts. The only place where -ptds is currently specified is in a branch of the GPU build.sh script that is only run when project flash is disabled. Could someone confirm that?

Assuming that this is the problem, I'm not sure of the best fix. Can I just change --cmake-args=\"-DCMAKE_INSTALL_LIBDIR=lib\" to `--cmake-args="-DCMAKE_INSTALL_LIBDIR=lib -DCUDF_USE_PER_THREAD_DEFAULT_STREAM=ON" in the conda build.sh file, or will that also affect builds in cases where we don't want to enable PTDS?

@vyasr
Copy link
Contributor

vyasr commented May 17, 2024

I'm going to close this issue. #11281 has a lot of the salient discussion. libcudf is unlikely to ever provide both PTDS and non-PTDS builds simultaneously due to resource constraints. Our plan for stream safety has evolved (see especially #11943) and we are tracking progress towards exposing stream-ordered APIs in #13744. Ultimately the plan is to convert all testing to pass streams through as discussed in that issue. Once that is done we can add suitable multi-stream testing as needed without relying on compilation with PTDS.

@vyasr vyasr closed this as completed May 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working libcudf Affects libcudf (C++/CUDA) code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants