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

Adapt cudf::scalar classes to changes in rmm::device_scalar #8411

Merged
merged 19 commits into from
Jun 8, 2021

Conversation

harrism
Copy link
Member

@harrism harrism commented Jun 1, 2021

rapidsai/rmm#789 refactors rmm::device_scalar, which all of cudf::scalar depends on. Notably, it renames some methods, makes stream parameters explicit, and deletes streamless constructors. As a result, the present PR deletes the default and non-stream copy constructors of all the cudf::*_scalar classes.

This should be merged immediately after rapidsai/rmm#789 because that PR will break the build.

@harrism harrism requested review from a team as code owners June 1, 2021 00:33
@github-actions github-actions bot added Java Affects Java cuDF API. Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Jun 1, 2021
@harrism harrism added breaking Breaking change improvement Improvement / enhancement to an existing function labels Jun 1, 2021
@harrism harrism self-assigned this Jun 1, 2021
Copy link
Contributor

@vuule vuule left a comment

Choose a reason for hiding this comment

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

Looks good, very small change suggested.

cpp/src/io/json/reader_impl.cu Outdated Show resolved Hide resolved
cpp/src/io/json/reader_impl.hpp Outdated Show resolved Hide resolved
Copy link
Contributor

@brandon-b-miller brandon-b-miller left a comment

Choose a reason for hiding this comment

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

Small cython change LGTM.

@harrism
Copy link
Member Author

harrism commented Jun 2, 2021

Small cython change LGTM.

@brandon-b-miller I can't see the Cython suggestion. Maybe you didn't add it? Edit: nevermind, I misread. I thought you meant you had one small cython change to suggest.

@harrism harrism requested a review from vuule June 2, 2021 01:13
@harrism
Copy link
Member Author

harrism commented Jun 2, 2021

CI won't pass until rapidsai/rmm/#789 is merged.

Copy link
Contributor

@revans2 revans2 left a comment

Choose a reason for hiding this comment

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

The changes look fine to me for the java side. I am going to try and run through the Spark integration tests with it too, but the changes are simple enough I don't foresee any issues

@revans2
Copy link
Contributor

revans2 commented Jun 2, 2021

I actually did find a bug, but it is in our tests. We were doing the wrong thing trying to get a long value from a DECIMAL32 scalar. If you want to apply the following patch it will fix the issue.

diff --git a/java/src/test/java/ai/rapids/cudf/ScalarTest.java b/java/src/test/java/ai/rapids/cudf/ScalarTest.java
index 00de3a696a..e49c86bc78 100644
--- a/java/src/test/java/ai/rapids/cudf/ScalarTest.java
+++ b/java/src/test/java/ai/rapids/cudf/ScalarTest.java
@@ -193,9 +193,14 @@ public class ScalarTest extends CudfTestBase {
     };
     for (BigDecimal dec: bigDecimals) {
       try (Scalar s = Scalar.fromDecimal(dec)) {
-        assertEquals(DType.fromJavaBigDecimal(dec), s.getType());
+        DType dtype =  DType.fromJavaBigDecimal(dec);
+        assertEquals(dtype, s.getType());
         assertTrue(s.isValid());
-        assertEquals(dec.unscaledValue().longValueExact(), s.getLong());
+        if (dtype.getTypeId() == DType.DTypeEnum.DECIMAL64) {
+          assertEquals(dec.unscaledValue().longValueExact(), s.getLong());
+        } else {
+          assertEquals(dec.unscaledValue().intValueExact(), s.getInt());
+        }
         assertEquals(dec, s.getBigDecimal());
       }
       try (Scalar s = Scalar.fromDecimal(-dec.scale(), dec.unscaledValue().intValueExact())) {

@@ -73,7 +73,7 @@ class scalar {
* @param is_valid true: set the value to valid. false: set it to null
* @param stream CUDA stream used for device memory operations.
*/
void set_valid(bool is_valid, rmm::cuda_stream_view stream = rmm::cuda_stream_default);
void set_valid_async(bool is_valid, rmm::cuda_stream_view stream = rmm::cuda_stream_default);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the r-value version of this function needs to be deleted?

Copy link
Member Author

@harrism harrism Jun 3, 2021

Choose a reason for hiding this comment

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

Actually I think it should not be deleted. Doing so results in ambiguity between the bool and bool&& versions of the function. The reason to delete would be to avoid the case where a reference to a temporary is passed to an async function, and the temporary is destroyed before the copy completes. But bool is a special case for which the lowest-level implementation in device_uvector uses cudaMemsetAsync. And since both set_valid_async and cudaMemsetAsync both take non-reference parameters, the value of the temporary will be copied into cudaMemsetAsync before the temporary can be destroyed.

@harrism
Copy link
Member Author

harrism commented Jun 2, 2021

If you want to apply the following patch it will fix the issue.

Applied.

rapids-bot bot pushed a commit to rapidsai/rmm that referenced this pull request Jun 8, 2021
This PR refactors `device_scalar` to use a  single-element `device_uvector` for its storage. This simplifies the implementation of device_scalar. Also changes the API of `device_scalar` so that its asynchronous / stream-ordered methods use the same API style (with explicit stream parameter) as `device_uvector` and `device_buffer`.

Closes #570

This is a breaking change. When it is merged, PRs are likely to need to be merged immediately in other libraries to account for the API changes. 

 - [x] cuDF: rapidsai/cudf#8411
 - [x] cuGraph: rapidsai/cugraph#1637
 - [x] RAFT: rapidsai/raft#259  
 - [x] ~cuML~ (unused)
 - [x] ~cuSpatial~ (unused)

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - Rong Ou (https://github.com/rongou)
  - Jake Hemstad (https://github.com/jrhemstad)

URL: #789
@harrism
Copy link
Member Author

harrism commented Jun 8, 2021

rerun tests

1 similar comment
@harrism
Copy link
Member Author

harrism commented Jun 8, 2021

rerun tests

@harrism
Copy link
Member Author

harrism commented Jun 8, 2021

@gpucibot merge

@codecov
Copy link

codecov bot commented Jun 8, 2021

Codecov Report

❗ No coverage uploaded for pull request base (branch-21.08@ae8ee8a). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##             branch-21.08    #8411   +/-   ##
===============================================
  Coverage                ?   82.84%           
===============================================
  Files                   ?      109           
  Lines                   ?    17913           
  Branches                ?        0           
===============================================
  Hits                    ?    14840           
  Misses                  ?     3073           
  Partials                ?        0           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae8ee8a...af870a8. Read the comment docs.

@rapids-bot rapids-bot bot merged commit aa82646 into rapidsai:branch-21.08 Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking Breaking change improvement Improvement / enhancement to an existing function Java Affects Java cuDF API. libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants