Added streams to JSON reader and writer api #14313

shrshi · 2023-10-23T16:17:39Z

Description

This PR contributes to #13744.

Added stream parameters to public APIs
cudf::io::read_json
cudf::io::write_json
Added stream gtests
Added copy constructor to internal JSON struct that was breaking for non-default streams

Checklist

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

copy-pr-bot · 2023-10-23T16:17:42Z

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

shrshi · 2023-10-23T16:59:09Z

/ok to test

shrshi · 2023-10-23T18:04:25Z

/ok to test

bdice

Nice! I have a couple of change requests, then this should be good to go.

cpp/include/cudf/io/json.hpp

cpp/tests/streams/io/functions_test.cpp

bdice

Thanks!

bdice · 2023-10-23T22:13:28Z

/ok to test

bdice · 2023-10-24T00:32:50Z

@shrshi Looks like there's a test failure (link to logs).

 C++ exception with description "cudf_identify_stream_usage found unexpected stream!" thrown in the test body.

This indicates that there might be an API that is using the default stream rather than a passed-in user stream.

vuule · 2023-10-24T22:21:27Z

Can you please also remove unused #include <cudf/utilities/default_stream.hpp> from JSON code? Looks like a missed a few in #13903
IMO it fits in the PR as a part of "don't use the default stream in JSON".

shrshi · 2023-10-24T23:35:17Z

/ok to test

shrshi · 2023-10-25T16:00:24Z

/ok to test

shrshi · 2023-10-25T16:07:52Z

Thank you for the feedback, @vuule and @bdice. I'm marking this PR as ready for review now.

bdice

Great job! Was the copy constructor of column_to_strings_fn the missing piece for the earlier errors?

shrshi · 2023-10-25T16:20:18Z

Yes, the error was that the implicitly generated copy constructor of column_to_strings_fn was calling the constructor of the string_scalar member with the default stream.

shrshi · 2023-10-25T18:38:15Z

/ok to test

shrshi · 2023-10-25T20:21:05Z

/ok to test

vuule

Tests can be cleaned up to exclude the default-able parameters.
Looks good otherwise!

vuule · 2023-10-26T18:01:16Z

cpp/tests/io/json_writer.cpp

+  cudf::io::write_json(options_builder.build(),
+                       cudf::test::get_default_stream(),
+                       rmm::mr::get_current_device_resource());


can these just be

Suggested change

cudf::io::write_json(options_builder.build(),

cudf::test::get_default_stream(),

rmm::mr::get_current_device_resource());

cudf::io::write_json(options_builder.build());

We have the same values for the defaults. Not sure why mr was specified in the first place.

now I see the difference, cudf::get_default_stream vs cudf::test::get_default_stream
does this matter for these tests? I don't see other PRs adding the stream parameter to all tests

Yes, the stream parameter can be left as is for now. For the moment, cudf::test::get_default_stream is only necessary for the stream tests. That may change in the future, but probably isn't worth planning for right now.

Thanks for the feedback! I've cleaned up the rmm::mr default parameter in both the calls.

Not sure I understand what "as is" is in @vyasr 's comment, but it looks like we might be adding the stream parameter to all API calls in the future so we might as do it now.

FWIW, the mr parameter can be removed from write_json calls in this file as well. I'm not blocking the PR on that, though.

shrshi · 2023-10-26T20:07:08Z

/ok to test

karthikeyann · 2023-10-27T13:04:53Z

cpp/src/io/json/write_json.cu

+  column_to_strings_fn(column_to_strings_fn const& other)
+    : options_{other.options_},
+      stream_{other.stream_},
+      mr_{other.mr_},
+      narep(other.narep, other.stream_),
+      struct_value_separator(other.struct_value_separator, other.stream_),
+      struct_row_begin_wrap(other.struct_row_begin_wrap, other.stream_),
+      struct_row_end_wrap(other.struct_row_end_wrap, other.stream_),
+      list_value_separator(other.list_value_separator, other.stream_),
+      list_row_begin_wrap(other.list_row_begin_wrap, other.stream_),
+      list_row_end_wrap(other.list_row_end_wrap, other.stream_)
+  {
+  }


column_to_strings_fn is not meant to be copied.
I just added the patch to fix the copy constructor from being invoked. (please apply pre-commit hooks for clang-format, after applying this patch)

diff --git a/cpp/src/io/json/write_json.cu b/cpp/src/io/json/write_json.cu index c98e93aec4..98b35fc943 100644 --- a/cpp/src/io/json/write_json.cu +++ b/cpp/src/io/json/write_json.cu @@ -633,17 +633,17 @@ struct column_to_strings_fn { auto child_string_with_null = [&]() { if (child_view.type().id() == type_id::STRUCT) { - return (*this).template operator()<cudf::struct_view>( + return this->template operator()<cudf::struct_view>( child_view, children_names.size() > child_index ? children_names[child_index].children : std::vector<column_name_info>{}); } else if (child_view.type().id() == type_id::LIST) { - return (*this).template operator()<cudf::list_view>(child_view, + return this->template operator()<cudf::list_view>(child_view, children_names.size() > child_index ? children_names[child_index].children : std::vector<column_name_info>{}); } else { - return cudf::type_dispatcher(child_view.type(), *this, child_view); + return cudf::type_dispatcher<cudf::id_to_type_impl, column_to_strings_fn const&>(child_view.type(), *this, child_view); } }; auto new_offsets = cudf::lists::detail::get_normalized_offsets( @@ -706,17 +706,17 @@ struct column_to_strings_fn { auto const& current_col = thrust::get<1>(i_current_col); // Struct needs children's column names if (current_col.type().id() == type_id::STRUCT) { - return (*this).template operator()<cudf::struct_view>( + return this->template operator()<cudf::struct_view>( current_col, children_names.size() > i ? children_names[i].children : std::vector<column_name_info>{}); } else if (current_col.type().id() == type_id::LIST) { - return (*this).template operator()<cudf::list_view>( + return this->template operator()<cudf::list_view>( current_col, children_names.size() > i ? children_names[i].children : std::vector<column_name_info>{}); } else { - return cudf::type_dispatcher(current_col.type(), *this, current_col); + return cudf::type_dispatcher<cudf::id_to_type_impl, column_to_strings_fn const&>(current_col.type(), *this, current_col); } });

Suggested change

column_to_strings_fn(column_to_strings_fn const& other)

: options_{other.options_},

stream_{other.stream_},

mr_{other.mr_},

narep(other.narep, other.stream_),

struct_value_separator(other.struct_value_separator, other.stream_),

struct_row_begin_wrap(other.struct_row_begin_wrap, other.stream_),

struct_row_end_wrap(other.struct_row_end_wrap, other.stream_),

list_value_separator(other.list_value_separator, other.stream_),

list_row_begin_wrap(other.list_row_begin_wrap, other.stream_),

list_row_end_wrap(other.list_row_end_wrap, other.stream_)

{

}

column_to_strings_fn(column_to_strings_fn const& other) = delete;

similarly other constructors and assignment operators can be deleted too.

Thank you, @karthikeyann. The patch has been applied.

TIL you can get the type dispatcher to take the functor by reference.
Thanks @karthikeyann!

Very nice job on this, @karthikeyann. 👏

…4549500; deleting copy and move assignment constructors

shrshi · 2023-10-27T19:28:58Z

/ok to test

karthikeyann

LGTM 👍

shrshi · 2023-10-30T17:32:42Z

/merge

karthikeyann · 2023-10-31T05:17:36Z

does codecov/patch check not stop the merge ?!

bdice · 2023-10-31T12:04:54Z

@karthikeyann Codecov is purely informational. It tends to have a lot of noise and false positives / false negatives. We are hoping that switching to a new branching strategy (like always merging to dev) will someday fix these issues.

Added streams to JSON reader and writer api; added tests

13e5be3

github-actions bot added libcudf Affects libcudf (C++/CUDA) code. CMake CMake build issue labels Oct 23, 2023

shrshi added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Oct 23, 2023

code linting fix for CMakeLists

c324948

bdice requested changes Oct 23, 2023

View reviewed changes

cpp/include/cudf/io/json.hpp Outdated Show resolved Hide resolved

cpp/tests/streams/io/functions_test.cpp Outdated Show resolved Hide resolved

shrshi added 3 commits October 23, 2023 21:07

Merge streams-io with branch-23.12

e33752e

Merge branch 'branch-23.12' into streams-io

68f80a2

docstring fix; default stream update in tests

3b8e982

shrshi requested a review from bdice October 23, 2023 22:03

bdice approved these changes Oct 23, 2023

View reviewed changes

added copy constructor to column_to_strings_fn struct

d3344c5

karthikeyann self-requested a review October 25, 2023 03:25

removed unused default stream headers from JSON code

3b8b031

shrshi marked this pull request as ready for review October 25, 2023 16:07

shrshi requested a review from a team as a code owner October 25, 2023 16:07

bdice approved these changes Oct 25, 2023

View reviewed changes

shrshi added 2 commits October 25, 2023 18:35

define/delete constructors in column_to_strings_fn struct

80c5a66

Merge branch 'branch-23.12' into streams-io

1147d6b

shrshi added 2 commits October 25, 2023 20:08

removed unused default stream header

1d87bfc

Merge branch 'streams-io' of github.com:shrshi/cudf into streams-io

12b3180

vuule requested changes Oct 26, 2023

View reviewed changes

cleanup of rmm default argument

bcc226c

shrshi requested a review from vuule October 26, 2023 20:06

Merge branch 'branch-23.12' into streams-io

f89d83b

vuule approved these changes Oct 26, 2023

View reviewed changes

karthikeyann requested changes Oct 27, 2023

View reviewed changes

shrshi added 3 commits October 27, 2023 19:12

applying patch https://github.com/rapidsai/cudf/pull/14313/files#r137…

b219b13

…4549500; deleting copy and move assignment constructors

json test cleanup; separating tests

13022da

Merge branch 'streams-io' of github.com:shrshi/cudf into streams-io

d145e97

shrshi requested a review from karthikeyann October 27, 2023 19:29

karthikeyann approved these changes Oct 30, 2023

View reviewed changes

rapids-bot bot merged commit abc0d41 into rapidsai:branch-23.12 Oct 30, 2023
56 of 57 checks passed

shrshi mentioned this pull request Nov 13, 2023

Added streams to CSV reader and writer api #14340

Merged

3 tasks

GregoryKimball mentioned this pull request Nov 22, 2023

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added streams to JSON reader and writer api #14313

Added streams to JSON reader and writer api #14313

shrshi commented Oct 23, 2023 •

edited

Loading

copy-pr-bot bot commented Oct 23, 2023

shrshi commented Oct 23, 2023

shrshi commented Oct 23, 2023

bdice left a comment

bdice left a comment

bdice commented Oct 23, 2023

bdice commented Oct 24, 2023

vuule commented Oct 24, 2023

shrshi commented Oct 24, 2023

shrshi commented Oct 25, 2023

shrshi commented Oct 25, 2023

bdice left a comment

shrshi commented Oct 25, 2023

shrshi commented Oct 25, 2023

shrshi commented Oct 25, 2023

vuule left a comment

vuule Oct 26, 2023

vuule Oct 26, 2023

vyasr Oct 26, 2023

shrshi Oct 26, 2023

vuule Oct 26, 2023

shrshi commented Oct 26, 2023

karthikeyann Oct 27, 2023 •

edited

Loading

karthikeyann Oct 27, 2023 •

edited

Loading

karthikeyann Oct 27, 2023

karthikeyann Oct 27, 2023

shrshi Oct 27, 2023

vuule Oct 27, 2023

bdice Oct 30, 2023

shrshi commented Oct 27, 2023

karthikeyann left a comment

shrshi commented Oct 30, 2023

karthikeyann commented Oct 31, 2023

bdice commented Oct 31, 2023

Added streams to JSON reader and writer api #14313

Added streams to JSON reader and writer api #14313

Conversation

shrshi commented Oct 23, 2023 • edited Loading

Description

Checklist

copy-pr-bot bot commented Oct 23, 2023

shrshi commented Oct 23, 2023

shrshi commented Oct 23, 2023

bdice left a comment

Choose a reason for hiding this comment

bdice left a comment

Choose a reason for hiding this comment

bdice commented Oct 23, 2023

bdice commented Oct 24, 2023

vuule commented Oct 24, 2023

shrshi commented Oct 24, 2023

shrshi commented Oct 25, 2023

shrshi commented Oct 25, 2023

bdice left a comment

Choose a reason for hiding this comment

shrshi commented Oct 25, 2023

shrshi commented Oct 25, 2023

shrshi commented Oct 25, 2023

vuule left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrshi commented Oct 26, 2023

karthikeyann Oct 27, 2023 • edited Loading

Choose a reason for hiding this comment

karthikeyann Oct 27, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shrshi commented Oct 27, 2023

karthikeyann left a comment

Choose a reason for hiding this comment

shrshi commented Oct 30, 2023

karthikeyann commented Oct 31, 2023

bdice commented Oct 31, 2023

shrshi commented Oct 23, 2023 •

edited

Loading

karthikeyann Oct 27, 2023 •

edited

Loading

karthikeyann Oct 27, 2023 •

edited

Loading