-
Notifications
You must be signed in to change notification settings - Fork 55
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
Cython refactoring #434
Cython refactoring #434
Conversation
a418f81
to
ad06450
Compare
driver_properties.pyx future.pyx cpp_StreamFuture buffer.pyx defaults.pyx remove kvikio_cxx_api.pxd file_handle.pyx parse_buffer_argument use abs import path
ad06450
to
c17ab8f
Compare
|
||
|
||
cdef extern from "cuda.h": | ||
ctypedef void* CUstream |
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.
We should probably cimport the Cython stream type from cuda-python here.
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.
Using from cuda.ccuda cimport CUstream
, I get:
building kvikio...
Using pip 24.2 from /home/mkristensen/apps/miniforge3/envs/kvikio-0809/lib/python3.11/site-packages/pip (python 3.11)
Obtaining file:///home/mkristensen/repos/kvikio/python/kvikio
Running command Checking if build backend supports build_editable
Checking if build backend supports build_editable ... done
Running command Preparing editable metadata (pyproject.toml)
*** scikit-build-core 0.10.1 using CMake 3.30.2 (metadata_editable)
2024-08-12 15:43:46,567 - scikit_build_core - WARNING - No license files found, set wheel.license-files to [] to suppress this warning
Preparing editable metadata (pyproject.toml) ... done
Building wheels for collected packages: kvikio
Running command Building editable for kvikio (pyproject.toml)
*** scikit-build-core 0.10.1 using CMake 3.30.2 (editable)
2024-08-12 15:43:46,953 - scikit_build_core - WARNING - No license files found, set wheel.license-files to [] to suppress this warning
*** Configuring CMake...
2024-08-12 15:43:46,966 - scikit_build_core - WARNING - Unsupported CMAKE_ARGS ignored: -DCMAKE_BUILD_TYPE=Release
2024-08-12 15:43:47,066 - scikit_build_core - WARNING - Unsupported CMAKE_ARGS ignored: -DCMAKE_BUILD_TYPE=Release
loading initial cache file build/cp311-cp311-linux_x86_64/CMakeInit.txt
-- KvikIO: Found cuFile Batch API: TRUE
-- KvikIO: Found cuFile Stream API: TRUE
-- Found nvcomp: /home/mkristensen/apps/miniforge3/envs/kvikio-0809/lib/cmake/nvcomp (found version 3.0.6)
-- Configuring done (1.2s)
-- Generating done (0.0s)
-- Build files have been written to: /home/mkristensen/repos/kvikio/python/kvikio/build/cp311-cp311-linux_x86_64
2024-08-12 15:43:48,280 - scikit_build_core - WARNING - Unsupported CMAKE_ARGS ignored: -DCMAKE_BUILD_TYPE=Release
*** Building project with Ninja...
[1/3] Transpiling file_handle.pyx to file_handle.cxx
[2/3] Building CXX object kvikio/_lib/CMakeFiles/file_handle.dir/file_handle.cxx.o
FAILED: kvikio/_lib/CMakeFiles/file_handle.dir/file_handle.cxx.o
/home/mkristensen/apps/miniforge3/envs/kvikio-0809/bin/x86_64-conda-linux-gnu-c++ -DBS_THREAD_POOL_ENABLE_PAUSE=1 -DCUFILE_FOUND -DKVIKIO_CUDA_FOUND -DKVIKIO_CUFILE_BATCH_API_FOUND -DKVIKIO_CUFILE_FOUND -DKVIKIO_CUFILE_STREAM_API_FOUND -Dfile_handle_EXPORTS -isystem /home/mkristensen/apps/miniforge3/envs/kvikio-0809/include/python3.11 -isystem /home/mkristensen/apps/miniforge3/envs/kvikio-0809/include/gdeflate -fvisibility-inlines-hidden -fmessage-length=0 -march=nocona -mtune=haswell -ftree-vectorize -fPIC -fstack-protector-strong -fno-plt -O2 -ffunction-sections -pipe -isystem /home/mkristensen/apps/miniforge3/envs/kvikio-0809/include -I/home/mkristensen/apps/miniforge3/envs/kvikio-0809/targets/x86_64-linux/include -L/home/mkristensen/apps/miniforge3/envs/kvikio-0809/targets/x86_64-linux/lib -L/home/mkristensen/apps/miniforge3/envs/kvikio-0809/targets/x86_64-linux/lib/stubs -O3 -DNDEBUG -fPIC -pthread -MD -MT kvikio/_lib/CMakeFiles/file_handle.dir/file_handle.cxx.o -MF kvikio/_lib/CMakeFiles/file_handle.dir/file_handle.cxx.o.d -o kvikio/_lib/CMakeFiles/file_handle.dir/file_handle.cxx.o -c /home/mkristensen/repos/kvikio/python/kvikio/build/cp311-cp311-linux_x86_64/kvikio/_lib/file_handle.cxx
/home/mkristensen/repos/kvikio/python/kvikio/build/cp311-cp311-linux_x86_64/kvikio/_lib/file_handle.cxx:1238:10: fatal error: cudaProfiler.h: No such file or directory
1238 | #include "cudaProfiler.h"
| ^~~~~~~~~~~~~~~~
compilation terminated.
ninja: build stopped: subcommand failed.
*** CMake build failed
Do we really need cudaProfiler?
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.
We want to use Cython types from cuda-python for consistency, but cuda-python exposes quite a few headers that may not be in the dependency list. Can you try adding a conda dependency on cuda-profiler-api (perhaps with a comment that it is needed by cuda-python)?
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.
installing cuda-profiler-api
works, but there doesn't seems to be a cuda-profiler-api<12
package?
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.
You will need that package only for building conda packages for CUDA 12. For CUDA 11 (conda/wheel) and CUDA 12 wheel builds, it is expected that the system has a CTK installation that provides all CUDA headers.
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.
Fails here: https://github.com/rapidsai/kvikio/actions/runs/10354623316/job/28660538187?pr=434#step:9:233
Maybe, this is something we should look at in a follow up PR? And roll back d965e66 ?
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.
I have reverted the change, let's do it in another PR. I like to keep this PR to refactoring
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.
Let's raise an issue to track. @bdice would you be able to help with this?
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.
No, I don’t think I can take this on. It is a low priority.
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.
For clarity just meant raising an issue. That said, if we don't think it is worth an issue, it is likely not worth doing either
Co-authored-by: Bradley Dice <bdice@bradleydice.com>
This reverts commit d965e66.
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.
No further comments.
/merge |
extern
to where there are used instead of having a single.pxd
file.libkvikio.pyx
into multiple files/modules