Repository navigation
Conversation
![opencodejr[bot]](https://avatars.githubusercontent.com/in/5497873?s=60&v=4){kind=small}
There was a problem hiding this comment.
The cap works. On the base branch, four images read in one session were all re-sent on every later request. With this PR and media.image.max_images: 2, only the newest two are sent and the older ones get the existing removal placeholder. I checked this with the simulated model and counted the images in each request body. The session-model-request tests pass and tsc is clean for core and schema.
Public API: this adds an optional config key, media.image.max_images (positive integer). It is backwards compatible: when it isn't set, behaviour is unchanged.
Concerns, most important first (details inline):
- The setting is read once when the layer is built, so editing it doesn't apply until a restart.
- Past the cap, each new image moves the cut by one, which invalidates the prompt cache on every request.
- The setting's description says the placeholder names the original file, but it doesn't.
#59303 also changes boundImages (fixed 30→20 cap), so the two PRs will conflict.
| const hooks = yield* PluginHooks.Service | ||
| const transport = yield* SessionModelTransport.Service | ||
| const app = yield* App.Metadata | ||
| // Read on every prepare: `media.image.max_images` is location-scoped, like the blob setting. |
There was a problem hiding this comment.
Despite the comment, this runs once when the layer is built, not on every prepare. Config reloads files in place (Config.reload publishes Event.Updated), so a user who edits max_images mid-session keeps the old value until the location restarts. The other media.image settings follow edits live: ConfigImagePlugin observes them with ConfigEntryObserver and pushes them into the Image service. Could this be read inside prepare, the way SessionBlob.enabled reads media_blob on each call, or go through the same plugin/service path?
| } | ||
| // A count-based cap cuts strictly by position: providers like vLLM limit the number of images per | ||
| // request independently of their byte size, so small images can still overflow a small per-request cap. | ||
| const countCut = maxImages === undefined ? 0 : Math.max(0, sizes.length - maxImages) |
There was a problem hiding this comment.
Once the session is over the cap, every new image moves cut by one. So the replaced prefix, and with it the provider's prompt cache, changes on every request that adds an image. The byte budget above deliberately drops down to a lower target so the cut moves only now and then (#56983), and #59303 uses steps of 10 for the same reason. Could the count cut work the same way, e.g. when the count goes over maxImages, drop down to a lower target (or in fixed steps) instead of exactly sizes.length - maxImages?
| max_width: PositiveInt.pipe(optional), | ||
| max_height: PositiveInt.pipe(optional), | ||
| max_base64_bytes: PositiveInt.pipe(optional), | ||
| max_images: PositiveInt.pipe(optional).annotate({ |
There was a problem hiding this comment.
This description, which users see, says the placeholder "names the original file, so a tool can still read it back". The placeholder used (IMAGE_REMOVED) is generic and doesn't include a file name or path. Please either change the wording or make the placeholder include the path.
Backends enforce a per-request image count (vLLM's --limit-mm-per-prompt, for example), and once a session accumulates more images than that cap every subsequent request is rejected, permanently breaking the session. The existing image budget in boundImages only bounds bytes, which small images can sail under forever. Add media.image.max_images to the config: when set, boundImages also drops the oldest images beyond the cap, replacing each with the same placeholder text the byte budget uses, so a tool can still read the original back by path. Unset keeps today's behavior byte-for-byte. The byte and count cuts run together and the tighter one wins. Regression tests cover the count-only cut, small images staying untouched without a cap, and the byte/count interplay. Verification: bun test test/session-model-request.test.ts test/session-compaction.test.ts test/config/ is 324/324; bun typecheck clean (1,884 files).
- Read media.image.max_images inside prepare on every request instead of once at layer construction, so config edits apply without a restart (same per-call pattern as SessionBlob.enabled). - Select the field with last-wins across config documents: Config.latest picks whole media objects, so a project config setting only max_width used to shadow a global max_images. - Give the count cap the same hysteresis as the byte budget: crossing the cap drops back to a lower target (cap minus min(10, cap/2), floor 1) instead of moving the replaced prefix on every new image, which churned the provider prompt cache per request. - The removal placeholder now names the pruned image file, matching the config description and letting Read retrieve the original. Tests: count-cap expectations updated for the hysteresis target; added hysteresis-stability, filename-in-placeholder, and config field merge-order cases. bun run script/test.ts test/session-model-request.test.ts is 15/15; bun typecheck clean.
2b359b8 to
abcb5e0
Compare
There was a problem hiding this comment.
Two of the three earlier points are fixed. max_images is now read on every request, so config edits take effect. The placeholder now names the file: in a drive run with the cap at 2, I saw The image "shot1.png" was removed…. Tests (15/15) and tsc in packages/core pass.
Public API: unchanged since the last review. It adds the optional media.image.max_images config key, which is backwards compatible.
The prompt-cache point isn't fixed yet (see inline). The count cut is still worked out from the total count alone, so it moves on every new image. It also keeps only the lower target, never up to the cap. The new hysteresis test asserts this behaviour, so it passes.
packages/core/src/session/model-request.ts:262: This cut is worked out fromsizes.lengthalone. So once you're over the cap it is alwayssizes.length - target, and it still moves by one on every new image. It also never keeps more thantarget. I calledboundImages(…, 4)on 1 to 10 images: 5 to 10 images all kept 2 and replaced 3, 4, … 8. The byte budget above avoids this by going through the images in order with a runningkept. The count cap needs the same loop, for examplefor (const _ of sizes) { kept++; if (kept > maxImages) while (kept > target) { kept--; cutCount++ } }. That way 5 images replace 3, and the cut stays put until the count crosses the cap again. The newprunes with hysteresis…test asserts the current behaviour (slice(0, 7)→ 5), so it will need updating too. With the loop it would be 3.
Issue for this PR
Closes #59794
Type of change
What does this PR do?
Backends enforce a per-request image count (vLLM's
--limit-mm-per-prompt '{"image":4}', for example), and once a session accumulates more images than that cap, every subsequent request is rejected — the session becomes permanently unusable. The existing image budget inboundImagesonly bounds bytes (IMAGE_BYTES_TRIGGER/TARGET), which small images can sail under forever.This adds
media.image.max_imagesto the config. When set,boundImagesalso drops the oldest images beyond the cap, replacing each with the same placeholder text the byte budget uses — so a tool can still read the original back by path. Unset keeps today's behavior byte-for-byte, which keeps the change zero-risk for existing sessions. The byte cut and the count cut run together and the tighter one wins, so the count cap can also catch images the byte budget kept.This is a different slice from #59303: that PR proposes a fixed 30-image cap plus compaction on count errors, which still fails on a 4-image backend (as noted in the issue). A user-configured cap matches what the backend actually enforces.
How did you verify your code works?
test/session-model-request.test.ts: the count-only cut drops the oldest first, 40 small images stay untouched without a cap, and the tighter of the byte/count cuts wins.bun test test/session-model-request.test.ts test/session-compaction.test.ts test/config/is 324/324 passing.bun typecheckclean (1,884 files across 2 projects).Screenshots / recordings
N/A, not a UI change.
Checklist
Added by the opencode bot (AI-generated).
Related: #47721, #48095, #52042
Public API: adds the optional
media.image.max_imagesconfig key (backwards compatible)