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

Fix default values for stream parameter #1583

Merged
merged 6 commits into from
Nov 21, 2023

Conversation

ttnghia
Copy link
Collaborator

@ttnghia ttnghia commented Nov 21, 2023

This changes the default values for stream parameter in various APIs. We should switch to use cudf::get_default_stream() which is consistent throughout the library (it will be either rmm::cuda_stream_default or rmm::cuda_stream_per_thread). Using only rmm::cuda_stream_default may lead to mixed use of rmm::cuda_stream_default and rmm::cuda_stream_per_thread in the same place.

@ttnghia ttnghia added the bug Something isn't working label Nov 21, 2023
@ttnghia ttnghia self-assigned this Nov 21, 2023
@ttnghia
Copy link
Collaborator Author

ttnghia commented Nov 21, 2023

build

@@ -16,45 +16,39 @@

#include <cudf/column/column_view.hpp>
#include <cudf/table/table.hpp>
#include <cudf/utilities/default_stream.hpp>

//
Copy link
Member

Choose a reason for hiding this comment

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

Comment with no text? Should either remove these or add a comment. I'm assuming this is to delineate sections of the includes (IMO not necessary, pretty obvious from include paths or lack thereof), but without text it's not clear what the intent is.

Applies to everywhere this was added in this PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is due to the issue with clang-format that messes up the header order. I have to use an empty comment line to prevent header reordering.

Copy link
Member

Choose a reason for hiding this comment

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

Does it need to be empty, or can the comment say "// this is here to avoid clang-format from rearranging the header files". Also wondering why we're explicitly avoiding the recommended header order?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The current clang-format config combines headers together and doesn't respect the conventional "near to far" order. That means we should include our header first, then library headers, then builtin headers. By using "near to far" order, we can ensure that most headers have their own dependencies included themselves.

We probably should change the config a little bit to account for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ideally if we switch to use .clang-format style from cudf then we don't have to worry about this as cudf config can avoid this mixing header issue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

There is another option: fix the style to preserve header grouping: #1584. With that, the header organization is totally up to the code author.

Copy link
Collaborator

@jbrennan333 jbrennan333 left a comment

Choose a reason for hiding this comment

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

+1 this looks good to me.

jlowe
jlowe previously approved these changes Nov 21, 2023
Signed-off-by: Nghia Truong <[email protected]>
Signed-off-by: Nghia Truong <[email protected]>
Signed-off-by: Nghia Truong <[email protected]>
This reverts commit 9b6b528.

Signed-off-by: Nghia Truong <[email protected]>
@jlowe
Copy link
Member

jlowe commented Nov 21, 2023

This last commit was so much easer to read! 😸

@ttnghia
Copy link
Collaborator Author

ttnghia commented Nov 21, 2023

build

@ttnghia ttnghia merged commit edc0868 into NVIDIA:branch-23.12 Nov 21, 2023
2 checks passed
@ttnghia ttnghia deleted the fix_stream branch June 21, 2024 17:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants