-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add macros KVIKIO_EXPECT and KVIKIO_FAIL to improve exception handling #653
Add macros KVIKIO_EXPECT and KVIKIO_FAIL to improve exception handling #653
Conversation
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
/ok to test |
2 similar comments
/ok to test |
/ok to test |
538890e
to
c841e8b
Compare
/ok to test |
/ok to test |
@madsbk @bdice I'm wondering if you have any thought about this CI failure: https://github.com/rapidsai/kvikio/actions/runs/13708284889/job/38339415663?pr=653 Additional observationsKvikIO_CUDA_SUPPORT=ON (default)The tests in
KvikIO_CUDA_SUPPORT=OFFHowever, if I add
|
@@ -52,7 +52,7 @@ using CUstream = struct CUstream_st*; | |||
#define CU_POINTER_ATTRIBUTE_CONTEXT 0 | |||
#define CU_POINTER_ATTRIBUTE_DEVICE_ORDINAL 0 | |||
#define CU_POINTER_ATTRIBUTE_DEVICE_POINTER 0 | |||
#define CU_MEMHOSTREGISTER_PORTABLE 0 | |||
#define CU_MEMHOSTALLOC_PORTABLE 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix ensures a successful build with -DKvikIO_CUDA_SUPPORT=OFF
option.
Related to the merged PR: #637
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, so the cuda_runtime.h
issue is all sorted out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the confusion. I updated my comment above. The cuda_runtime.h
issue still exists.
#define CU_MEMHOSTALLOC_PORTABLE
here is a fix for the C++-part of the build:
# This is successful now
build-kvikio-cpp -DKvikIO_CUDA_SUPPORT=OFF -DBUILD_TESTS=ON -j 16
build-all -DKvikIO_CUDA_SUPPORT=OFF -DBUILD_TESTS=ON -j 16
still fails with the cuda_runtime.h
issue.
} | ||
return {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This return {}
and ditto below is to avoid a compile warning (error):
no return statement in function returning...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks @kingcrimsontianyu
The CI issue on CUDA 11.8 is now fixed. According to test_wheel.sh
So the failed test should have been decorated by the marker PS: The local build fail with |
/merge |
This PR follows cuDF's enhanced exception handling approach and adds the macros
KVIKIO_EXPECT
andKVIKIO_FAIL
to KvikIO.The benefit is that the file name and line number where the exception is thrown are automatically included in the exception message. This also leads to cleaner code.
This PR does not add the stacktrace capability as in cuDF's exception handling. This feature should be added in the future.
Closes #654