Skip to content

Conversation

@varun-sundar-rabindranath
Copy link
Contributor

@varun-sundar-rabindranath varun-sundar-rabindranath commented Oct 28, 2025

Purpose

Revert part of #27370

Adding this triton_kernels as dependency seems to break wheel installation.

(new) coder@4xl40s devspaces (main) $ uv pip install vllm --extra-index-url https://wheels.vllm.ai/nightly
Using Python 3.12.11 environment at: new
  × Failed to resolve dependencies for `vllm` (v0.11.1rc4.dev17+g361a7463d.cu129)
  ╰─▶ Package `triton-kernels` was included as a URL dependency. URL dependencies must be expressed as direct requirements or constraints.
      Consider adding `triton-kernels @ git+https://github.com/triton-lang/[email protected]#subdirectory=python/triton_kernels` to your
      dependencies or constraints file.

Solve : #27573

Test Plan

Test Result

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request reverts the addition of the triton_kernels dependency to fix a critical installation issue encountered with uv. While this change correctly addresses the installation problem, it also removes a dependency required for mxfp4 fused moe functionality, as indicated by the removed comment. This will likely break that feature. My review comment highlights this trade-off and suggests that this side effect be documented in the pull request description. For a long-term solution, I've recommended considering making this an optional dependency to avoid breaking the build for users who do not require this specific feature.

@varun-sundar-rabindranath
Copy link
Contributor Author

cc @zyongye

@zyongye
Copy link
Member

zyongye commented Oct 28, 2025

I wonder what is the correct way to add a dependency like this?

@cjackal
Copy link
Contributor

cjackal commented Oct 28, 2025

Apparently this type of direct URL dependency in a package is forbidden by PyPI for security reason; downloading from an untrusted(non-PyPI, that is) domain must be triggered by the end user, not by a hidden dependency.

So the solution would be like building the wheel and uploading to a proper wheel index, I think.

Copy link
Collaborator

@simon-mo simon-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wheel installation requires all packages in same registry (PyPI).

Let's vendor the code.

@mgoin mgoin added the ready ONLY add when PR is ready to merge/full CI is needed label Oct 28, 2025
@noooop
Copy link
Collaborator

noooop commented Oct 29, 2025

unblock buildkite/ci/pr/language-models-test-extended-pooling


this pr work great with pooling models

Signed-off-by: Varun Sundar Rabindranath <[email protected]>
@varun-sundar-rabindranath
Copy link
Contributor Author

@simon-mo This is the previous build-kite run, https://buildkite.com/vllm/ci/builds/36673#019a2e0d-01c0-4071-b17d-d1c66f9efe02
It fails the LoRA tests and the Quantization tests. I rebased the branch to latest main to trigger the current run.

I saw your comment on #27598 (comment) mentioning LoRA and Quantization nightly failures. Maybe it is okay to force-merge this PR ?

@simon-mo
Copy link
Collaborator

Oh this is a revert to unbreak main, happy to force merge

@simon-mo simon-mo merged commit f6d5f58 into vllm-project:main Oct 29, 2025
13 of 18 checks passed
bhagyashrigai pushed a commit to odh-on-pz/vllm-upstream that referenced this pull request Oct 29, 2025
ilmarkov pushed a commit to neuralmagic/vllm that referenced this pull request Nov 7, 2025
ZhengHongming888 pushed a commit to ZhengHongming888/vllm that referenced this pull request Nov 8, 2025
rtourgeman pushed a commit to rtourgeman/vllm that referenced this pull request Nov 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci/build ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants