Skip to content

graph: nocache attn: avoid null mask buffer access - #24420

Closed
richiejp wants to merge 1 commit into
ggml-org:masterfrom
richiejp:fix-swa-mask-no-cache-encoder
Closed

richiejp wants to merge 1 commit into
ggml-org:masterfrom
richiejp:fix-swa-mask-no-cache-encoder

Conversation

@richiejp

Copy link
Copy Markdown

Overview

When there is a model with only SWA layers then the scheduler never
allocates the kq_mask buffer. Meanwhile llm_graph_input_attn_no_cache::set_input()
fills the non-SWA mask unconditionally triggering an assertion failure.

This change tracks when SWA and non-SWA layers are used in a graph and
fills only the mask(s) that are used. In theory nextn layers can be on
a separate graph so we can not simply check hparam's layers for SWA.

Additional information

This is similar to #23131. Presently there are no SWA only no-kv-cache models
so this bug can't be triggered. However I am working on adding the OpenAI Privacy
Filter model archicture which does token classification using only SWA.

I didn't simply check that the buffer is allocated because if there is another bug
that causes the buffer alloation to be missed then it kicks the bucket down the road.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: Yes, I vibe coded the original fix and then threw that away and wrote this, using Claude for review and codebase searches.

When there is a model with only SWA layers then the scheduler never
allocates the kq_mask buffer. Meanwhile llm_graph_input_attn_no_cache::set_input()
fills the non-SWA mask unconditionally triggering an assertion failure.

This change tracks when SWA and non-SWA layers are used in a graph and
fills only the mask(s) that are used. In theory nextn layers can be on
a separate graph so we can not simply check hparam's layers for SWA.
@richiejp
richiejp requested a review from CISC as a code owner June 10, 2026 15:05
@CISC

CISC commented Jun 10, 2026

Copy link
Copy Markdown
Member

This change tracks when SWA and non-SWA layers are used in a graph and fills only the mask(s) that are used. In theory nextn layers can be on a separate graph so we can not simply check hparam's layers for SWA.

First off, not sure why you think that is a blocker? hparams.n_layer() will give you the number of regular layers.

Secondly, it is preferable to not preemptively fix something you think may be an issue, it is better to bring this up in the PR that potentially introduces it since it is likely to be ironed out during review.

@richiejp

Copy link
Copy Markdown
Author

First off, not sure why you think that is a blocker? hparams.n_layer() will give you the number of regular layers.

I choose what I thought was the most direct way to get the SWA layers actually in the current graph rather than the whole model. I dont know if there is some other way for layers to be in separate graphs. I guess not.

Secondly, it is preferable to not preemptively fix something you think may be an issue, it is better to bring this up in the PR that potentially introduces it since it is likely to be ironed out during review.

OK

@richiejp richiejp closed this Jun 10, 2026
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