why not pass the model config to it, to decide whether it should:

• return it from kwarsgs if it's there
• return None if unnecessary (non flash attn)
• compute it and sync
  to avoid the

max_seqlen = None
if falsh_attn(config)..

in multiple places

another thing we can do is extend the old get_vision_cu_seqlens to return a tuple[tensor, int | none] since they are always used together or neither @vasqu

Yea tbh, I don't see why we can't combine them. We should let the single version of cu seqlens live tho to be BC

Agreed, this is cleaner. I updated get_vision_max_seqlen to take the model config and handle the decision internally, and added get_vision_attention_seqlens to return both values while keeping get_vision_cu_seqlens unchanged for BC.

the goal of the changes is that by this point, if the model uses fa, max seqlens should be already computed

i say let it fail loudly if not

Yep agree, don't like the recompute in any case will make it hard to see on new models whether they used the utils properly

Agreed. When Flash Attention is used, max_seqlen should already have been computed by this point. I removed the recomputation fallback, and it now raises a clear ValueError if the value is missing.

can be left in **kwargs: Unpack[TransformersKwargs] since we do pass the kwargs to attn interface

Done, max_seqlen is no longer declared by the pass-through Block/Encoder layers and just flows through **kwargs. It stays explicit only on Attention, where it’s actually consumed.

doesn't the attn interface raise an error on its own when it's missing ?

I checked the Flash Attention source. The native flash_attn_varlen_func requires cu_seqlens_q/k and max_seqlen_q/k together and would reject an incomplete call.
However, Transformers currently checks in _flash_attention_forward:

is_fa_with_varlen_kwargs = all(
    value is not None
    for value in (cu_seq_lens_q, cu_seq_lens_k, max_length_q, max_length_k)
)

Therefore, when cu_seq_lens_q/k are provided but max_length_q/k are None, Transformers does not call flash_attn_varlen_func and never reaches the native error. Without an attention mask or packed position IDs, it instead falls through to the fixed-length flash_attn_func, which can incorrectly allow attention across packed sequences.
I propose centralizing the validation and routing in _flash_attention_forward:

explicit_varlen_kwargs = (
    cu_seq_lens_q,
    cu_seq_lens_k,
    max_length_q,
    max_length_k,
)
has_any_varlen_kwargs = any(value is not None for value in explicit_varlen_kwargs)
has_all_varlen_kwargs = all(value is not None for value in explicit_varlen_kwargs)

if attention_mask is not None:
    # Derive the complete varlen arguments internally.
elif has_any_varlen_kwargs:
    if not has_all_varlen_kwargs:
        raise ValueError(...)
    # Use the complete precomputed varlen arguments.
elif is_fa_with_position_ids:
    # Derive the complete varlen arguments from position_ids.
else:
    # Fixed-length Flash Attention.

This makes the explicitly provided varlen arguments an all-or-none group, while leaving the attention_mask and packed position_ids paths unchanged. We can then remove the repeated checks from each model Attention implementation.
Would centralizing this validation in _flash_attention_forward and removing the per-model checks align with what you had in mind?

Tbh I'm against too much validation here. It will make the code much more unreadable. This is kind of a power feature and we just have to guarantee that our vision utils do this properly.

For BC purposes we could still calculate the max seq len within the attention module with the vision utils

After thinking it over, I agree that we shouldn’t raise here. For BC, if max_seqlen is missing, we should compute it with the helper. In the standard Transformers model path, it is already precomputed and passed down, so this fallback adds no extra max computation or synchronization. It only runs when users call a Block or Attention module directly, preserving the previous behavior.

+1, seems a bit breaking and shouldn't require validation as we are sure that max_len is passed from the VisionModel

I kept a fallback to preserve BC. The normal model path still precomputes max_seqlen, so the fallback won’t be hit there. It only handles direct Block/Attention calls where max_seqlen isn’t provided. I’ve updated all affected models accordingly.

yeh i think we don't need to raise the error in every vision attn, we pass the max cuseqlens here so it should be raised by the attn interface when given None (if it isn't the case already)

for the sake of bc it should probably be added last in order

max_seqlen was already a required argument in MiniCPMV4_6VisionAttention before this PR, in the same position before attention_mask. I only widened its type because non-FA paths can now explicitly pass None. To preserve the original signature more strictly, I’ll remove the default while keeping its existing position, avoiding any change to the positional argument mapping.

I also checked the other Attention signatures and found that some newly added max_seqlen parameters were inserted before existing parameters such as position_embeddings, which does introduce a positional BC issue. I’ll move those newly added optional parameters after the existing ones. Thanks for prompting this broader check.

ah i commented on the wrong one i guess, i saw a max_seqlen added in between two args somewhere

ah yes thanks ! just read the rest of your reply 😅

@ShareLer here

ha, i kinda assumed it was already moved to kwargs in prev PR for export

Imo we can put it as Unpack[TransformersKwargs], what do yall think?

Hmm, it is a bit weird but I'd rather keep the full signature here to indicate that this is now required here. Also cu seqlens was needed for the non FA paths so would be weird to kwarg only max seqlen

I think keeping max_seqlen explicit in the Attention signature is clearer here. Flash Attention requires it together with cu_seqlens, so exposing both makes the varlen input contract clear to callers. Keeping it optional preserves BC for direct Block/Attention calls, while still indicating that callers should pass the precomputed value when available. Intermediate Blocks can continue to pass it through **kwargs.

+1, seems a bit breaking and shouldn't require validation as we are sure that max_len is passed from the VisionModel

not sure but we can just ignore the none guard imo

same here no?

Ok happens below as well, but not mentioning anymore everywhere

Imo, we should just have

Elsewhere as well then

I.e. we should allow to return the max seqlens that are already prepared within the function (cu seqlen does the same)

get_vision_cu_seqlens implements get-or-compute directly from kwargs. get_vision_max_seqlen already does the same at the model level, but by the time we enter Attention, max_seqlen has been bound to the explicit parameter and is no longer in kwargs.

Therefore, replacing the current guard with:

max_seqlen = get_vision_max_seqlen(cu_seqlens, self.config)

would ignore the precomputed value and recompute it in every layer. To preserve the explicit Attention signature, we would need to pass the already-bound value into the helper:

def get_vision_max_seqlen(
    cu_seqlens,
    config,
    max_seqlen=None,
    kwargs=None,
    kwarg_name="max_seqlen",
):
    if max_seqlen is None and kwargs is not None:
        max_seqlen = kwargs.pop(kwarg_name, None)

    if max_seqlen is not None:
        return max_seqlen

    if not is_flash_attention_requested(config):
        return None

    return int((cu_seqlens[1:] - cu_seqlens[:-1]).max().item())

This would centralize the None check in the helper rather than remove it. Since you also preferred keeping max_seqlen explicit in the Attention signature earlier, I think the current explicit fallback is clearer than adding another way to pass the same value into the helper. Would keeping it as is make more sense to you?

Can we not just slightly adjust the calls like max_seqlen = get_vision_max_seqlen(cu_seqlens, self.config, kwargs={"max_seq_len": max_seq_len})?

My goal here is to avoid the none guard just so that we return the existing max seqlen if possible

This is a bit weird imo we dont need to assume this to not exist here, we already guaranteed it so no need to allow this to be none

I think this guard is still needed for non-FA paths. get_vision_max_seqlen intentionally returns None for eager/SDPA, while get_downsampled_inputs is still called when use_vit_merger=True, including with the default SDPA implementation. Removing the guard would evaluate None // 4. Making the value always available would require an otherwise unnecessary max computation and synchronization for non-FA paths, so I’d prefer to keep the guard here.

Gotcha, let's keep a small comment then tho?

Oh yea we never really had actual tests tbh, I'd rather remove this and just verify with our slow tests

not on you, I remember audio being a bit too different which is why we opted for per file implementations maybe something we should push for @IlyasMoutawwakil @eustlb @ebezzam

can the max_seqlen be computed from inputs["cu_window_seqlens"] here ?

Not quite: cu_window_seqlens describes the windowed attention used by the ViT merger, while this max_seqlen is for the encoder’s per-image packed attention and is based on the full h * w sequence length. These values differ when an image is larger than one window. The merger already derives its own maximum from window_h * window_w, so I think the current full-image cu_seqlens is still needed here.

i see, thanks !

is the int and item both necessary ?

removed int conversion

max_seqlen is computed the same way independently of modality so not sure @vasqu maybe we should strip the utility from the keyword "vision" and use it everywhere ?

It's just a bit weird because it would live in vision utils. Maybe we need multimodal_utils, vision, and audio?

this can be just in utils.generic for now, but mm_utils also works, although might stay mostly empty

I moved the modality-agnostic logic to utils.generic.get_max_seqlen, updated both vision and audio paths to use it, and removed the duplicated audio helpers.