Repository navigation
Migrate DeepJIT CUDA integration to the PyTorch stable ABI - #11
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>
…lean under -Werror across CUDA, Ascend, and the shared runtime. Signed-off-by: Chris Leonard <chleonar@redhat.com>
| | **Ascend** | Bisheng and ld.lld compile and link Ascend kernel source; ACL loads and launches kernels. | CANN with `bin/bisheng`, `bin/ld.lld`, and the Ascend `adv_api` headers; ACL and `torch_npu` headers and runtime. | | ||
|
|
||
| The host environment must provide Linux, a C++20 compiler and standard library with `std::format` support, Python, pybind11, and the dependencies for the selected backend. DeepJIT is intended to be embedded into your extension as a header-only dependency. | ||
| The host environment must provide Linux, a C++20 compiler and standard library with `std::format` support, and the dependencies for the selected backend. The default GIL support also requires Python and pybind11; consumers that manage the GIL themselves can disable it as described under [Integration](#integration). DeepJIT is intended to be embedded into your extension as a header-only dependency. CUDA consumers should compile with `TORCH_TARGET_VERSION=0x020d000000000000` and `USE_CUDA`; this targets PyTorch's stable ABI with PyTorch 2.13 as the minimum runtime version. |
There was a problem hiding this comment.
@cleonard530 if we use the old cuda stream shim, what's the min runtime version we can target?
There was a problem hiding this comment.
We could target 2.10 then (TORCH_TARGET_VERSION=0x020a000000000000)
|
I believe this PR is ready for review @LyricZhao |
oraluben
left a comment
There was a problem hiding this comment.
Thanks @cleonard530 !
Overall LGTM. I think we'd prefer the minimal pytorch version to be 2.10 (at least for CUDA), and some minor questions.
| std::shared_ptr<Runtime> process_test_runtime; | ||
| std::shared_ptr<Runtime> gil_test_runtime; | ||
|
|
||
| cudaStream_t get_stream_from_pool(const int32_t device_index) { |
There was a problem hiding this comment.
If i'm not mistaken, upstream torch is planning to add stable api for these two, so maybe we could move them to a separate util file for future removal?
There was a problem hiding this comment.
Yes, we can definitely move this and similar torch functions to a util file!
|
|
||
| pybind11_add_module(_C csrc/python_api.cpp) | ||
| target_compile_definitions(_C PRIVATE | ||
| TORCH_TARGET_VERSION=0x020d000000000000 |
There was a problem hiding this comment.
It might be better to target 2.10 as minimal requirement as it's feasible. For ascend it's ok to target 2.13 if it's necessary.
There was a problem hiding this comment.
Done!
To move to torch 2.10, we needed to replace
auto stream = torch::stable::accelerator::getCurrentStream(device_index);
return static_cast<CUstream>(stream.nativeHandle());
with
void* stream_ptr = nullptr;
TORCH_ERROR_CODE_CHECK(
aoti_torch_get_current_cuda_stream(device_index, &stream_ptr));
return static_cast<CUstream>(stream_ptr);
which requires the macro USE_CUDA to be defined since that method is guarded in PyTorch, so I added this guard in the new include/deep_jit/backend/cuda/stable_torch_utils.h file for it to work
#ifndef USE_CUDA
#define DEEP_JIT_UNDEFINE_USE_CUDA
#define USE_CUDA
#endif
#include <torch/csrc/stable/accelerator.h>
#include <torch/csrc/stable/c/shim.h>
#include <torch/headeronly/util/shim_utils.h>
#ifdef DEEP_JIT_UNDEFINE_USE_CUDA
#undef USE_CUDA
#undef DEEP_JIT_UNDEFINE_USE_CUDA
#endif
The final #ifdef block is so the rest of the build is not polluted with the USE_CUDA macro.
|
|
||
| namespace deep_jit { | ||
|
|
||
| class GilScopedRelease { |
There was a problem hiding this comment.
I'm not very familiar with the nogil support of pytorch upstream, would you mind elaborate a little bit? Does this mean that DeepJIT itself supports nogil? Or downstream libraries (e.g. deepgemm / deepep) needs extra changes.
There was a problem hiding this comment.
When kernels are registered with TORCH_LIBRARY, the operator is registered through PyTorch's dispatcher mechanism (basically becomes just like any other PyTorch op) and can be invoked from Python through torch.ops.<op_name>. When a PyTorch operator is invoked from Python, PyTorch releases the GIL before executing the operator through the dispatcher, so by the time it reaches GilScopedRelease gil_release here, the GIL has already been released and that guard is effectively a no-op. This means that DeepJIT doesn't need to explicitly release the GIL for downstream operators that are registered through TORCH_LIBRARY/STABLE_TORCH_LIBRARY.
Currently, DeepGEMM dispatches through pybind, so the explicit GIL release is still required there. We are working on moving DeepGEMM's registration from pybind to TORCH_LIBRARY (see vllm-project/DeepGEMM#15 and deepseek-ai/DeepGEMM#393). We need that migration to happen before we can remove the explicit GIL release from this path, which is why we added a macro flag DJ_DISABLE_GIL rather than removing the release outright. This allows existing downstream users that still dispatch through pybind to continue working correctly while we migrate.
I'm not as familiar with DeepEP, so if it still relies on pybind or otherwise requires the GIL to be released at this point, that flag would need to remain for it as well, or any other downstream task that aren't registered through TORCH_LIBRARY.
Once the downstream libraries have all moved from pybind to TORCH_LIBRARY/STABLE_TORCH_LIBRARY, and assuming they don't otherwise interact with Python while executing their kernels, we should be able to remove the explicit GilScopedRelease entirely.
Let me know if you have any other questions or need anything clarified. Thanks!
Signed-off-by: Chris Leonard <chleonar@redhat.com>
Summary
DJ_DISABLE_GILwhich can be used to makeGilScopedReleasea no-op. The reason for this is because if we are using ops registered withTORCH_LIBRARY, the ops invoked throughtorch.opsreleases the GIL already. The current implementation is the only thing inincludethat is stopping vllm-project/DeepGEMM/15 from using the CPython Stable ABI (abi3) (we remove the<deep_jit/python_api.hpp>directly in that PR, so it didn't effect the CPython Stable ABI).The Ascend build is unchanged for now, but a followup PR could migrate this to the Torch Stable ABI as well. This might require the torch minimum version staying 2.13 though, as
c10_npu::getCurrentNPUStream()would need the shim invoked thoughstream.nativeHandle(), whereauto stream = torch::stable::accelerator::getCurrentStream(device_index);.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.