Replace default stream for scalars and column factories usages (because of defaulted arguments) #14354

karthikeyann · 2023-11-02T18:45:13Z

Description

This PR contributes to #13744

Adds missing stream for scalars, column factories
Uses right set_null_mask for moving null masks instead of copy. (due to defaulted stream argument).

Checklist

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

cpp/src/groupby/sort/aggregate.cpp

ttnghia · 2023-11-03T04:51:15Z

cpp/src/io/csv/writer_impl.cu

+  cudf::string_scalar newline{options.get_line_terminator(), true, stream};
  auto p_str_col_w_nl = cudf::strings::detail::join_strings(str_column_view,
                                                            newline,
-                                                            string_scalar("", false),
+                                                            string_scalar("", false, stream),


newline is defined separately while the 3rd parameter is defined inline. Can you make them consistent please?

I don't think, it's not a matter of being consistent. That temporary scalar is not required after this usage. So, it's not a named parameter. its scope ends after function call.
newline is used below.

ttnghia · 2023-11-03T04:52:12Z

cpp/src/io/csv/writer_impl.cu

+            cudf::string_scalar(delimiter_str, true, stream),
+            narep,


One parameter is inline defined while the other is not. Please make them consistent.

That temporary scalar is not required after this usage. So, it's not a named parameter. its scopes end after function call.
Here narep is used below this line too.

bdice

I started a review earlier and didn't submit it -- sorry. Flushing comments for now, and I'll try to get back to completing the review soon.

bdice · 2023-11-02T18:52:00Z

cpp/src/groupby/groupby.cu

+                               empty_like(values),
+                               0,
+                               {},
+                               cudf::get_default_stream());


This is introducing a use of the default stream, which we want to avoid. There are public APIs accepting a user stream that can call this, which won't use the desired stream. We need to refactor all callers of this code path, and pass their stream through.

is the stream relevant in this case? Is any allocation actually happening when creating an empty column?

I've filed an issue a while ago for it: #11463 and there is also another issue for it: #13737.

So before that issue is closed, we don't have any input stream to put into line 115 above and have to use the default stream anyway.

groupby needs a bigger cleanup; I can exclude it as part of this PR. It should be a separate PR.

Removed groupby from this PR.

cpp/src/io/csv/writer_impl.cu

cpp/src/lists/dremel.cu

vuule

Looks good.
Posted a few follow-up questions.

cpp/src/unary/cast_ops.cu

cpp/src/table/table.cpp

cpp/src/scalar/scalar.cpp

bdice

We should not add new calls to cudf::get_default_stream() in internal implementations. @vuule just raised this here: https://github.com/rapidsai/cudf/pull/14354/files#r1393097787

It's the same problem as mentioned here: #14354 (comment)

Is there a way we can fix this in order, so that those calls have the stream passed through by their caller?

vuule · 2023-11-14T19:12:21Z

We should not add new calls to cudf::get_default_stream() in internal implementations. @vuule just raised this here: https://github.com/rapidsai/cudf/pull/14354/files#r1393097787

It's the same problem as mentioned here: #14354 (comment)

Is there a way we can fix this in order, so that those calls have the stream passed through by their caller?

I'm okay with explicitly passing the default stream in cases where we don't have the user stream available (yet). It makes the default stream use very visible so we'll find these places easily once we get to updating those APIs to take a stream. IMO this is a good change, even though it does not really change the stream.

vyasr · 2023-11-14T20:07:30Z

I would recommend just adding a stream parameter with a default value and then forwarding that to the column constructor. Is there any reason we can't do that? It shouldn't break any existing APIs. While we're at it, we probably want to add a default mr parameter too. The table shouldn't be default copy-constructible.

I think either leaving the implicit default stream or adding an explicit argument to the column constructor is fine as a short-term solution, but since columns already support streams in the public API I don't see any reason to delay adding that support for tables.

karthikeyann · 2023-11-15T19:13:52Z

As @vuule mentioned, default stream is added to make its use visible and hence allows us to replace it in future PRs.
@vyasr Adding default arguments (stream/mr) will not break existing APIs. I am also concerned about their future usage ignoring these defaults, which will again need to be fixed by similar cleanup-PRs (if they are not caught by existing stream tests). Is there any alternative or a tradeoff? Ideally, we should prefer to make it hard to use the APIs wrongly, and easy to use it in intended way.

I thought about explicit keyword for copy constructor of scalar (I think, it's similar for column as well). I feared if it might cause our code be too verbose. But I compiled libcudf with explicit for scalar; only scalar tests need to be updated. Also, this might be a breaking change, as users need to update their code to call the copy constructor explicitly.

Moving this PR for 24.02.

Removing my blocking review, after further conversation.

cpp/src/io/json/write_json.cu

vuule · 2023-11-18T03:12:16Z

cpp/src/io/csv/writer_impl.cu

+      cudf::string_scalar(options_.get_true_value(), true, stream_),
+      cudf::string_scalar(options_.get_false_value(), true, stream_),


Would you mind making a change similar to the one in #14444?

cpp/src/io/csv/reader_impl.cu

wence- · 2023-11-20T15:38:36Z

None of the Python changes look like they belong in this PR. Is it because you were previously targeted to 23.12 and have some changes from there that are not yet in 24.02 due to the forward-merge no coming across yet?

karthikeyann · 2023-11-28T19:17:44Z

@wence- pulled 24.02. python changes are gone.

wence- · 2023-11-29T10:17:50Z

@wence- pulled 24.02. python changes are gone.

Thanks! Removed the now unnecessary codeowner review requests.

cpp/src/scalar/scalar.cpp

cpp/src/strings/convert/convert_booleans.cu

cpp/src/strings/convert/convert_durations.cu

cpp/src/strings/convert/convert_floats.cu

cpp/src/strings/convert/convert_integers.cu

cpp/src/strings/convert/convert_ipv4.cu

cpp/src/table/table.cpp

Co-authored-by: Bradley Dice <[email protected]>

cpp/src/scalar/scalar.cpp

karthikeyann · 2023-12-13T02:27:11Z

/merge

Add missing stream for scalars, column factories

694f8d4

karthikeyann added libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Nov 2, 2023

karthikeyann requested a review from a team as a code owner November 2, 2023 18:45

karthikeyann requested review from bdice and nvdbaranec November 2, 2023 18:45

ttnghia reviewed Nov 3, 2023

View reviewed changes

cpp/src/groupby/sort/aggregate.cpp Show resolved Hide resolved

ttnghia reviewed Nov 3, 2023

View reviewed changes

add missing stream documentation

06d8691

karthikeyann requested a review from ttnghia November 3, 2023 16:28

GregoryKimball assigned karthikeyann Nov 3, 2023

bdice reviewed Nov 6, 2023

View reviewed changes

GregoryKimball mentioned this pull request Nov 9, 2023

[FEA] Stream-preserving constructors for scalar class and its derived classes #14333

Open

karthikeyann added 2 commits November 10, 2023 21:14

address review comments, skip groupby

11141ef

Merge branch 'branch-23.12' of github.com:rapidsai/cudf into fix-stream1

4e43fae

karthikeyann requested review from vuule and bdice November 10, 2023 15:53

karthikeyann changed the title ~~Add missing stream for scalars, column factories~~ Replace default stream for scalars, column factories (because of defaulted arguments) Nov 14, 2023

karthikeyann changed the title ~~Replace default stream for scalars, column factories (because of defaulted arguments)~~ Replace default stream for scalars and column factories (because of defaulted arguments) Nov 14, 2023

karthikeyann changed the title ~~Replace default stream for scalars and column factories (because of defaulted arguments)~~ Replace default stream for scalars and column factories usages (because of defaulted arguments) Nov 14, 2023

vuule approved these changes Nov 14, 2023

View reviewed changes

cpp/src/unary/cast_ops.cu Show resolved Hide resolved

cpp/src/table/table.cpp Outdated Show resolved Hide resolved

cpp/src/scalar/scalar.cpp Outdated Show resolved Hide resolved

bdice previously requested changes Nov 14, 2023

View reviewed changes

Merge branch 'branch-23.12' into fix-stream1

92b3263

karthikeyann changed the base branch from branch-23.12 to branch-24.02 November 15, 2023 19:16

karthikeyann requested a review from a team as a code owner November 15, 2023 19:16

vuule reviewed Nov 18, 2023

View reviewed changes

cpp/src/io/json/write_json.cu Outdated Show resolved Hide resolved

vuule reviewed Nov 18, 2023

View reviewed changes

cpp/src/io/csv/reader_impl.cu Outdated Show resolved Hide resolved

GregoryKimball mentioned this pull request Nov 21, 2023

[FEA] Expose public stream-ordered C++ APIs #13744

Open

Merge branch 'branch-24.02' into fix-stream1

034d767

wence- removed request for a team and shwina November 29, 2023 10:16

Merge branch 'branch-24.02' into fix-stream1

a18c7ef

bdice reviewed Nov 29, 2023

View reviewed changes

karthikeyann and others added 4 commits December 7, 2023 00:34

Apply suggestions from code review

c2c3221

Co-authored-by: Bradley Dice <[email protected]>

add explicit to table copy ctor

576e1c6

Merge branch 'branch-24.02' into fix-stream1

850ffe6

add stream to duration_scalar::count

acef7d0

karthikeyann requested a review from bdice December 6, 2023 19:31

ttnghia reviewed Dec 7, 2023

View reviewed changes

cpp/src/scalar/scalar.cpp Outdated Show resolved Hide resolved

address review comments

07ae25d

karthikeyann requested a review from ttnghia December 7, 2023 14:47

Merge branch 'branch-24.02' into fix-stream1

c36c2cf

bdice approved these changes Dec 12, 2023

View reviewed changes

ttnghia approved these changes Dec 12, 2023

View reviewed changes

vyasr approved these changes Dec 12, 2023

View reviewed changes

rapids-bot bot merged commit 0698438 into rapidsai:branch-24.02 Dec 13, 2023
67 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace default stream for scalars and column factories usages (because of defaulted arguments) #14354

Replace default stream for scalars and column factories usages (because of defaulted arguments) #14354

karthikeyann commented Nov 2, 2023 •

edited

Loading

ttnghia Nov 3, 2023

karthikeyann Nov 3, 2023 •

edited

Loading

ttnghia Nov 3, 2023

karthikeyann Nov 3, 2023 •

edited

Loading

bdice left a comment

bdice Nov 2, 2023

vuule Nov 6, 2023

ttnghia Nov 6, 2023 •

edited

Loading

ttnghia Nov 6, 2023

karthikeyann Nov 7, 2023

karthikeyann Nov 10, 2023

vuule left a comment

bdice left a comment •

edited

Loading

vuule commented Nov 14, 2023

vyasr commented Nov 14, 2023 •

edited

Loading

karthikeyann commented Nov 15, 2023

vuule Nov 18, 2023

wence- commented Nov 20, 2023

karthikeyann commented Nov 28, 2023 •

edited

Loading

wence- commented Nov 29, 2023

karthikeyann commented Dec 13, 2023

		cudf::string_scalar(options_.get_true_value(), true, stream_),
		cudf::string_scalar(options_.get_false_value(), true, stream_),

Replace default stream for scalars and column factories usages (because of defaulted arguments) #14354

Replace default stream for scalars and column factories usages (because of defaulted arguments) #14354

Conversation

karthikeyann commented Nov 2, 2023 • edited Loading

Description

Checklist

Choose a reason for hiding this comment

karthikeyann Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikeyann Nov 3, 2023 • edited Loading

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ttnghia Nov 6, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vuule left a comment

Choose a reason for hiding this comment

bdice left a comment • edited Loading

Choose a reason for hiding this comment

vuule commented Nov 14, 2023

vyasr commented Nov 14, 2023 • edited Loading

karthikeyann commented Nov 15, 2023

Choose a reason for hiding this comment

wence- commented Nov 20, 2023

karthikeyann commented Nov 28, 2023 • edited Loading

wence- commented Nov 29, 2023

karthikeyann commented Dec 13, 2023

karthikeyann commented Nov 2, 2023 •

edited

Loading

karthikeyann Nov 3, 2023 •

edited

Loading

karthikeyann Nov 3, 2023 •

edited

Loading

ttnghia Nov 6, 2023 •

edited

Loading

bdice left a comment •

edited

Loading

vyasr commented Nov 14, 2023 •

edited

Loading

karthikeyann commented Nov 28, 2023 •

edited

Loading