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

[FEA] Rename PER_THREAD_DEFAULT_STREAM cmake option to be unique to cuDF #10862

Closed
jrhemstad opened this issue May 16, 2022 · 3 comments · Fixed by #11134
Closed

[FEA] Rename PER_THREAD_DEFAULT_STREAM cmake option to be unique to cuDF #10862

jrhemstad opened this issue May 16, 2022 · 3 comments · Fixed by #11134
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS

Comments

@jrhemstad
Copy link
Contributor

Is your feature request related to a problem? Please describe.

The PER_THREAD_DEFAULT_STREAM cmake option controls enabling CUDA_API_PER_THREAD_DEFAULT_STREAM to enable per-thread default stream semantics in CUDA.

This name is not scoped to CUDF. This is not descriptive that it is a CUDF specific option and it could collide with other projects.

Describe the solution you'd like

Rename to CUDF_USE_PER_THREAD_DEFAULT_STREAM.
@jrhemstad jrhemstad added feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. labels May 16, 2022
@jrhemstad jrhemstad mentioned this issue May 16, 2022
Closed
@jrhemstad jrhemstad added the Spark Functionality that helps Spark RAPIDS label May 16, 2022
@jbrennan333 jbrennan333 mentioned this issue May 17, 2022
Merged
@jbrennan333
Copy link
Contributor

I put up PR #10877 which fixes this, but does not remove PER_THREAD_DEFAULT_STREAM yet.
Will follow-up in 22.08 to remove PER_THREAD_DEFAULT_STREAM.

rapids-bot bot pushed a commit that referenced this issue May 24, 2022
@jbrennan333
Replace defaulted stream value for libcudf APIs that use NVCOMP (#10877)
e096345 
This PR addresses #10862, but does not completely remove `PER_THREAD_DEFAULT_STREAM`.   I just adds the new define `CUDF_USE_PER_THREAD_DEFAULT_STREAM` and adds a deprecation warning for `PER_THREAD_DEFAULT_STREAM`.

This PR also addresses #10864, but only changes the default parameter for `read_avro` and `read_parquet` to `cudf::default_stream_value`, which is defined as `rmm::cuda_stream_per_thread` when `CUDF_USE_PER_THREAD_DEFAULT_STREAM` is defined.  These cover the current interfaces to nvcomp.

The goal for this PR is to ensure that we pass `rmm::cuda_stream_per_thread` to the nvcomp apis when `CUDF_USE_PER_THREAD_DEFAULT_STREAM` is defined.  This is needed for nvcomp-2.3, which is provided as dynamic libraries where we cannot recompile with PTDS enabled.  nvcomp-2.3 is being enabled in #10851.

I have not marked this PR as closing the above issues, because we will need to follow up in a future PR to remove  `PER_THREAD_DEFAULT_STREAM` and in another to replace all of the rest of the cuDF API call sites to use the new `cudf::default_stream_value`.

i am putting this up as a draft PR because I have not tested with nvcomp-2.3 yet.

Authors:
  - Jim Brennan (https://github.com/jbrennan333)

Approvers:
  - https://github.com/nvdbaranec
  - AJ Schmidt (https://github.com/ajschmidt8)
  - Jason Lowe (https://github.com/jlowe)
  - Vukasin Milovanovic (https://github.com/vuule)
  - Jake Hemstad (https://github.com/jrhemstad)
  - Robert Maynard (https://github.com/robertmaynard)

URL: #10877
@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.

@jbrennan333
Copy link
Contributor

This issue is still in progress. I will be putting up a PR to remove PER_THREAD_DEFAULT_STREAM.

@jbrennan333 jbrennan333 mentioned this issue Jun 22, 2022
Merged
rapids-bot bot pushed a commit that referenced this issue Jun 23, 2022
@jbrennan333
Remove deprecated PER_THREAD_DEFAULT_STREAM (#11134)
4014a65 
Closes #10862.

The `PER_THREAD_DEFAULT_STREAM` build option was deprecated in branch-22.06 via #10877, and replaced with the new build option `CUDF_USE_PER_THREAD_DEFAULT_STREAM`.

This PR removes `PER_THREAD_DEFAULT_STREAM`.

I am putting this up as a draft because I am not certain if we want to do this in 22.08 or wait for a later release?

Authors:
  - Jim Brennan (https://github.com/jbrennan333)

Approvers:
  - Jason Lowe (https://github.com/jlowe)
  - Vyas Ramasubramani (https://github.com/vyasr)

URL: #11134
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
feature request New feature or request libcudf Affects libcudf (C++/CUDA) code. Spark Functionality that helps Spark RAPIDS
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove deprecated PER_THREAD_DEFAULT_STREAM jbrennan333/cudf
2 participants
@jrhemstad @jbrennan333