Remove validation that requires introspection #11938

vyasr · 2022-10-17T22:38:18Z

Description

This PR removes optional validation for some APIs. Performing these validations requires data introspection, which we do not want. This PR resolves #5505.

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/include/cudf/copying.hpp

cpp/tests/copying/scatter_tests.cpp

harrism

Looks good. Just a couple of comments.

cpp/include/cudf/filling.hpp

cpp/include/cudf/copying.hpp

galipremsagar

Cython/Python approval.

cpp/include/cudf/detail/scatter.hpp

davidwendt

Have to love negative lines of code.

vyasr · 2022-10-18T20:59:23Z

rerun tests

codecov · 2022-10-18T22:52:33Z

Codecov Report

Base: 87.40% // Head: 86.87% // Decreases project coverage by -0.53% ⚠️

Coverage data is based on head (aafe0d2) compared to base (f72c4ce).
Patch coverage: 88.75% of modified lines in pull request are covered.

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

Additional details and impacted files

@@               Coverage Diff                @@
##           branch-22.12   #11938      +/-   ##
================================================
- Coverage         87.40%   86.87%   -0.54%     
================================================
  Files               133      133              
  Lines             21833    21900      +67     
================================================
- Hits              19084    19025      -59     
- Misses             2749     2875     +126

Impacted Files	Coverage Δ
python/cudf/cudf/core/dataframe.py	`93.73% <ø> (-0.05%)`	⬇️
python/cudf/cudf/core/indexed_frame.py	`92.03% <ø> (ø)`
python/cudf/cudf/core/udf/__init__.py	`50.00% <ø> (ø)`
python/cudf/cudf/io/orc.py	`92.94% <ø> (-0.09%)`	⬇️
python/cudf/cudf/testing/dataset_generator.py	`72.83% <ø> (-0.42%)`	⬇️
...thon/dask_cudf/dask_cudf/tests/test_distributed.py	`18.86% <ø> (+4.94%)`	⬆️
python/cudf/cudf/core/_base_index.py	`82.20% <43.75%> (-3.35%)`	⬇️
python/cudf/cudf/io/text.py	`91.66% <66.66%> (-8.34%)`	⬇️
python/strings_udf/strings_udf/__init__.py	`84.31% <76.00%> (-12.57%)`	⬇️
python/cudf/cudf/core/index.py	`92.96% <95.71%> (+0.33%)`	⬆️
... and 24 more

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.

vyasr · 2022-10-18T23:07:55Z

@jlowe I can fix the java errors, but before I do so do you know if this removal will cause any problems? Does anything in the Spark plugin etc currently rely on this behavior that might need to add in an extra layer to do the check now?

jlowe · 2022-10-19T13:55:50Z

Does anything in the Spark plugin etc currently rely on this behavior that might need to add in an extra layer to do the check now?

No, I don't see any cases where the plugin is relying on the check for proper behavior. However this change will break the Java APIs and thus the Spark plugin build when it merges. I'll work on a Spark plugin PR that can be tested against this one to help avoid surprises and reduce the time the plugin build is broken.

vyasr · 2022-10-19T22:11:48Z

Awesome thanks @jlowe. What is the best path forward here? Should I go ahead and update the JNI code in this PR so that you can build directly against it, or is that not something that you need?

jlowe · 2022-10-19T22:15:50Z

What is the best path forward here?

Go ahead and make the Java changes in this PR. I've already posted a plugin PR at NVIDIA/spark-rapids#6860 with the assumption the check parameter will be removed from Table.scatter and Table.repeat. After you post the Java changes to this PR, I'll do a build against the cudf PR to verify the Spark plugin is still happy with my plugin changes to fix the build. Assuming all goes well, we'll be ready to quickly fix the Spark plugin build when this PR merges and is published in the nightly snapshot.

vyasr · 2022-10-19T22:47:16Z

Yup I saw the PR. I've made the changes here and TableJni.cpp now compiles for me locally, so I think it's ready.

vyasr · 2022-10-19T22:54:19Z

Ah there appear to be some tests that I need to update. That shouldn't affect your ability to build and test the plugin against this though. I'll get on those tests.

java/src/main/java/ai/rapids/cudf/Table.java

jlowe

Built this against NVIDIA/spark-rapids#6860 and verified the integration tests for joins and arrays passed (i.e.: the places where we use Table.scatter and Table.repeat).

vyasr · 2022-10-20T16:04:32Z

@gpucibot merge

vyasr added 5 commits October 17, 2022 14:39

Move scatter bounds-checking logic into Python.

6900acc

Match old error and remove now unnecessary try except.

0033927

Remove all usage of check_bounds in scatter in C++.

7000465

Remove check_count from repeat in Python.

b639d7f

Remove references to check_count in repeat C++ code.

ba94570

vyasr added 3 - Ready for Review Ready for review by team libcudf Affects libcudf (C++/CUDA) code. improvement Improvement / enhancement to an existing function breaking Breaking change labels Oct 17, 2022

vyasr requested a review from a team as a code owner October 17, 2022 22:38

vyasr self-assigned this Oct 17, 2022

vyasr requested a review from a team as a code owner October 17, 2022 22:38

vyasr requested review from mroeschke, skirui-source, harrism and vuule October 17, 2022 22:38

github-actions bot added the Python Affects Python cuDF API. label Oct 17, 2022

vyasr mentioned this pull request Oct 17, 2022

[DISCUSSION] libcudf should not introspect input data to perform error checking #5505

Closed

davidwendt reviewed Oct 17, 2022

View reviewed changes

cpp/include/cudf/copying.hpp Show resolved Hide resolved

davidwendt reviewed Oct 17, 2022

View reviewed changes

cpp/tests/copying/scatter_tests.cpp Show resolved Hide resolved

harrism approved these changes Oct 18, 2022

View reviewed changes

cpp/include/cudf/filling.hpp Outdated Show resolved Hide resolved

cpp/include/cudf/copying.hpp Show resolved Hide resolved

galipremsagar approved these changes Oct 18, 2022

View reviewed changes

mroeschke approved these changes Oct 18, 2022

View reviewed changes

vyasr added 2 commits October 18, 2022 13:26

Delete one more test.

b8c262b

Address PR comments.

a957527

vyasr requested a review from davidwendt October 18, 2022 20:29

davidwendt reviewed Oct 18, 2022

View reviewed changes

cpp/include/cudf/detail/scatter.hpp Show resolved Hide resolved

davidwendt reviewed Oct 18, 2022

View reviewed changes

cpp/include/cudf/detail/scatter.hpp Show resolved Hide resolved

davidwendt added 2 commits October 18, 2022 16:42

Add UB doc

187c920

Add UB doc to detail::scatter

c1f0e07

davidwendt approved these changes Oct 18, 2022

View reviewed changes

jlowe mentioned this pull request Oct 19, 2022

Adjust to cudf removal of checks in scatter and repeat NVIDIA/spark-rapids#6860

Merged

Update TableJni.

d11e1d7

github-actions bot added the Java Affects Java cuDF API. label Oct 19, 2022

Update Table class.

1ee835e

vyasr requested a review from a team as a code owner October 19, 2022 23:05

vyasr added 2 commits October 19, 2022 16:05

Remove now invalid test.

773854f

Update more function calls in tests.

fba3d58

ttnghia reviewed Oct 20, 2022

View reviewed changes

java/src/main/java/ai/rapids/cudf/Table.java Show resolved Hide resolved

ttnghia reviewed Oct 20, 2022

View reviewed changes

java/src/main/java/ai/rapids/cudf/Table.java Show resolved Hide resolved

Remove remaining javadoc.

aef6e1e

ttnghia approved these changes Oct 20, 2022

View reviewed changes

jlowe approved these changes Oct 20, 2022

View reviewed changes

rapids-bot bot merged commit ff41841 into rapidsai:branch-22.12 Oct 20, 2022

vyasr deleted the task/remove_validation branch October 20, 2022 23:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove validation that requires introspection #11938

Remove validation that requires introspection #11938

vyasr commented Oct 17, 2022 •

edited

Loading

harrism left a comment

galipremsagar left a comment

davidwendt left a comment

vyasr commented Oct 18, 2022

codecov bot commented Oct 18, 2022 •

edited

Loading

vyasr commented Oct 18, 2022

jlowe commented Oct 19, 2022

vyasr commented Oct 19, 2022

jlowe commented Oct 19, 2022

vyasr commented Oct 19, 2022

vyasr commented Oct 19, 2022

jlowe left a comment

vyasr commented Oct 20, 2022

Remove validation that requires introspection #11938

Remove validation that requires introspection #11938

Conversation

vyasr commented Oct 17, 2022 • edited Loading

Description

Checklist

harrism left a comment

Choose a reason for hiding this comment

galipremsagar left a comment

Choose a reason for hiding this comment

davidwendt left a comment

Choose a reason for hiding this comment

vyasr commented Oct 18, 2022

codecov bot commented Oct 18, 2022 • edited Loading

Codecov Report

vyasr commented Oct 18, 2022

jlowe commented Oct 19, 2022

vyasr commented Oct 19, 2022

jlowe commented Oct 19, 2022

vyasr commented Oct 19, 2022

vyasr commented Oct 19, 2022

jlowe left a comment

Choose a reason for hiding this comment

vyasr commented Oct 20, 2022

vyasr commented Oct 17, 2022 •

edited

Loading

codecov bot commented Oct 18, 2022 •

edited

Loading