Repository navigation
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 058884a921
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
I think you should wait for feedback from maintainers to disssus about value of this feature and maintainability before opening a PR. This is common practice as attn backend bas been altered. Otherwise, it is quite hard to be accepted. |
There was a problem hiding this comment.
I don't think we should introduce contrib/ directory and we have implemented the registration for Attention backend. This PR is introducing one independent registry for sparse attention. And it's trying to make sparse default when users have installed it. But in most of scenarios, sparse attention could lead to accuracy regression. So it's better to make it optional instead of default.
Could we make SparseAttentionBackend inherit AttentionBackend?
@gcanlin Thanks for the detailed review — these are all valid points and I want to engage with them carefully. On inheriting AttentionBackend This is architecturally sound and I've prototyped it. The inheritance chain works: SparseAttentionBackend(AttentionBackend) VIDEO_SPARSE_ATTN becomes one line in DiffusionAttentionBackendEnum, the independent However, inheriting introduces a new problem the current design avoids --attn-backend is a global setting. get_attn_backend() returns the same backend class Cross-attention (Q from patch tokens, KV from text encoder) must remain fully dense. The current per-model enable_sparse_attention() hook exists precisely because it The complexity is equivalent; it just moves from one place to another: Current PR: one enable_sparse_attention() per model (gcanlin's concern) My proposal I'm happy to go either direction, but I'd like to resolve this in RFC #2233 before
On "sparse should not be default" Completely agreed. The _auto_select() behavior in registry.py is wrong — On contrib/ Will remove from this PR. Reference implementations will move to separate repos. Happy to hear from @tzhouam @hsliuustc0106 @SamitHuang on the module_type question |
|
@gcanlin Thanks for the detailed review. This revision addresses all four points:
@alex-jw-brooks The new implementation inherits the existing @lishunyang12 This revision was made in direct response to maintainer feedback from this PR. The architecture now follows the existing |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 51fc4cec07
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
0511e65 to
12af120
Compare
| for ep in importlib.metadata.entry_points(group="vllm_omni.sparse_attn"): | ||
| if ep.name.lower() == name.lower(): | ||
| return ep.load() | ||
| except Exception: |
There was a problem hiding this comment.
If ep.load() raises (e.g. plugin exists but has an ImportError), this silently returns None — user sees "falling back to dense" with no clue the plugin was found but broken. Please distinguish not-found vs load-failure, e.g. except Exception as e: logger.warning("Failed to load sparse attn plugin %s: %s", name, e).
There was a problem hiding this comment.
Fixed — ep.load() failure now logs logger.warning(..., exc_info=True) with the plugin name and entry_point value, distinct from the "not found" path which returns None silently.
| from vllm_omni.diffusion.forward_context import get_forward_context | ||
|
|
||
| return get_forward_context().omni_diffusion_config.sparse_attn | ||
| except Exception: |
There was a problem hiding this comment.
Bare except Exception masks real bugs (e.g. AttributeError from misconfigured config) the same way it handles the expected "no forward context" case. Narrow the catch or add logger.debug so users know when sparse attention silently fell back to dense.
There was a problem hiding this comment.
Fixed — narrowed to except (RuntimeError, LookupError, AssertionError) for the "not yet initialised" case, with a separate except AttributeError: logger.debug(...) for misconfigured config.
| normalized_kwargs["sparse_attn"] = merged | ||
| except json.JSONDecodeError: | ||
| logger.warning("Invalid --attn-backend JSON: %s", attn_backend_raw) | ||
| elif attn_backend_raw: |
There was a problem hiding this comment.
Plain names like flash_attn or sage_attn also hit this branch and get routed into sparse_attn → wrong dispatcher. Known dense backend names should be excluded before this elif.
There was a problem hiding this comment.
Fixed — known dense backends are now excluded via _dense_names (built dynamically from DiffusionAttentionBackendEnum minus SPARSE_ATTENTION) before routing a plain name to the sparse_attn dispatcher.
eb92fcc to
49b576c
Compare
lishunyang12
left a comment
There was a problem hiding this comment.
still waiting on the earlier threads. one more nit.
| # Handle --attn-backend: accepts simple name or JSON with config. | ||
| # --attn-backend spargeattn | ||
| # --attn-backend '{"backend":"spargeattn","topk":0.3}' | ||
| import json |
There was a problem hiding this comment.
Nit: move import json to the module-level imports.
There was a problem hiding this comment.
Fixed — moved to module-level imports (line 12).
bd69bd2 to
d0a2da0
Compare
|
|
||
|
|
||
| @dataclass | ||
| class DiffusionSparseAttnConfig: |
There was a problem hiding this comment.
Could we create DiffusionAttentionConfig first? Actually, this PR is doing two things implementing sparse attention and convert the current attention env way to CLI way. So after we add --attn-backend, we need to refactor the original attention backends. It's better to separate them into 2 PRs.
There was a problem hiding this comment.
I didn't think so.
DiffusionAttentionConfig would make sense when there are shared fields across multiple attention backends — for example, per-layer backend selection or step-level scheduling. Right now neither exists: FLASH_ATTN, SAGE_ATTN, and TORCH_SDPA have no kernel-level parameters, and DiffusionSparseAttnConfig is the only config type needed.
OmniDiffusionConfig already has attention_backend: str (which backend to use) and sparse_attn: DiffusionSparseAttnConfig (sparse-specific params) as two separate flat fields. Wrapping them into DiffusionAttentionConfig now would produce an empty shell with no new capability.
On splitting into 2 PRs: the CLI changes (--attn-backend, JSON routing in async_omni_engine.py, _dense_names exclusion) are tightly coupled to the sparse attention plugin mechanism — they exist specifically to route plugin names into DiffusionSparseAttnConfig. A PR1 without the sparse attention backend would have no consumers and no testable behavior.
When a second attention type needs config params (e.g. a quantized FlashAttn with a head_dim_override), introducing DiffusionAttentionConfig as a common wrapper would be the right time. Happy to open a follow-up RFC for that if it's the preferred direction.
There was a problem hiding this comment.
does vllm upstream provide sparseattnconfig for llm?
There was a problem hiding this comment.
RFC #2632 makes the right call — I'm happy to extend this PR to cover P1–P3.
Concretely, the changes needed are minimal:
is_self_attention: bool→AttentionRole(SELF | CROSS)enumDiffusionSparseAttnConfig(backend, params)→AttentionSpec(backend, extra)- Add
AttentionConfig(default, per_role)to replace the flat
attention_backend + sparse_attnfields onOmniDiffusionConfig - Make
get_attn_backend()role-aware (cache key fix included) - Add
extra: dicttoAttentionMetadata
This PR already covers P4 (first sparse backend as end-to-end validation).
I'll push the P1–P3 changes in a follow-up commit — should keep the diff
to ~150 additional lines across the affected files.
There was a problem hiding this comment.
@zzhang-fr Would appreciate it if you could open a new PR to implement #2632. Let this PR #2231 focus on the interface definition of sparse attention backend. What do you think?
There was a problem hiding this comment.
Agreed on the split. Will keep this PR focused on the sparse attention plugin interface.
For the attention backend refactoring, I'll study the design and ongoing discussion in #2632 first before opening a new PR.
There was a problem hiding this comment.
does vllm upstream provide sparseattnconfig for llm?
@hsliuustc0106 Upstream doesn't have a sparse attention config, but it does have AttentionConfig as a structured config class (vllm/config/attention.py) exposed via EngineArgs.attention_config (L575 in arg_utils.py). It holds backend: AttentionBackendEnum plus backend-specific flags like flash_attn_version and use_cudnn_prefill.
The gap: upstream's AttentionConfig is flat — single global backend, no per-role selection, no opaque extra dict. #2632 follows the same pattern but extends it with per-role (self/cross) backend selection for diffusion.
| return None | ||
|
|
||
| @staticmethod | ||
| def _resolve_plugin(cfg: DiffusionSparseAttnConfig) -> SparseAttnFn | None: |
There was a problem hiding this comment.
Is it necessary to introduce plugin for attention backend? If developers want to integrate their new sparse attention backend, just adding a new enum in DiffusionAttentionBackendEnum would be fine?
There was a problem hiding this comment.
Good question. For dense backends (FLASH_ATTN, SAGE_ATTN, etc.), adding an enum entry is exactly the right approach — they are in-tree and vllm-omni owns their maintenance.
Sparse attention is different: the maintainers explicitly don't want to take on the burden of maintaining sparse kernel backends inside vllm-omni. Each kernel (SpargeAttn, RainFusion, FlashInfer-sparse, MindIE-SD) has its own release cycle, hardware requirements, and API surface. Pulling them in-tree means vllm-omni has to track upstream changes, handle import errors, and gate releases on kernel compatibility.
The plugin system delegates that responsibility to the kernel authors. They ship sparge-vllm-omni, users pip install it, done — no PR to vllm-omni core required.
SPARSE_ATTENTION in the enum is the single in-tree entry point — it's the dispatcher. The actual kernels live outside.
What do you think?
There was a problem hiding this comment.
I suggest to provide simplest and easy-to-extend abstraction for sparse attn since hardware differs a lot here
Adds sparse attention support for DiT video models through the
existing AttentionBackend plugin system — no new registry needed.
Usage:
# Via CLI (serve)
vllm-omni serve Wan-AI/Wan2.2-T2V-A14B-Diffusers \
--attn-backend spargeattn
# Via CLI with JSON config
vllm-omni serve Wan-AI/Wan2.2-T2V-A14B-Diffusers \
--attn-backend '{"backend":"spargeattn","topk_ratio":0.3}'
# Via Python API
from vllm_omni.diffusion.data import DiffusionSparseAttnConfig
omni = Omni(model=..., sparse_attn=DiffusionSparseAttnConfig(
backend="spargeattn", topk_ratio=0.5))
How it works:
- SparseAttentionBackend is an AttentionBackend dispatcher that resolves
sparse plugins (e.g. SpargeAttn) via entry_points at init time
- Self-attention layers delegate to the plugin; cross-attention
always uses dense SDPA (controlled by is_self_attention flag)
- Sparse attention runs through the Attention wrapper, so SP/ring
parallel communication is preserved
- Opt-in only: no effect unless user provides sparse_attn config
or --attn-backend pointing to a sparse plugin
Signed-off-by: Zhen Zhang <zhen.zhang.fr@huawei.com>
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
Purpose
Build a unified plugin-based sparse attention interface for DiT modules by
extending the existing
AttentionBackendinfrastructure. Resolves #1568.Changes in this revision
SparseAttentionBackendnow inheritsAttentionBackend;_SparseAttentionImplinherits
AttentionImpl.SPARSE_ATTENTIONis a single line inDiffusionAttentionBackendEnum— the independent registry is deleted.contrib/removed. Reference implementations live in external repos.--attn-backend sparse_attentionor a pluginname directly. No auto-activation.
is_self_attention=FalseinWanCrossAttention;enable_sparse_attention()hook removed fromWanTransformer3DModel.selector.py:--attn-backend spargeattnworksdirectly once a plugin is installed — no
--sparse-attn-backendneeded.DiffusionSparseAttnConfigsimplified tobackend + params: dict—vllm-omni does not interpret any kernel-level parameter.
Design
SparseAttentionBackend(AttentionBackend)is registered asSPARSE_ATTENTIONin
DiffusionAttentionBackendEnum. All kernel implementations live in externalpip-installable packages — zero vendor backends in vllm-omni core.
Core changes (vllm-omni)
vllm_omni/diffusion/attention/backends/sparse_attention.py— new file:SparseAttentionBackend(AttentionBackend)+_SparseAttentionImpl(AttentionImpl)SPARSE_ATTENTIONadded toDiffusionAttentionBackendEnumis_self_attention: bool = Trueparam onAttention.__init__— cross-attentionpasses
is_self_attention=False, permanently dense, zero overhead_get_plugin_backend_clsinselector.pyWanCrossAttentionmarkedis_self_attention=False; hook removedDiffusionSparseAttnConfigfields:backend: str+params: dictPlugin interface
Plugins register a function via entry_points — not a class:
Function signature:
fn(q, k, v, params: dict, is_causal: bool) -> TensorThe
paramsdict is forwarded verbatim fromDiffusionSparseAttnConfig.params.vllm-omni does not interpret any kernel-level parameter — the plugin reads what it needs.
Configuration
DiffusionSparseAttnConfigaccepts flat or nested JSON:Usage
Test Result
Visual quality samples (dense vs topk=0.5 vs topk=0.3):
https://github.com/user-attachments/assets/62a7abfd-758f-4ec0-958b-fa3de672f972
https://github.com/user-attachments/assets/016c8463-f0bc-4a69-a891-b07fbc784a68
https://github.com/user-attachments/assets/17d2d464-0205-4d2c-8683-bd2e768c11f3