Workaround thrust-copy-if limit in json get_tree_representation #12190

davidwendt · 2022-11-17T20:41:51Z

Description

Workaround in json's get_tree_representation due to limitation in thrust::copy_if which fails if the input-iterator spans more than int-max.

Found existing thrust issue: NVIDIA/cccl#747
This calls the thrust::copy_if in chunks if the iterator can span greater than int-max.

Found while working on #12079

Checklist

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

codecov · 2022-11-18T01:29:15Z

Codecov Report

Base: 88.37% // Head: 88.18% // Decreases project coverage by -0.18% ⚠️

Coverage data is based on head (641d5ad) compared to base (a9f9958).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head 641d5ad differs from pull request most recent head 80cd72f. Consider uploading reports for the commit 80cd72f to get more accurate results

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-23.02   #12190      +/-   ##
================================================
- Coverage         88.37%   88.18%   -0.19%     
================================================
  Files               137      137              
  Lines             22657    22657              
================================================
- Hits              20022    19981      -41     
- Misses             2635     2676      +41

Impacted Files	Coverage Δ
python/cudf/cudf/core/dataframe.py	`93.66% <ø> (ø)`
python/cudf/cudf/core/series.py	`95.83% <ø> (ø)`
python/cudf/cudf/core/_base_index.py	`81.15% <100.00%> (ø)`
python/cudf/cudf/core/index.py	`92.92% <100.00%> (ø)`
...thon/dask_cudf/dask_cudf/tests/test_distributed.py	`18.86% <0.00%> (-67.93%)`	⬇️
python/strings_udf/strings_udf/__init__.py	`69.35% <0.00%> (-6.46%)`	⬇️
python/cudf/cudf/core/column/categorical.py	`89.34% <0.00%> (-0.22%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

elstehle

Thanks for the workaround. This looks good. There should be effectively "zero" overhead in cases when we're running for less than INT_MAX items.

nit: would we want to put this into some common ground for other places to use, where we could run into scenarios that exceed INT_MAX problem sizes (e.g., the wordpiece tokenizer)?

davidwendt · 2022-11-30T22:13:26Z

nit: would we want to put this into some common ground for other places to use, where we could run into scenarios that exceed INT_MAX problem sizes (e.g., the wordpiece tokenizer)?

I was considering that. I think the other 2 places this is occurs will be fixed differently in the future (not require the copy-if).
The only other one I found was this one here (which also used the sentinel overload) and so I thought it best to keep it isolated until we found another case. I could easily be persuaded to move this to a detail header.

hyperbolic2346

Small not on comment typo, but otherwise looks good.

hyperbolic2346 · 2022-12-02T16:46:32Z

cpp/src/io/json/json_tree.cu

+ *
+ * Workaround for thrust::copy_if bug (https://github.com/NVIDIA/thrust/issues/1302)
+ * where it cannot iterate over int-max values `distance(first,last) > int-max`
+ * This calls thrust::copy_if in 2B chunks instead.


ttnghia · 2022-12-02T17:37:12Z

cpp/src/io/json/json_tree.cu

+OutputIterator thrust_copy_if(rmm::exec_policy policy,
+                              InputIterator first,
+                              InputIterator last,
+                              StencilIterator stencil,
+                              OutputIterator result,
+                              Predicate pred)
+{
+  auto const copy_size = std::min(static_cast<std::size_t>(std::distance(first, last)),
+                                  static_cast<std::size_t>(std::numeric_limits<int>::max()));
+
+  auto itr = first;
+  while (itr != last) {
+    auto const copy_end =
+      static_cast<std::size_t>(std::distance(itr, last)) <= copy_size ? last : itr + copy_size;
+    result = thrust::copy_if(policy, itr, copy_end, stencil, result, pred);
+    stencil += std::distance(itr, copy_end);
+    itr = copy_end;
+  }
+  return result;
+}
+
+template <typename InputIterator, typename OutputIterator, typename Predicate>
+OutputIterator thrust_copy_if(rmm::exec_policy policy,
+                              InputIterator first,
+                              InputIterator last,
+                              OutputIterator result,
+                              Predicate pred)
+{
+  return thrust_copy_if(policy, first, last, first, result, pred);


Wow, I assume that you're going to extract this out into a common utility file and call it across many places (in a future follow-up PR).

#12190 (comment)

I'll look into moving this in a follow-up PR as suggested.

davidwendt · 2022-12-06T13:36:09Z

@gpucibot merge

Workaround thrust-copy-if limit in json get_tree_representation

5b06e35

davidwendt added bug Something isn't working 2 - In Progress Currently a work in progress libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change labels Nov 17, 2022

davidwendt self-assigned this Nov 17, 2022

davidwendt added 14 commits November 18, 2022 08:48

Merge branch 'branch-23.02' into rework-copy-if-json-tree

e99af2f

Merge branch 'branch-23.02' into rework-copy-if-json-tree

32e2961

remove unneeded statement

86978e0

Merge branch 'branch-23.02' into rework-copy-if-json-tree

a8091b2

Merge branch 'branch-23.02' into rework-copy-if-json-tree

4c0dc44

Merge branch 'branch-23.02' into rework-copy-if-json-tree

5043067

Merge branch 'branch-23.02' into rework-copy-if-json-tree

484230a

Merge branch 'branch-23.02' into rework-copy-if-json-tree

9a6da7f

Merge branch 'branch-23.02' into rework-copy-if-json-tree

7a39893

Merge branch 'branch-23.02' into rework-copy-if-json-tree

953da79

Merge branch 'branch-23.02' into rework-copy-if-json-tree

76b5e6e

Merge branch 'branch-23.02' into rework-copy-if-json-tree

bbc77e3

Merge branch 'branch-23.02' into rework-copy-if-json-tree

5b2caac

use rmm::exec_policy as parm instead of stream

e8f9307

davidwendt added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Nov 30, 2022

davidwendt marked this pull request as ready for review November 30, 2022 14:46

davidwendt requested a review from a team as a code owner November 30, 2022 14:46

davidwendt requested review from hyperbolic2346 and divyegala November 30, 2022 14:46

elstehle approved these changes Nov 30, 2022

View reviewed changes

davidwendt added 4 commits November 30, 2022 19:04

Merge branch 'branch-23.02' into rework-copy-if-json-tree

7ecbc9a

Merge branch 'branch-23.02' into rework-copy-if-json-tree

573ce29

Merge branch 'branch-23.02' into rework-copy-if-json-tree

beecb9a

Merge branch 'branch-23.02' into rework-copy-if-json-tree

362067b

hyperbolic2346 approved these changes Dec 2, 2022

View reviewed changes

ttnghia reviewed Dec 2, 2022

View reviewed changes

davidwendt added 2 commits December 2, 2022 13:57

Merge branch 'branch-23.02' into rework-copy-if-json-tree

4ee27f3

Merge branch 'branch-23.02' into rework-copy-if-json-tree

80cd72f

rapids-bot bot merged commit 394f414 into rapidsai:branch-23.02 Dec 6, 2022

davidwendt deleted the rework-copy-if-json-tree branch December 6, 2022 13:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workaround thrust-copy-if limit in json get_tree_representation #12190

Workaround thrust-copy-if limit in json get_tree_representation #12190

davidwendt commented Nov 17, 2022

codecov bot commented Nov 18, 2022 •

edited

Loading

elstehle left a comment

davidwendt commented Nov 30, 2022 •

edited

Loading

hyperbolic2346 left a comment

hyperbolic2346 Dec 2, 2022

ttnghia Dec 2, 2022 •

edited

Loading

davidwendt Dec 2, 2022

davidwendt Dec 6, 2022

davidwendt commented Dec 6, 2022

Workaround thrust-copy-if limit in json get_tree_representation #12190

Workaround thrust-copy-if limit in json get_tree_representation #12190

Conversation

davidwendt commented Nov 17, 2022

Description

Checklist

codecov bot commented Nov 18, 2022 • edited Loading

Codecov Report

elstehle left a comment

Choose a reason for hiding this comment

davidwendt commented Nov 30, 2022 • edited Loading

hyperbolic2346 left a comment

Choose a reason for hiding this comment

hyperbolic2346 Dec 2, 2022

Choose a reason for hiding this comment

ttnghia Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

davidwendt Dec 2, 2022

Choose a reason for hiding this comment

davidwendt Dec 6, 2022

Choose a reason for hiding this comment

davidwendt commented Dec 6, 2022

codecov bot commented Nov 18, 2022 •

edited

Loading

davidwendt commented Nov 30, 2022 •

edited

Loading

ttnghia Dec 2, 2022 •

edited

Loading