Migrate DeepJIT CUDA integration to the PyTorch stable ABI - #11
cleonard530 wants to merge 6 commits into
Conversation
janeyx99
left a comment
There was a problem hiding this comment.
Thanks for looking into this! Overall I’m glad we have the shims for the APIs but we should upstream the headeronly bits for better UX.
Could you add a benchmark on a deepJIT workload to validate this change?
| inline CUstream get_current_cuda_stream(const int32_t device_index) { | ||
| void* stream_ptr = nullptr; | ||
| TORCH_ERROR_CODE_CHECK( | ||
| aoti_torch_get_current_cuda_stream(device_index, &stream_ptr)); |
There was a problem hiding this comment.
For more recent torch, we should have a more high level better UX API for this, no?
There was a problem hiding this comment.
Yes, we definitely have this for torch >= 2.13. I wasn't sure how restrictive we wanted to be here, but seeing your comment below we can update this.
| cudaStream_t get_stream_from_pool(const int32_t device_index) { | ||
| void* stream = nullptr; | ||
| TORCH_ERROR_CODE_CHECK( | ||
| torch_get_cuda_stream_from_pool(false, device_index, &stream)); |
There was a problem hiding this comment.
Would be good to upstream a high level C++ wrapper API for this one
|
|
||
| explicit TorchCUDAStreamGuard(cudaStream_t stream, int32_t device_index) { | ||
| TORCH_ERROR_CODE_CHECK( | ||
| aoti_torch_create_cuda_stream_guard( |
| source_path.write_text(f'#include <{header.as_posix()}>\n', encoding='utf-8') | ||
| command = [ | ||
| os.environ.get('CXX', 'c++'), '-std=c++20', '-fsyntax-only', '-Werror', | ||
| '-DTORCH_TARGET_VERSION=0x020a000000000000', '-DUSE_CUDA', |
Will do! |
…uda stream instead of C shim directly
|
@janeyx99, I added the benchmark results to the description. |
mehmetoguzderin
left a comment
There was a problem hiding this comment.
Code seems to use different target version at different places, is that intentional? Thanks a lot in advance.
Co-authored-by: Mehmet Oguz Derin <mehmetoguzderin@mehmetoguzderin.com>
Co-authored-by: Mehmet Oguz Derin <mehmetoguzderin@mehmetoguzderin.com>
No that was not intentional. Thanks for catching that! |
|
Thank you @cleonard530 for the clarification & update! |
…dRelease used when this macro is set Signed-off-by: Chris Leonard <chleonar@redhat.com>
Summary
The Ascend build is unchanged.
Note: The CUDA extension uses PyTorch’s stable ABI, but it is not CPython ABI-stable because DeepJIT’s GIL helper still depends on pybind11 and CPython-specific APIs. Separate builds are required per Python version.
cc @janeyx99, @Harry-Chen
Testing
python tests/test_cuda.py: All DeepJIT tests passedran torch-abi-audit on minimal extension example (found here 36a98fc) and got
Most of these stable shims came in from included headers, but some came from
#include <deep_jit/backend/cuda/backend.hpp>, and none of them were unstable.Bencmark
These benchmarks compare main against the stable-ABI branch on the SM90 DeepGEMM DeepJIT paths using the same workloads and report both
first callandwarm calltimings.First callmeasures cold-start end-to-end latency, including JIT compilation, loading, and initial launch overhead in a fresh subprocess with a fresh JIT cache, whilewarm callmeasures steady-state per-launch GPU execution after warmup. For evaluating the specific deep_jit launch-path change,warm callis the more direct signal because it isolates runtime launch behavior from one-time compilation costs. The kernels and parameters shapes used for this benchmark are:fp8_gemm_nt:a shape (4096, 2048),b shape (7168, 2048)grouped_fp8_nt:a shape (3712, 2048),b shape (4, 4096, 2048),grouped_layout shape (3712)tf32_hc_prenorm_gemm:a shape (4096, 7168),b shape (24, 7168),num_splits=16fp8_mqa_logits:seq=512kv=4096h=32d=128First Call
Warm Calls (average over 500 calls)
Across the SM90 DeepJIT benchmark cases, the stable-ABI branch runtime is essentially the same as main.