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

More error checking in from_dlpack #10850

Merged

Conversation

wence-
Copy link
Contributor

@wence- wence- commented May 13, 2022

The implementation copying a full column at a time relies on
unit-stride, column-major data in the incoming dlpack tensor. Check
these preconditions and report error messages, rather than either
silently producing bad data (unit-stride, row-major 2D tensors) or
invalid memory accesses (non-unit-stride in the fastest-varying
dimension). Closes #10754 by providing an explicit error message for
this unsupported case.

wence- added 2 commits May 13, 2022 06:16
The implementation copying a full column at a time relies on
unit-stride, column-major data in the incoming dlpack tensor. Check
these preconditions and report error messages, rather than either
silently producing bad data (unit-stride, row-major 2D tensors) or
invalid memory accesses (non-unit-stride in the fastest-varying
dimension). Closes rapidsai#10754 by providing an explicit error message for
this unsupported case.
@wence- wence- requested a review from a team as a code owner May 13, 2022 13:59
@github-actions github-actions bot added the libcudf Affects libcudf (C++/CUDA) code. label May 13, 2022
@wence- wence- added the non-breaking Non-breaking change label May 13, 2022
@wence-
Copy link
Contributor Author

wence- commented May 13, 2022

I would like to add some tests for this, but need a little guidance on where the best place is/how to do so. I guess since cupy is a hard dependency I can just test at the python level?

@PointKernel
Copy link
Member

@wence- I'm not familiar with this part of the code but looks like we can custom a tensor (https://github.com/rapidsai/cudf/blob/branch-22.06/cpp/tests/interop/dlpack_test.cpp#L345-L351) and verify if the function can throw the expected message via CUDF_EXPECT_THROW_MESSAGE (a simple use case here).

@PointKernel PointKernel added the improvement Improvement / enhancement to an existing function label May 13, 2022
@wence-
Copy link
Contributor Author

wence- commented May 13, 2022

@wence- I'm not familiar with this part of the code but looks like we can custom a tensor (https://github.com/rapidsai/cudf/blob/branch-22.06/cpp/tests/interop/dlpack_test.cpp#L345-L351) and verify if the function can throw the expected message via CUDF_EXPECT_THROW_MESSAGE (a simple use case here).

Thanks, although I am loathe to introduce testing of specific error messages, in light of #10632 and the in-progress #10637.

@PointKernel
Copy link
Member

@wence- I'm not familiar with this part of the code but looks like we can custom a tensor (https://github.com/rapidsai/cudf/blob/branch-22.06/cpp/tests/interop/dlpack_test.cpp#L345-L351) and verify if the function can throw the expected message via CUDF_EXPECT_THROW_MESSAGE (a simple use case here).

Thanks, although I am loathe to introduce testing of specific error messages, in light of #10632 and the in-progress #10637.

EXPECT_THROW may be what you need then.

cpp/src/interop/dlpack.cpp Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented May 13, 2022

Codecov Report

Merging #10850 (4b9743f) into branch-22.06 (9def28c) will increase coverage by 0.09%.
The diff coverage is n/a.

@@               Coverage Diff                @@
##           branch-22.06   #10850      +/-   ##
================================================
+ Coverage         86.23%   86.32%   +0.09%     
================================================
  Files               143      144       +1     
  Lines             22546    22656     +110     
================================================
+ Hits              19442    19558     +116     
+ Misses             3104     3098       -6     
Impacted Files Coverage Δ
python/dask_cudf/dask_cudf/backends.py 85.51% <0.00%> (-0.94%) ⬇️
python/cudf/cudf/core/scalar.py 89.01% <0.00%> (-0.31%) ⬇️
python/dask_cudf/dask_cudf/io/tests/test_s3.py 96.05% <0.00%> (-0.15%) ⬇️
python/cudf/cudf/io/__init__.py 100.00% <0.00%> (ø)
python/cudf/cudf/core/udf/typing.py 97.54% <0.00%> (ø)
python/dask_cudf/dask_cudf/tests/test_groupby.py 100.00% <0.00%> (ø)
python/cudf/cudf/core/missing.py 100.00% <0.00%> (ø)
python/cudf/cudf/core/column/column.py 89.77% <0.00%> (+0.01%) ⬆️
python/cudf/cudf/core/column/numerical_base.py 98.91% <0.00%> (+0.01%) ⬆️
python/cudf/cudf/core/_internals/where.py 94.73% <0.00%> (+0.03%) ⬆️
... and 13 more

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 9def28c...4b9743f. Read the comment docs.

@wence-
Copy link
Contributor Author

wence- commented May 16, 2022

EXPECT_THROW may be what you need then.

Thanks, done.

Copy link
Contributor

@davidwendt davidwendt left a comment

Choose a reason for hiding this comment

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

There are a couple of instances (lines 169 and 174) near these changes where the CUDF_EXPECTS text reads
greater than or equal-to
Could you remove the hyphen?

@wence-
Copy link
Contributor Author

wence- commented May 16, 2022

There are a couple of instances (lines 169 and 174) near these changes where the CUDF_EXPECTS text reads greater than or equal-to Could you remove the hyphen?

Done in 4b9743f

@PointKernel
Copy link
Member

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 9e004c3 into rapidsai:branch-22.06 May 16, 2022
@wence- wence- deleted the wence/from_dlpack-error-checking branch May 16, 2022 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Improvement / enhancement to an existing function libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] cudf.from_dlpack() is not compatible with TF to_dlpack()
3 participants