Skip to content

perf(review): cap vision request context to bound KV cache under load - #4369

Merged
JSONbored merged 1 commit into
mainfrom
claude/vision-speed-optimizations
Jul 9, 2026
Merged

perf(review): cap vision request context to bound KV cache under load#4369
JSONbored merged 1 commit into
mainfrom
claude/vision-speed-optimizations

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • The self-host Ollama concurrency tuning (feat(selfhost): tune Ollama concurrency for a shared embed+vision GPU (#4327) #4352/GPU passthrough for Ollama + embedding benchmark vs. the current CPU-bound setup #4327) sized VRAM headroom assuming a bounded per-request context for concurrent slots. Vision calls had no num_ctx cap, so under concurrent load a large context is the actual OOM/thrashing risk on a shared embed+vision GPU (per the concurrency research: "the real danger is pushing the VLM to higher parallelism with uncapped context").
  • Adds a generic providerOptions passthrough on createOpenAiCompatibleAi's chat path (an Ollama-specific request extension field, silently ignored by every other provider — embeddings, subscription CLIs, Anthropic) and sets num_ctx: 4096 on the self-host visual-vision call.
  • Companion image-downscaling optimization (screenshots are captured fullPage: true, so image height can far exceed the 1440px viewport width) is scoped separately — it needs a real image-processing dependency + Dockerfile/esbuild change since this self-host build bundles everything with zero node_modules at runtime today, filed as its own follow-up rather than bundled into this smaller, zero-dependency fix.

Part of #4325.

Scope

Validation

  • git diff --check
  • npm run typecheck
  • npm run test:coverage locally — new providerOptions passthrough branches (present/absent) covered in test/unit/selfhost-ai.test.ts; the vision call site's num_ctx value asserted in test/unit/visual-vision-wiring.test.ts
  • npm audit --audit-level=moderate — 0 vulnerabilities
  • Ran npm run test:ci (full unsharded suite) end-to-end — green

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text stays sanitized, low-noise, and does not imply compensation guarantees or optimization tactics.
  • N/A — no auth/UI/API/OpenAPI/MCP surface touched.

Notes

  • No migration, OpenAPI, or cf-typegen regeneration needed.

The self-host Ollama concurrency tuning (#4327/#4352) sized VRAM
headroom assuming a bounded per-request context; vision calls had no
num_ctx cap, so under concurrent load a large context is the real
OOM/thrashing risk on a shared embed+vision GPU. Adds a generic
providerOptions passthrough on the OpenAI-compatible chat path
(Ollama-specific request extension, ignored by every other provider)
and sets num_ctx: 4096 on the self-host visual-vision call.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@JSONbored JSONbored self-assigned this Jul 9, 2026
@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 9, 2026
@loopover-orb

loopover-orb Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-09 04:50:39 UTC

4 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
Adds an optional `providerOptions` passthrough field to `AiRunOptions` and wires it into `createOpenAiCompatibleAi`'s chat request body (src/selfhost/ai.ts) so the self-host vision call in `runSelfHostVisualVision` (src/queue/processors.ts) can set `num_ctx: 4096`, capping per-request KV cache to protect the shared embed+vision GPU under concurrent load. The change is small, additive, and optional (spread only when `providerOptions` is set), so it can't affect any other call site or provider. Tests directly exercise both branches (present/absent `providerOptions`) against the real request-serialization code path and assert the exact field on the vision call site, which is genuine coverage rather than a fabricated scenario.

Nits — 5 non-blocking
  • Only mocked-fetch unit tests verify the request shape; nothing here confirms Ollama's OpenAI-compatible endpoint actually honors a top-level `options.num_ctx` field in production — worth a quick manual/integration check against a live Ollama instance before relying on it for the OOM mitigation.
  • `providerOptions?: Record<string, unknown>` in src/selfhost/ai.ts is untyped/unconstrained; narrowing to something like `{ num_ctx?: number }` would catch typos at compile time given this is currently only used for one field.
  • The rationale comment is duplicated near-verbatim across src/selfhost/ai.ts and src/queue/processors.ts; consider stating it once and cross-referencing to avoid drift if the reasoning changes later.
  • Add a follow-up note/metric to confirm 4096 doesn't truncate legitimate large vision prompts (e.g. very tall fullPage screenshots) now that the downscaling optimization is deliberately deferred to a separate PR.
  • If more Ollama-only tuning knobs get added later (e.g. `num_predict`), consider typing `providerOptions` as a small discriminated interface instead of `Record<string, unknown>` to keep the passthrough self-documenting.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 49 registered-repo PR(s), 41 merged, 504 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 49 PR(s), 504 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: not available
  • Official Gittensor activity: 49 PR(s), 504 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed


💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →.

Checked by Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@JSONbored
JSONbored merged commit 084cb38 into main Jul 9, 2026
9 checks passed
@JSONbored
JSONbored deleted the claude/vision-speed-optimizations branch July 9, 2026 05:03
@codecov

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.95%. Comparing base (e432f96) to head (a23a849).
⚠️ Report is 8 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4369   +/-   ##
=======================================
  Coverage   93.95%   93.95%           
=======================================
  Files         399      399           
  Lines       36796    36797    +1     
  Branches    13443    13444    +1     
=======================================
+ Hits        34571    34572    +1     
  Misses       1569     1569           
  Partials      656      656           
Files with missing lines Coverage Δ
src/queue/processors.ts 95.39% <ø> (ø)
src/selfhost/ai.ts 98.65% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier.

Development

Successfully merging this pull request may close these issues.

1 participant