Skip to content

Conversation

@leo-pony
Copy link
Collaborator

@leo-pony leo-pony commented Oct 9, 2025

What this PR does / why we need it?

Adapt to vllm changes: "Move Backend enum into registry #25893"

Does this PR introduce any user-facing change?

No

How was this patch tested?

Signed-off-by: leo-pony <nengjunma@outlook.com>
@github-actions
Copy link

github-actions bot commented Oct 9, 2025

👋 Hi! Thank you for contributing to the vLLM Ascend project. The following points will speed up your PR merge:‌‌

  • A PR should do only one thing, smaller PRs enable faster reviews.
  • Every PR should include unit tests and end-to-end tests ‌to ensure it works and is not broken by other future PRs.
  • Write the commit message by fulfilling the PR description to help reviewer and future developers understand.

If CI fails, you can run linting and testing checks locally according Contributing and Testing.

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 aims to fix a CI break resulting from the _Backend enum being moved in the vllm library. The current implementation uses a version check that is brittle because it relies on an exact version match. My review provides a more robust solution using a try-except block, which is a standard and more reliable way to handle such dependency changes across different versions.

Comment on lines +29 to +34
from vllm_ascend.utils import vllm_version_is

if vllm_version_is("0.11.0"):
from vllm.platforms import _Backend
else:
from vllm.attention.backends.registry import _Backend
Copy link
Contributor

Choose a reason for hiding this comment

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

high

The use of vllm_version_is with an exact version match is brittle for ensuring compatibility across different vllm versions. This logic implies that only version 0.11.0 uses the old import path, while all other versions (both older and newer) use the new one, which is an unlikely scenario for an API migration.

A more robust and idiomatic Python pattern for handling moved imports is to use a try...except ImportError block. This approach is independent of version string formats (which can be inconsistent, especially for development builds) and directly verifies the import's availability.

Suggested change
from vllm_ascend.utils import vllm_version_is
if vllm_version_is("0.11.0"):
from vllm.platforms import _Backend
else:
from vllm.attention.backends.registry import _Backend
try:
# In recent vLLM versions, _Backend was moved to the backends registry.
from vllm.attention.backends.registry import _Backend
except ImportError:
# Fallback to the old location for older vLLM versions.
from vllm.platforms import _Backend

@wangxiyuan
Copy link
Collaborator

we'd like to remove this patch file once ds3.2 is updated to vLLM way.

@wangxiyuan wangxiyuan added ready read for review and removed ready read for review labels Oct 9, 2025
@leo-pony leo-pony closed this Oct 14, 2025
@leo-pony leo-pony deleted the atte_backend_adapt branch October 14, 2025 02:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants