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

Handle some zero-sized corner cases in dlpack interop #11449

Merged
merged 22 commits into from
Sep 6, 2022

Conversation

wence-
Copy link
Contributor

@wence- wence- commented Aug 3, 2022

Description

Previously, we did not explicitly check for zero-sized inputs in to/from_dlpack.
cupy>=11 changes behaviour in how it provides information for zero-sized arrays
(strides of empty tensors were previously reported as one, but now are zero).
This broke some of the validity checking in from_dlpack. To handle such cases,
check the shape of the incoming tensor as well as strides.

Checklist

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

@wence- wence- requested review from a team as code owners August 3, 2022 10:01
@wence- wence- requested review from bdice, charlesbluca and vuule August 3, 2022 10:01
@github-actions github-actions bot added Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Aug 3, 2022
@wence- wence- added 3 - Ready for Review Ready for review by team non-breaking Non-breaking change and removed libcudf Affects libcudf (C++/CUDA) code. Python Affects Python cuDF API. labels Aug 3, 2022
cpp/src/interop/dlpack.cpp Show resolved Hide resolved
cpp/src/interop/dlpack.cpp Show resolved Hide resolved
cpp/tests/interop/dlpack_test.cpp Show resolved Hide resolved
cpp/tests/interop/dlpack_test.cpp Show resolved Hide resolved
python/cudf/cudf/io/dlpack.py Show resolved Hide resolved
@wence-
Copy link
Contributor Author

wence- commented Aug 3, 2022

cc: @shwina, @leofang

@codecov
Copy link

codecov bot commented Aug 3, 2022

Codecov Report

❗ No coverage uploaded for pull request base (branch-22.10@8ad0290). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@               Coverage Diff               @@
##             branch-22.10   #11449   +/-   ##
===============================================
  Coverage                ?   86.41%           
===============================================
  Files                   ?      145           
  Lines                   ?    22991           
  Branches                ?        0           
===============================================
  Hits                    ?    19868           
  Misses                  ?     3123           
  Partials                ?        0           

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.

@wence-
Copy link
Contributor Author

wence- commented Aug 3, 2022

This is the cudf part of the changes necessary to allow using cupy 11 and later.

@vuule vuule added the bug Something isn't working label Aug 3, 2022
Copy link
Contributor

@shwina shwina left a comment

Choose a reason for hiding this comment

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

Python looks good!

Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Thank you for the informative author comments throughout. It helped my review a lot to have a second level of description to validate my understanding of what the changes were doing. I have one minor suggestion for improving a comment.

cpp/tests/interop/dlpack_test.cpp Outdated Show resolved Hide resolved
@github-actions github-actions bot added conda Python Affects Python cuDF API. libcudf Affects libcudf (C++/CUDA) code. labels Aug 18, 2022
@wence-
Copy link
Contributor Author

wence- commented Aug 18, 2022

I've pushed a change to the conda environment to check if running with cupy 11 actually works. I've converted to draft so this is not accidentally merged (because it might be necessary to synchronise other cupy-11-related fixes elsewhere)

@wence- wence- requested a review from a team as a code owner August 25, 2022 16:06
@wence-
Copy link
Contributor Author

wence- commented Aug 25, 2022

I think everything is now good. Should we drop all the conda environment changes and do those separately?

Copy link
Contributor

@galipremsagar galipremsagar left a comment

Choose a reason for hiding this comment

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

Looks ready to go, just need to revert the temporary changes.

ci/gpu/build.sh Outdated Show resolved Hide resolved
ci/gpu/build.sh Outdated Show resolved Hide resolved
@galipremsagar
Copy link
Contributor

@wence- After the temporary commits are reverted and this integration PR(rapidsai/integration#528) is merged we will have to wait a couple of hours for the new env/images to be shipped so that CI can pass.

@github-actions github-actions bot removed the gpuCI label Aug 25, 2022
We use doctest.ELLIPSIS in the doctest runner option flags, so
foo...bar will match any string that starts with foo and ends with
bar.
@wence- wence- added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team 5 - DO NOT MERGE Hold off on merging; see PR for details labels Aug 26, 2022
@wence-
Copy link
Contributor Author

wence- commented Sep 1, 2022

Note, cupy.from_dlpack which I use in favour of the deprecated cupy.fromDLpack (deprecated in cupy 10) in the tests does not exist in cupy<10. Right now cudf asks for cupy>=9.5. What's the right way to support this (requirement for backwards compat is only in test usage).

@bdice
Copy link
Contributor

bdice commented Sep 1, 2022

Note, cupy.from_dlpack which I use in favour of the deprecated cupy.fromDLpack (deprecated in cupy 10) in the tests does not exist in cupy<10. Right now cudf asks for cupy>=9.5. What's the right way to support this (requirement for backwards compat is only in test usage).

@wence- You can use packaging.version.parse to parse versions and either conditionally skip a test if the version is too low or choose the appropriate function for each range of compatible versions. Examples:

from packaging import version
PANDAS_VERSION = version.parse(pd.__version__)

if parse_version(dask.__version__) > parse_version("2021.07.0"):

@wence-
Copy link
Contributor Author

wence- commented Sep 1, 2022

Merged trunk to hopefully fix build errors

@wence-
Copy link
Contributor Author

wence- commented Sep 1, 2022

Argh, shouldn't have merged main :(.

@wence- wence- force-pushed the wence/fix/dlpack-empty branch from 5a61e46 to e24dc97 Compare September 1, 2022 15:36
@wence-
Copy link
Contributor Author

wence- commented Sep 1, 2022

@galipremsagar sorry, we both fixed the same thing and I pushed over the top of you

@galipremsagar
Copy link
Contributor

@gpucibot merge

@rapids-bot rapids-bot bot merged commit bc7109e into rapidsai:branch-22.10 Sep 6, 2022
@wence- wence- deleted the wence/fix/dlpack-empty branch September 6, 2022 20:58
@vyasr vyasr mentioned this pull request Nov 1, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge bug Something isn't working libcudf Affects libcudf (C++/CUDA) code. non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants