Support ragged patch grids in attentional_pool - #195
Merged
Conversation
…ragged-edge handling
avgpool's window sum was correct only because `_pad_grid_to_windows` zero-pads and `F.pad` defaults to 0; the mask was applied to the denominator alone. Zero the padded cells through `valid` so both pooling adapters exclude padding the same way. Bit-exact given zero padding.
Cross-check both pooling adapters against a brute-force pool that attends over each window's real patches with no padded tensor at all, assert the pooled output is independent of the pad value, and cover grid % window == 1, window > grid, window == grid, and non-square token counts.
Codecov Report✅ All modified and coverable lines are covered by tests.
🚀 New features to boost your workflow:
|
Naeemkh
approved these changes
Sep 8, 2026
Naeemkh
left a comment
Member
There was a problem hiding this comment.
I checked the two places this could go wrong and both hold up: the validity mask and the patch data are reshaped in the same window order, and since the number of windows is a ceiling division, every window keeps at least one real patch, so a fully masked window, which would make attention return NaN, is unreachable rather than merely untested. The breaking removal is self-contained; every reference to the dropped parameter lives in a file this PR already changes, so nothing is orphaned.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
attentional_poolnow pools a patch grid the pool window does not evenly divide, instead of rejecting it. The grid is padded toceil(grid/window) * windowand each partial edge window pools only its real patches — the padded patches are masked out of that window's K/V and out of the mean-query. Output length isceil(grid/window)², so any window pools any grid (e.g. a 3×3 window on a 14×14 SigLIP2 grid → 5×5 = 25 tokens, previously refused)._pad_grid_to_windowshelper for the pad + validity mask, andavgpoolnow masks its window sum through the samevalidtensor rather than relying on the pad value being zero.pooled_token_countloses its keyword-onlyrequire_divisibleparameter andDIVISIBLE_ONLY_POOL_TYPESis removed. The parameter existed only to raise on ragged grids, which is the behaviour being removed, so there is no shim — out-of-tree callers passingrequire_divisible=must drop the argument.Testing
uv run pytest tests/unit/ -v --timeout=120— 1757 passed, 3 skipped (skips are pre-existing gates: reserved arches,mlflowabsent,RUN_HF_TESTS)uv run pytest examples/vlm/tests -q— 33 passed;uv run pytest examples/vlm/eval/tests/unit -q— 104 passeduv run ruff check kempnerforge/ tests/ scripts/passes;uv run ruff format --check kempnerforge/ tests/ scripts/— 166 files already formatteduv run pyright kempnerforge/— 0 errors, 0 warningstests/unit/test_adapter.py— 118 passed. Corner cases:grid % window == 1(7×7 @ 3 → the corner window has exactly one real patch),window > grid(2×2 @ 3 → 1 token),window == grid(4×4 @ 4 → 1 token, divisible path), non-perfect-square token counts still raise.atol=1e-5. Separately, padding with1e4instead of0leaves both adapters' output unchanged, so padding is masked rather than merely zero.attn_maskfails 11 tests; replacing the ragged denominator withw*wfails the avgpool edge-window test; reverting avgpool's numerator masking fails only the 4 pad-value tests (i.e. that change is bit-exact under zero padding).examples/vlm/configs/*.toml, comparing this branch'spooled_token_countagainst main's (extracted verbatim fromgit show origin/main:…/adapter.py): 10 configs usemlp_2layer(identity, unaffected), 3 leavevision_encoder.num_tokens=0(probed at build), 0 useattentional_pool. Because that set is weak on its own, also swept the whole space — grid 1..64 × window 1..16 = 1024 pairs: 211 divisible pairs unchanged, 0 changed; 813 ragged pairs previously rejected are now allowed; 0 ragged counts changed. The checker refuses (exit 2) on an empty/incomplete config enumeration, a config that fails to load, and a truncated/empty/helper-less main source — all 6 refusals verified against genuinely bad input.kempner_eng), ragged run — the capability:examples/vlm/train.pywithattentional_pool,pool_window=3, 14×14 grid → 25 tokens/image, 20 steps forward+backward under FSDP2 (dp_shard=4), loss 10.81 → 8.63,step_20committed. The same config onorigin/mainexits 1 withValueError: this pooling connector requires the patch grid (14x14) be divisible by the pool window (3).pool_window=2(14×14 → 49 tokens) on this branch and onorigin/main, 4×H200 each from an empty checkpoint dir —num_image_tokens=49and 29,831,680 params on both, and the per-step loss is identical at every one of the 20 steps (10.8125 → 8.3750, max |Δ| = 0.000000). The comparison tool refuses on a missing/empty log, a log with no step lines, a log short ofmax_steps, a log ending in a traceback, and asymmetric step ids (7 guards verified).torchrun -m pytest:tests/distributed/test_checkpoint.py8 passed,test_resilience.py5 passed,test_vlm_fsdp.py6 passed — the adapter is FSDP2-wrapped on the VLM path.Shutdown requested at step 63 — saving emergency checkpoint→step_63committed →Graceful shutdown complete. Auto-resume logsResumed from step 63, 290,304 tokens seenandResumed DataLoader: epoch=0, skip_batches=63, then continues from step 64 with loss in the pre-interrupt range (3.72 → 4.19).scripts/slurm/multinode.sh: 2 nodes x 2 H200 (holygpu8a[10201,10601]), cross-node NCCL/gloo overib0, srun one task per GPU,world_size=4,num_image_tokens=25,Applied VLM FSDP2: dp_mesh=('dp_shard',)— 20 steps,step_20committed, exit 0. Its per-step loss is identical to the single-node 4-GPU ragged run at every step (max |delta| = 0.000000), so the node layout does not perturb the ragged path.attentional_poolalso exercised through all four VLM arches (joint_decoder,mot,moma,cross_attention) forward+backward — the pooled count feeds the residual/positional layout, and all four give finite grads at 25 pooled tokens.torch.compile(fullgraph=True)over both pooling adapters at 5 (grid, window) pairs incl. 3 ragged: no graph break, outputs match eager toatol=1e-5, backward finite.attentional_pool's 10 state-dict keys andavgpool's 2 are byte-identical acrosspool_window2/3/5/7, and window-2 weights load unchanged into a window-3 module. Activation checkpointing wraps onlyTransformerBlock/Attention, never the adapter, so there is no recompute interaction.kf-codebase-design-new.htmlafter implementation: qualifies for core as a general model knob / capability improvement to an existing core model block (Part 3 lists "Ragged pooling → To core"), entering through the existingadapterregistry key andadapter.pool_windowfield — no new registry category, no new config field, no core→examples/dependency, and it helps every pooling VLM rather than one experiment.Closes #135