Skip to content

fix: do not leak set_attention_backend into process-wide registry#14250

Open
Solaris-star wants to merge 2 commits into
huggingface:mainfrom
Solaris-star:fix/14249-attention-backend-leak
Open

fix: do not leak set_attention_backend into process-wide registry#14250
Solaris-star wants to merge 2 commits into
huggingface:mainfrom
Solaris-star:fix/14249-attention-backend-leak

Conversation

@Solaris-star

@Solaris-star Solaris-star commented Jul 20, 2026

Copy link
Copy Markdown

What does this PR do?

ModelMixin.set_attention_backend() already pins the backend on this model's attention processors. It also called _AttentionBackendRegistry.set_active_backend(), which is process-global.

Any other model whose per-module backend is still None then falls through to that global backend in dispatch_attention_fn. Example: model A sets FA3; model B later runs with attn_mask and unexpectedly hits FA3, which rejects the mask.

This PR stops flipping the global registry from set_attention_backend(). Per-module pins stay. Use attention_backend(...) when a temporary process-wide override is intentional.

Fixes #14249

Before submitting

  • Did you use an AI agent (Claude Code, Codex, Cursor, etc.) to help with this PR? If so:
    • Did you read the Coding with AI agents guide?
    • Did you run the self-review skill on the diff?
    • Did you share the final self-review notes in the PR description or a comment?
  • Did you read the contributor guideline?
  • Did you read our philosophy doc? (important for complex PRs)
  • Was this discussed/approved via a GitHub issue or the forum? Please add a link to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?
  • Are you the author (or part of the team) of the model/pipeline (only applicable for model/pipeline related PRs)?

Self-review notes

  • Root cause is global registry mutation, not missing per-module assignment.
  • Change is intentionally small: remove one global write; keep per-module loop.
  • No new tests in this PR yet; happy to add a unit test that asserts active backend is unchanged after set_attention_backend if maintainers want it.
  • Docs: behavior of attention_backend() context manager is unchanged.

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

Model.set_attention_backend() already pins the backend on each of this
model's attention processors. Flipping _AttentionBackendRegistry's
active backend made other models with unset per-module backends pick up
the same backend (e.g. FA3 + attn_mask on an unrelated model).

Leave the global active backend alone; use attention_backend() when a
temporary process-wide override is intended.

Fixes huggingface#14249

Signed-off-by: Solaris-star <820622658@qq.com>
@github-actions github-actions Bot added models fixes-issue size/S PR with diff < 50 LOC labels Jul 20, 2026
Regression coverage for huggingface#14249 / huggingface#14250.

Signed-off-by: Solaris-star <820622658@qq.com>
@github-actions github-actions Bot added tests size/M PR with diff < 200 LOC and removed size/S PR with diff < 50 LOC labels Jul 20, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

set_attention_backend on one model leaks globally to other models via the active-backend registry

1 participant