Repository navigation
[revert] [Build] Fix QuTLASS compilation with PyTorch 2.13 - #61844
cleonard530 wants to merge 8 commits into
Conversation
Signed-off-by: Chris Leonard <chleonar@redhat.com>
Signed-off-by: Chris Leonard <chleonar@redhat.com>
Harry-Chen
changed the title
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
@yewentao256 PTAL. |
|
✅ @cleonard530, CI is now available for this PR.
|
|
Although they are orthogonal, it may be better to land this after #61700 is merged |
There was a problem hiding this comment.
🟢 Approval recommended
No blocking issues remain, and the reported successful build and ABI audit support the restored configuration.
0 open findings
What changed in this PR
Reverts #58173’s QuTLASS build workaround to restore PyTorch stable ABI targeting and reject incompatible source trees.
Changes:
- Checks for the header introduced by QuTLASS’s stable ABI migration.
- Restores the PyTorch 2.11 target definition and removes
QUTLASS_DISABLE_PYBIND.
| File | Description |
|---|---|
| cmake/external_projects/qutlass.cmake | Restores stable ABI build settings and adds a source compatibility guard. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Many vLLM users will use their own pytorch version, so we also need to consider backward compatibility |
I think the main purpose is not compatibility, but to correctly mark |
|
/ci run |
|
✅ Triggered Buildkite CI #95323 for commit |
|
/ci run |
|
✅ Triggered Buildkite CI #95380 for commit |
|
@yewentao256, as @ZJY0516 and @Harry-Chen mentioned, this can allow users to use their own pytorch version and help enforce ABI stability. For some additional context, with the current setup, QuTLASS could use symbols that are allowed under whatever version it's built with (in this case 2.13). So vLLM now has a minimum torch version of 2.13, when it could have as low as 2.11 (see new image of torch-abi-audit in description). Migrating vLLM to the stable ABI allows us to build against one PyTorch version and run against any version at or above TORCH_TARGET_VERSION, without rebuilding. However, because of this single dependency, we're unnecessarily excluding two PyTorch versions that vLLM could otherwise support. This could be inconvenient for developers who want to use older PyTorch versions without rebuilding, as well as distributors who want to package wheels for multiple PyTorch versions without maintaining a separate build for each one. And as @Harry-Chen mentioned, this flag help us enforce that QuTLASS does not bring in any unstable symbols (the bug that caused the original PR was really a feature!). There is a CI check as well to make sure this doesn't happen, but this is an extra safeguard in case that CI check fails. I am not surprised that this has not caused bugs, as it's a very particular use case, and a developer probably would've just updated python if they ran into this, but I still think it's worth adding it back for the reasons described above. Let me know what you think or if you have any questions! |
So I am thinking we should pin 2.13 version instead? |
This should not be an issue IMHO. Anyhow we have not announced any ABI stability for vLLM yet. And setting the lowest possible torch version does not bring obvious harm. |
|
I agree with @Harry-Chen that I doubt lowering it would bring harm. But, @yewentao256, if your concern is that Qutlass has been using 2.13 for weeks, and dropping it lower might break what we know works, then we could raise it to 2.13. I think it's better to have some torch_version instead of nothing. |
|
It theoretically shouldn't make trouble if pinning 2.11, but still not well tested in current main branch. So agree with @cleonard530 pinning 2.13 would be better in current situation |
|
For all libraries that depend on QuTLASS it would be ideal for the torch version pin to be as low as possible, but I understand the concern of not wanting to promise something that’s not well tested. Another thing to keep in mind is that this pin is mostly used as binary metadata and the README/documentation/runtime requirements can still recommend 2.13 as the tested version. This way we don’t block others from going off the beaten path and using the binary with 2.11 or 2.12. Otherwise, is there a process for how we can test to ensure that 2.11 works well too? |
|
/ci retry |
|
✅ Triggered Buildkite CI #95676 for commit |
@yewentao256 Maybe let's land this as a quick fix? And I think @cleonard530 is planning on some formal stable ABI checks in the near term to prevent future breaks. |
| QUTLASS_DISABLE_PYBIND=1 | ||
| TARGET_CUDA_ARCH=${QUTLASS_TARGET_CC} | ||
| CUTLASS_ENABLE_DIRECT_CUDA_DRIVER_CALL=1 | ||
| TORCH_TARGET_VERSION=0x020B000000000000ULL |
There was a problem hiding this comment.
| TORCH_TARGET_VERSION=0x020B000000000000ULL | |
| TORCH_TARGET_VERSION=0x020D000000000000ULL |
@Harry-Chen I think we should pin 2.13 now to avoid a breaking change based on current situation
Fully agree that should have some more checks here to avoid changes of the code by accident
There was a problem hiding this comment.
I still don't think setting to 2.11 will be a breaking change, because as long as the code compiles with 2.11, they are compatible with libtorch 2.11+, given the nature of how libtorch stable ABI works. Nothing changes for the user IMHO, until they decide that they will try to reuse a .so file from different commits / release with build system changes, which is not desired to work by design.
There was a problem hiding this comment.
It might still break some user specific env, eg. that's why we have #58173 to fix for our customer.
There was a problem hiding this comment.
We don't know if the changes will bring more bugs here as current main built based on 2.13 close to one month
There was a problem hiding this comment.
I and @cleonard530 think #58173 was because a stale version of cutlass was checked out in the source tree. And this PR exactly adds a check against this situation, not to work around it by disabling stable ABI checks: https://github.com/vllm-project/vllm/pull/61844/changes#diff-cc0576febbd89620d5e7a9b3dc8ff346e44df0b4e2c8a6ecaba019668c05fe8cR55-R60
I did not know about the specific environment, but the release pipeline never failed before or after #58173:
We don't know if the changes will bring more bugs here as current main built based on 2.13 close to one month
That flag does not change the version qutlass is built against, but just works as a feature gate. The APIs you can use with 2.11 is a strict subset of what you can use with 2.13. But it is always built against the latest pytorch headers, 2.14 today.
There was a problem hiding this comment.
Feature gate doesn't mean the code is exactly same https://github.com/pytorch/pytorch/blob/2b3ec34829036a65cd9d1398ea72a0167dc37470/torch/csrc/stable/macros.h#L114, for instance
My concern is about the change in compiled behavior, while targeting 2.11 should theoretically be compatible, it doesn't necessarily produce the same binary behavior as targeting 2.13.
Since the lower target hasn't been well tested in the current setup, I'd prefer either keeping 2.13 for this fix.
(I don't think just standard CI runs are enough, as users and customers have many specific envs, you never know what will happen in their situation)
|
Hi @cleonard530, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
Overview
This PR is to revert the changes in #58173, and to add a check to make sure this doesn't happen again. I believe what happened was that a local old version of QuTLASS, before it was migrated to the stable ABI, was being built against the new flags that help ensure QuTLASS stays stable. So I also added a check in here to make sure that a version of QuTLASS after this update is being used. This checks for the file
qutlass/csrc/include/registration.hwhich was only added after the migration.cc @Harry-Chen @janeyx99
Validation
main branch:
torch-abi-audit vllm:
this branch
build succeeds and
[STABLE ] [abi3-ok ] _qutlass_C.abi3.so (stable_shim=60, unstable=0) min-torch=2.11.0Note: the drop in the number of stable_shim comes from internal implementation differences in torch/stable. Particularly the inclusions of
torch_exception_get_what,torch_exception_get_what_without_backtrace, andtorch_new_stable_ivaluein 2.13.Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.