Enable compiled binary ops in libcudf, python and java #8741

karthikeyann · 2021-07-14T15:50:46Z

cudf::binary_operation calls compiled binary ops.
cudf::jit::binary_operation calls jit binary ops
So, compiled binary ops is called in libcudf (groupby, rescale), python (binary ops) and java (binary ops)

Breaking change:
New: Logical and Comparison operators can have output type to be only bool type.
Old: Logical operators can have integer or any other output type that can be constructed from bool type. Comparison operators required bool type only.

In this release (21.10), experimental namespace is dropped, and compiled binary ops replaces jit binary ops in libcudf, except for user defined op.

revans2

I ran through all of the Spark tests with this and they all passed. I deleted the JIT cache before running them and it was not recreated. The effectively removes all JIT from the Spark plugin.

nvdbaranec · 2021-07-14T17:13:45Z

cpp/include/cudf/detail/binaryop.hpp

+ * @copydoc cudf::experimental::binary_operation(scalar const&, column_view const&, binary_operator,
+ * data_type, rmm::mr::device_memory_resource *)
+ *
+ * @param stream CUDA stream used for device memory operations and kernel launches.


Parameter order here is incorrect. Should be stream, mr

Same applies for the other 3 declarations in this file.

@copydoc is used at many places in our cpp documentation to avoid duplication of doc text, most often used in detail APIs. There is no cleaner way so far.

Note: Alternatively, we could contribute to doxygen to reorder the @param as listed in the function signature. Timeline: Uncertain.

@copydoc is used at many places in our cpp documentation to avoid duplication of doc text, most often used in detail APIs. There is no cleaner way so far.

Right, but isn't this going to result in incorrect documentation? My expectation is that you will end up getting something like:

* @param lhs The left operand scalar * @param rhs The right operand column * @param op The binary operator * @param output_type The desired data type of the output column * @param mr Device memory resource used to allocate the returned column's device memory * @param stream stream CUDA stream used for device memory operations and kernel launches. * @return Output column of `output_type` type containing the result of * the binary operation * @throw cudf::logic_error if @p output_type dtype isn't fixed-width

...which is the incorrect order.

This is how it's rendered now.

This is not better too.
(I generated this temporarily to show. it's not in released documentation)

detail APIs are not in released libcudf documentation. They are excluded by doxygen.

karthikeyann · 2021-07-14T18:37:12Z

@revans2 Great.

codecov · 2021-07-14T20:16:04Z

Codecov Report

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

❗ Current head bc29430 differs from pull request most recent head e7e81ce. Consider uploading reports for the commit e7e81ce to get more accurate results

@@               Coverage Diff               @@
##             branch-21.10    #8741   +/-   ##
===============================================
  Coverage                ?   10.73%           
===============================================
  Files                   ?      114           
  Lines                   ?    19058           
  Branches                ?        0           
===============================================
  Hits                    ?     2046           
  Misses                  ?    17012           
  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 6cd0167...e7e81ce. Read the comment docs.

harrism · 2021-07-14T23:11:00Z

In next release (21.10), experimental namespace will be dropped, and compiled binary ops will replace jit binary ops in libcudf, except for user defined op.

Do we need to formally deprecate anything? Will dropping the experimental namespace be a breaking change?

karthikeyann · 2021-07-15T19:20:09Z

In next release (21.10), experimental namespace will be dropped, and compiled binary ops will replace jit binary ops in libcudf, except for user defined op.

Do we need to formally deprecate anything? Will dropping the experimental namespace be a breaking change?

No deprecation needed. Dropping experimental means replacing jit with compiled. Few operator + output type combinations will not work. It will break for these few combinations. There are no changes in API interface.

galipremsagar · 2021-07-15T23:08:22Z

Looks like there is a repartition error specific to this PR

karthikeyann · 2021-07-20T04:46:52Z

rerun tests

harrism · 2021-07-27T01:23:47Z

In next release (21.10), experimental namespace will be dropped, and compiled binary ops will replace jit binary ops in libcudf, except for user defined op.

@karthikeyann since this got delayed to 21.10, should we go ahead and do this as part of this PR? Or are you concerned that we need to test it in the field as "experimental" first?

CC @jrhemstad for another opinion.

…le_compiled_binops

karthikeyann · 2021-08-01T19:38:02Z

will do this as part of this PR.

revans2 · 2021-08-16T14:57:36Z

You forgot to delete the experimental namespace from the JNI code. After that it looks good to me.

diff --git a/java/src/main/native/src/ColumnViewJni.cpp b/java/src/main/native/src/ColumnViewJni.cpp
index 4bd1ac06f3..e9d427cb54 100644
--- a/java/src/main/native/src/ColumnViewJni.cpp
+++ b/java/src/main/native/src/ColumnViewJni.cpp
@@ -1110,7 +1110,7 @@ JNIEXPORT jlong JNICALL Java_ai_rapids_cudf_ColumnView_binaryOpVV(JNIEnv *env, j
     cudf::data_type n_data_type = cudf::jni::make_data_type(out_dtype, scale);
     cudf::binary_operator op = static_cast<cudf::binary_operator>(int_op);
     std::unique_ptr<cudf::column> result =
-        cudf::experimental::binary_operation(*lhs, *rhs, op, n_data_type);
+        cudf::binary_operation(*lhs, *rhs, op, n_data_type);
     return reinterpret_cast<jlong>(result.release());
   }
   CATCH_STD(env, 0);
@@ -1142,7 +1142,7 @@ JNIEXPORT jlong JNICALL Java_ai_rapids_cudf_ColumnView_binaryOpVS(JNIEnv *env, j
 
     cudf::binary_operator op = static_cast<cudf::binary_operator>(int_op);
     std::unique_ptr<cudf::column> result =
-        cudf::experimental::binary_operation(*lhs, *rhs, op, n_data_type);
+        cudf::binary_operation(*lhs, *rhs, op, n_data_type);
     return reinterpret_cast<jlong>(result.release());
   }
   CATCH_STD(env, 0);
diff --git a/java/src/main/native/src/ScalarJni.cpp b/java/src/main/native/src/ScalarJni.cpp
index b43b32e6be..cd3f23beac 100644
--- a/java/src/main/native/src/ScalarJni.cpp
+++ b/java/src/main/native/src/ScalarJni.cpp
@@ -468,7 +468,7 @@ JNIEXPORT jlong JNICALL Java_ai_rapids_cudf_Scalar_binaryOpSV(JNIEnv *env, jclas
 
     cudf::binary_operator op = static_cast<cudf::binary_operator>(int_op);
     std::unique_ptr<cudf::column> result =
-        cudf::experimental::binary_operation(*lhs, *rhs, op, n_data_type);
+        cudf::binary_operation(*lhs, *rhs, op, n_data_type);
     return reinterpret_cast<jlong>(result.release());
   }
   CATCH_STD(env, 0);

cpp/tests/binaryop/binop-compiled-fixed_point-test.cpp

karthikeyann · 2021-08-19T08:51:14Z

rerun tests

karthikeyann · 2021-08-19T16:44:34Z

rerun tests

galipremsagar · 2021-08-20T04:01:45Z

rerun tests

galipremsagar · 2021-08-20T16:09:52Z

rerun tests

karthikeyann · 2021-08-22T15:05:07Z

rerun tests

…le_compiled_binops

karthikeyann · 2021-08-23T14:34:19Z

@gpucibot merge

karthikeyann added 4 commits July 14, 2021 21:08

add experimental::detail::binary_operation

1ccc242

use experimental::detail::binary_operation in libcudf functions

0734a85

use compiled experimental::binary_operation in python

d65fe3a

use compiled experimental::binary_operation in java

010bd4a

karthikeyann requested review from a team as code owners July 14, 2021 15:50

karthikeyann requested review from trxcllnt, nvdbaranec and rgsl888prabhu July 14, 2021 15:50

github-actions bot added Java Affects Java cuDF API. Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Jul 14, 2021

karthikeyann added breaking Breaking change feature request New feature or request labels Jul 14, 2021

revans2 approved these changes Jul 14, 2021

View reviewed changes

nvdbaranec reviewed Jul 14, 2021

View reviewed changes

add logical_and logical_or to bool output type list

0f35ff2

karthikeyann added 4 - Needs cuDF (Python) Reviewer 4 - Needs Review Waiting for reviewer to review or respond 3 - Ready for Review Ready for review by team labels Jul 14, 2021

add NullMax, NullMin string unit tests

67c94ab

galipremsagar removed the 4 - Needs cuDF (Python) Reviewer label Jul 22, 2021

harrism approved these changes Jul 27, 2021

View reviewed changes

Merge branch 'branch-21.10' of github.com:rapidsai/cudf into fea-enab…

cd3c244

…le_compiled_binops

remove unused include

50e4437

jlowe mentioned this pull request Aug 9, 2021

[DOC] Remove JIT cache setup specialization NVIDIA/spark-rapids#3174

Closed

reorg code, jit::binary_operation, compiled as cudf::binary_operation

0f5d3cf

github-actions bot added the CMake CMake build issue label Aug 16, 2021

review comments jni, cython fix as well

35c9ebb

github-actions bot removed the Java Affects Java cuDF API. label Aug 16, 2021

karthikeyann added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 4 - Needs Review Waiting for reviewer to review or respond 4 - Needs cuDF (Java) Reviewer labels Aug 17, 2021

remove debug rmm_mode=cuda cmake change

61e22bd

github-actions bot removed the CMake CMake build issue label Aug 17, 2021

harrism reviewed Aug 17, 2021

View reviewed changes

cpp/tests/binaryop/binop-compiled-fixed_point-test.cpp Outdated Show resolved Hide resolved

delete commented code

62b52d7

Merge branch 'branch-21.10' of github.com:rapidsai/cudf into fea-enab…

e7e81ce

…le_compiled_binops

rapids-bot bot merged commit 332dedf into rapidsai:branch-21.10 Aug 23, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable compiled binary ops in libcudf, python and java #8741

Enable compiled binary ops in libcudf, python and java #8741

karthikeyann commented Jul 14, 2021 •

edited

Loading

revans2 left a comment

nvdbaranec Jul 14, 2021

karthikeyann Jul 14, 2021

nvdbaranec Jul 15, 2021

karthikeyann Jul 15, 2021

karthikeyann Jul 15, 2021 •

edited

Loading

karthikeyann Jul 15, 2021

karthikeyann commented Jul 14, 2021

codecov bot commented Jul 14, 2021 •

edited

Loading

harrism commented Jul 14, 2021

karthikeyann commented Jul 15, 2021

galipremsagar commented Jul 15, 2021 •

edited

Loading

karthikeyann commented Jul 20, 2021

harrism commented Jul 27, 2021 •

edited

Loading

karthikeyann commented Aug 1, 2021

revans2 commented Aug 16, 2021

karthikeyann commented Aug 19, 2021

karthikeyann commented Aug 19, 2021

galipremsagar commented Aug 20, 2021

galipremsagar commented Aug 20, 2021

karthikeyann commented Aug 22, 2021

karthikeyann commented Aug 23, 2021

Enable compiled binary ops in libcudf, python and java #8741

Enable compiled binary ops in libcudf, python and java #8741

Conversation

karthikeyann commented Jul 14, 2021 • edited Loading

revans2 left a comment

Choose a reason for hiding this comment

nvdbaranec Jul 14, 2021

Choose a reason for hiding this comment

karthikeyann Jul 14, 2021

Choose a reason for hiding this comment

nvdbaranec Jul 15, 2021

Choose a reason for hiding this comment

karthikeyann Jul 15, 2021

Choose a reason for hiding this comment

karthikeyann Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

karthikeyann Jul 15, 2021

Choose a reason for hiding this comment

karthikeyann commented Jul 14, 2021

codecov bot commented Jul 14, 2021 • edited Loading

Codecov Report

harrism commented Jul 14, 2021

karthikeyann commented Jul 15, 2021

galipremsagar commented Jul 15, 2021 • edited Loading

karthikeyann commented Jul 20, 2021

harrism commented Jul 27, 2021 • edited Loading

karthikeyann commented Aug 1, 2021

revans2 commented Aug 16, 2021

karthikeyann commented Aug 19, 2021

karthikeyann commented Aug 19, 2021

galipremsagar commented Aug 20, 2021

galipremsagar commented Aug 20, 2021

karthikeyann commented Aug 22, 2021

karthikeyann commented Aug 23, 2021

karthikeyann commented Jul 14, 2021 •

edited

Loading

karthikeyann Jul 15, 2021 •

edited

Loading

codecov bot commented Jul 14, 2021 •

edited

Loading

galipremsagar commented Jul 15, 2021 •

edited

Loading

harrism commented Jul 27, 2021 •

edited

Loading