ci: enable CUDA test compilation in CI (#22)#80
Merged
Conversation
Closes #22 - Uncomment test_cuda in CMakeLists.txt (mark .c source as CUDA language so nvcc compiles it). The test requires a GPU to actually run but compiles on any runner with the CUDA toolkit installed. - test_cuda.c: gracefully skip if no GPU available (rippra_cuda_centroid_init fails), prints SKIP and returns 0 instead of erroring. - Add synthetic data generation and ctest run to CUDA CI job with continue-on-error: true — compiles on every push, runs only on GPU runners. - CUDA README badge already exists.
changed to ../cuda/rippra_cuda.h
nvcc treats .c files as C++ where goto cannot bypass variable initialization. Moved all declarations to the top of main() and removed the skip: label in favor of a skip flag.
- nvcc treats .c files as C++: goto cannot bypass variable initialization - GPU section wrapped in a block so cleanup: label is within the same scope - Skip path (no GPU) does early return instead of goto, avoiding crossing GPU allocs - max_diff_W/max_diff_c moved to common scope within the GPU block
- nvcc (C++ mode) rejects goto bypassing for-loop variable declarations - Split cleanup into 'cleanup' (non-GPU) and 'gpu_cleanup' (GPU) - GPU skip path does direct return (no goto over GPU allocs) - All loop variables declared at function scope, not in for-loop init - gpu_cleanup label placed before any for-loop blocks
- rippra_cuda.h was missing declarations for dm_init, dm_free, and full_pipeline - nvcc linker error when compiling test_cuda.c
sr-857
approved these changes
Jul 8, 2026
sr-857
left a comment
sr-857
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
Thanks for working on enabling CUDA compilation in CI. The overall direction looks good, especially the cleanup restructuring and graceful handling of systems without a GPU. However, I don't think this PR is ready to merge yet because the CUDA Build Check is still failing, which is the primary objective of this change.
Requested Changes
-
Please fix the failing CUDA CI job before merging.
- Since this PR is intended to enable CUDA compilation in CI, the CUDA workflow should pass before merge.
- It would help to identify whether the failure is due to compilation, linking, or runtime.
-
continue-on-errormay hide real failures.- Using
continue-on-error: truefor the CUDA test prevents CI from catching legitimate regressions. - If the intention is only to skip execution when no GPU is available, consider conditionally running the test instead of ignoring failures entirely.
- Using
-
Check the return values of all
cudaMemcpy()calls.- Earlier CUDA API calls are checked, but the final
cudaMemcpy()operations copying results back to the host are not. - These should also be validated for consistency and easier debugging.
- Earlier CUDA API calls are checked, but the final
-
Validate memory allocations.
calloc()andmalloc()return values are currently assumed to succeed.- Even in test code, checking allocation failures improves robustness.
-
Avoid relative include paths if possible.
- Changing
works, but configuring the include directories in CMake would be more maintainable than relying on relative paths.
#include "../cuda/rippra_cuda.h"
- Changing
Positive Notes
- Good use of separate
gpu_cleanupandcleanuplabels to satisfy NVCC/C++ restrictions. - Gracefully skipping execution when no GPU is available is appropriate for CI.
- Marking the test source as CUDA so it is compiled by NVCC is a reasonable approach.
- The refactoring for NVCC compatibility is much cleaner than the previous version.
Once the CUDA CI job passes and the above issues are addressed, this should be in good shape for approval.
- centroid_kernels.cu, dm_kernels.cu, matrix_kernels.cu were not part of any library target - test_cuda (and any CUDA code) got linker errors because these .cu implementations were missing - Also removed unused 'ok' variable from test_cuda.c to silence nvcc warning
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #22
language so nvcc compiles the test as a compile check.
eturn 0) if no GPU available, print SKIP.
\continue-on-error: true\ — compiles every push; runs test only on GPU runners.