[Do not merge!] [Build] Migrate vendored DeepGEMM from pybind to TORCH_LIBRARY (abi3) - #48962
[Do not merge!] [Build] Migrate vendored DeepGEMM from pybind to TORCH_LIBRARY (abi3)#48962cleonard530 wants to merge 8 commits into
Conversation
| set(_dg_pythons "${Python_EXECUTABLE}") | ||
| endif() | ||
| message(STATUS "DeepGEMM _C will be built for: ${_dg_pythons}") | ||
| message(STATUS "DeepGEMM extension will be built with: ${Python_EXECUTABLE}") |
There was a problem hiding this comment.
This used to link private CPython symbols, so each python needed it's own C.cpython-3XY-….so.
Now, it builds against the CPython Limited/Stable ABI (Py_LIMITED_API), so one _C_extension.abi3.so works across supported CPythons.
| FILES_MATCHING PATTERN "_C.cpython-*.so") | ||
| endforeach() | ||
| add_custom_target(_deep_gemm_C ALL DEPENDS ${_dg_markers}) | ||
| set(_dg_dir "${CMAKE_CURRENT_BINARY_DIR}/deepgemm_C") |
There was a problem hiding this comment.
This block replaced the per-Python build loop with a single abi3 build
| env.pop("DG_SKIP_CUDA_BUILD", None) | ||
|
|
||
| print(f"[build_deepgemm_C] building in {src} with {sys.executable}", flush=True) | ||
| subprocess.check_call( |
There was a problem hiding this comment.
Use deep_gemm's setup.py to build deep_gemm
There was a problem hiding this comment.
This call also gets Python_EXECUTABLE so building DeepGEMM's abi3 extension always tracks whatever Python vLLM is currently being built for.
| required = required_pythons() | ||
| missing = [v for v in required if v not in found] | ||
| print(f"deepgemm _C: found {sorted(found)}, required {required}, missing {missing}") | ||
| sys.exit(1 if missing else 0) |
There was a problem hiding this comment.
This file used to check each required python version in pyproject.toml and failed if matching *.so python version weren't there.
| f"deepgemm vendored binding: shim={shim.is_file()}, " | ||
| f"extensions={[p.name for p in so_files]}" | ||
| ) | ||
| if missing: |
There was a problem hiding this comment.
Now, it checks vllm.third_party.deep_gemm to make sure the _C.py shim and _C_extension*.so are there.
|
Marked as ready to automatically trigger CI run. |
|
@Harry-Chen, the failure for these three test comes from this check which was added between the previously tagged commit and my commit, but not directly due to the migration. We will need to figure out how to handle this before anything is merged (I don't think it will be difficult to fix), but I am planning on putting that off until we decide if we will be using upstream or the vllm-project/DeepGEMM, which doesn't have the check. |
I see. I think currently we are using https://github.com/deepseek-ai/DeepGEMM/tree/nv_dev, which adds sm120 support (from NVIDIA) on top of upstream master. |
Oh yes, you're right @Harry-Chen. Once we agree we are in a good spot, should we try to merge into this branch? Do you know how receptive the maintainers for this repo will be to our migration efforts? CC @janeyx99 |
I have an issue there: deepseek-ai/DeepGEMM#333. But given the situation of the repo, I do not think we should put too much hope in upstream PRs. But since NVIDIA has their own branch on master, I do not know whether maintaining a fork would be a good idea.
|
255e87e to
bd62abb
Compare
| @@ -234,7 +224,8 @@ def _deepgemm_fp8_gemm_nt_warmup( | |||
|
|
|||
| device = w.device | |||
| a1q = torch.empty((max_tokens, k), device=device, dtype=torch.float8_e4m3fn) | |||
| a1q_scales = torch.empty( | |||
| # Must be a power of two (UE8M0 packing asserts zero sign/mantissa bits). | |||
| a1q_scales = torch.ones( | |||
There was a problem hiding this comment.
Not related to the TORCH_LIBRARY migration — a separate pre-existing bug. torch.empty left a1q_scales uninitialized, occasionally tripping DeepGEMM's UE8M0 assert (scales must be exact powers of two). Switched to torch.ones; warmup output is discarded, so this has no effect on real inference.
1c2538e to
e98f4cf
Compare
|
@Harry-Chen, I opened a PR upstream on the |
|
This pull request has merge conflicts that must be resolved before it can be |
Signed-off-by: Chris Leonard <chleonar@redhat.com>
…py only needs SRC and OUT; update the cmake call to match. Signed-off-by: Chris Leonard <chleonar@redhat.com>
Signed-off-by: Chris Leonard <chleonar@redhat.com>
Signed-off-by: Chris Leonard <chleonar@redhat.com>
…epgemm.sh git tag Signed-off-by: Chris Leonard <chleonar@redhat.com>
Signed-off-by: Chris Leonard <chleonar@redhat.com>
… parameter to a mutable parameter Signed-off-by: Chris Leonard <chleonar@redhat.com>
torch.empty leaves a1q_scales as garbage, which fails DeepGEMM's UE8M0 assert that scales be exact powers of two. Use torch.ones instead; warmup output is discarded, so this is a no-op for real inference. Unrelated to the TORCH_LIBRARY migration — a pre-existing bug in the branch beyond the last pinned git tag. Signed-off-by: Chris Leonard <chleonar@redhat.com>
e98f4cf to
2e77b11
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
Purpose
This PR continues the libtorch stable ABI migration (see #26946) for vLLM. Here, we are not migrating deep_gemm to the stable abi yet, but are moving it off of pybind and onto TORCH_LIBRARY. Once we make these updates, we will open a seperate PR to make deep_gemm torch ABI stable. This points to the DeepGEMM updates in the PR deepseek-ai/DeepGEMM#393
cleonard530/DeepGEMM(migrate_pybind_to_torch_library) so vLLM vendors the TORCH_LIBRARY + abi3 layout instead of pybind11. If this works, we will need to discuss how we want to mergecleonard530/DeepGEMM. vLLM has a fork ofDeepGEMM, but currently the build still points towards upstream.g++build of_C.cpython-*.sowith a single_C_extension.abi3.sobuilt via DeepGEMM’ssetup.py, plusdeep_gemm/_C.pyas thetorch.ops.deep_gemmshim._C.py+_C_extension*.so) and dropDEEPGEMM_PYTHON_INTERPRETERS/TARGET_PY.This closes the per-interpreter DeepGEMM binding path tracked after the failed single-abi3 attempt ([Build] Make bundled DeepGEMM wheel portable across Python versions #41476 / Revert "[Build] Make bundled DeepGEMM wheel portable across Python versions" #41512) and implements the TORCH_LIBRARY direction noted in [Build] DeepGEMM wheel integration: planned cleanups #42431.
@janeyx99 @Harry-Chen
Test Plan
Test Result
PASS100 failed, 322 passed, all failures arefatal error: sanitizer/asan_interface.h: No such file or directory. Not related to TORCH_LIBRARY migrationPASSPASSPASSEssential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Migration progress using the Audit Python extension torch-abi-audit:
Note, deep_gemm used to have
uses-private-apiinstead ofabi3-ok