feat(endpoints): add image UUID cache reuse - #869
Conversation
Try out this PRQuick install: pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@4cdc4240de02abbbd375c9a89fe327d84b84909aRecommended with virtual environment (using uv): uv venv --python 3.12 && source .venv/bin/activate
uv pip install --upgrade --force-reinstall git+https://github.com/ai-dynamo/aiperf.git@4cdc4240de02abbbd375c9a89fe327d84b84909aLast updated for commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a vLLM-oriented UUID-Keyed Multimodal Cache Support
🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/aiperf/endpoints/openai_chat.py (2)
110-112:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve strip key on
raw_messagespath so final-turn cleanup still executes.Line 111 returns
strip_key=None; then Lines 91-92 cannot evict session state on final turns, leaving stale UUID tracker entries behind.Proposed fix
if turns[-1].raw_messages is not None: - return turns[-1].raw_messages, None + strip_key: str | None = ( + request_info.x_correlation_id + if uuid_and_strip and request_info.x_correlation_id + else None + ) + return turns[-1].raw_messages, strip_keyAlso applies to: 91-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aiperf/endpoints/openai_chat.py` around lines 110 - 112, The early return that yields raw_messages currently returns None for strip_key which prevents the final-turn cleanup from running; update the return(s) that return turns[-1].raw_messages so they return the existing strip_key value instead of None (e.g., return turns[-1].raw_messages, strip_key), and make the same change for the other similar return path near lines 91-92 to ensure session eviction/UUID tracker cleanup still executes.
134-138:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCommit UUID strip-state only after a successful send boundary.
Line 137 updates
_mm_uuid_sessionswhile building payload. If the request fails/cancels before ingestion, a retry may strip URLs for UUIDs the backend never primed.Suggested direction
- if strip_key is not None and newly_emitted: - self._mm_uuid_sessions.setdefault(strip_key, set()).update(newly_emitted) - return messages, strip_key + # Defer commit until transport reports successful request completion. + # Return newly_emitted as commit-delta for caller-managed success hook. + return messages, strip_key, newly_emitted+# On successful completion (outside payload formatting), then: +self._mm_uuid_sessions.setdefault(strip_key, set()).update(newly_emitted)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aiperf/endpoints/openai_chat.py` around lines 134 - 138, The code currently mutates _mm_uuid_sessions during payload building (using strip_key and newly_emitted), which can mark UUIDs as primed even if the request fails; change the flow so that the update to self._mm_uuid_sessions.setdefault(strip_key, set()).update(newly_emitted) happens only after a confirmed successful send/ingest boundary. Concretely, have _build_payload (or the function returning messages, strip_key) return the newly_emitted set (or a success callback token) rather than applying the update, and then perform the update in the send/ingest success path (the caller that actually posts the request), using the same strip_key and newly_emitted identifiers so abandoned builds don’t commit strip state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/aiperf/endpoints/openai_chat.py`:
- Around line 110-112: The early return that yields raw_messages currently
returns None for strip_key which prevents the final-turn cleanup from running;
update the return(s) that return turns[-1].raw_messages so they return the
existing strip_key value instead of None (e.g., return turns[-1].raw_messages,
strip_key), and make the same change for the other similar return path near
lines 91-92 to ensure session eviction/UUID tracker cleanup still executes.
- Around line 134-138: The code currently mutates _mm_uuid_sessions during
payload building (using strip_key and newly_emitted), which can mark UUIDs as
primed even if the request fails; change the flow so that the update to
self._mm_uuid_sessions.setdefault(strip_key, set()).update(newly_emitted)
happens only after a confirmed successful send/ingest boundary. Concretely, have
_build_payload (or the function returning messages, strip_key) return the
newly_emitted set (or a success callback token) rather than applying the update,
and then perform the update in the send/ingest success path (the caller that
actually posts the request), using the same strip_key and newly_emitted
identifiers so abandoned builds don’t commit strip state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7f37a432-b16b-44ec-9cc8-78adedb3fb3b
📒 Files selected for processing (6)
src/aiperf/common/models/dataset_models.pysrc/aiperf/dataset/loader/single_turn.pysrc/aiperf/endpoints/openai_chat.pytests/unit/dataset/loader/test_single_turn.pytests/unit/endpoints/test_chat_endpoint_multimodal_cache_mode.pytests/unit/endpoints/test_openai_chat_completions.py
✅ Files skipped from review due to trivial changes (1)
- src/aiperf/dataset/loader/single_turn.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/aiperf/common/models/dataset_models.py
- tests/unit/dataset/loader/test_single_turn.py
f108d2c to
1b592c0
Compare
matthewkotila
left a comment
There was a problem hiding this comment.
Reviewed against origin/main, validated against the actual files, and reproduced wire output against the in-repo mock server. Also validated end-to-end against real vLLM (Qwen/Qwen3-VL-2B-Instruct, --mm-processor-cache-gb 4, --enforce-eager, sliding-window dataset, 4 turns, 1 repeat): TTFT avg 397.80 → 61.36 ms (−85%), TTFT max 1,404.44 → 76.58 ms (−95%), end-to-end duration 12.55 → 9.03 s (−28%). The feature delivers the win the PR description claims. The dedup-at-load-time architecture is clean and the default-off path is byte-identical to main. One coverage gap stands out — the equivalent multi-turn JSONL row shape is unhooked.
Suggested fix order (with rough complexity):
- 🟡 F1 —
multi_turnJSONL loader doesn't apply load-time dedup; wire emitsuuidkeys but nourlstrip - 🟢 F2 — consider treating
""as an opt-out sentinel on the wire (matches dedup; lets parallel-array form mix UUIDed and non-UUIDed images) - 🟢 F3 — first-occurrence empty-content treated as cache-served (dedup signal collision)
- 🟢 F4 —
--uuid-and-stripsilently no-ops on non-chat endpoints - 🟢 F5 — unused
is_final_turn/url_indexconftest params - 🟡 F6 — flag name describes implementation, not user intent (rename consideration)
Working well: stateless endpoint, strict 1:1 alignment validation at both layers, default-off backward compatibility verified. Pre-deduping at load time means the hot path does no work and there's no concurrency bookkeeping to worry about.
…ad conftest params Addresses two review findings on PR #869: F1 — `MultiTurnDatasetLoader.convert_to_conversations` now raises `NotImplementedError` when `endpoint.uuid_and_strip` is set. Multi-turn JSONL has no load-time dedup pass, so the previous behavior shipped full image bytes on every repeat while still tagging them with `uuid` keys — silently broken benchmarks. Fail loudly until multi-turn dedup lands. Flag description and CLI docs updated to spell out the single-turn-only restriction. F5 — `tests/unit/endpoints/conftest.py` was passing `is_final_turn` and `url_index` through `create_request_info` with values identical to the `RequestInfo` defaults, and no test exercises non-default values. Drop the dead pass-throughs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Qi Wang <qiwa@nvidia.com>
1b592c0 to
3176455
Compare
…ad conftest params Addresses two review findings on PR #869: F1 — `MultiTurnDatasetLoader.convert_to_conversations` now raises `NotImplementedError` when `endpoint.uuid_and_strip` is set. Multi-turn JSONL has no load-time dedup pass, so the previous behavior shipped full image bytes on every repeat while still tagging them with `uuid` keys — silently broken benchmarks. Fail loudly until multi-turn dedup lands. Flag description and CLI docs updated to spell out the single-turn-only restriction. F5 — `tests/unit/endpoints/conftest.py` was passing `is_final_turn` and `url_index` through `create_request_info` with values identical to the `RequestInfo` defaults, and no test exercises non-default values. Drop the dead pass-throughs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Qi Wang <qiwa@nvidia.com>
edb55df to
504d81e
Compare
This comment has been minimized.
This comment has been minimized.
…ad conftest params Addresses two review findings on PR #869: F1 — `MultiTurnDatasetLoader.convert_to_conversations` now raises `NotImplementedError` when `endpoint.uuid_and_strip` is set. Multi-turn JSONL has no load-time dedup pass, so the previous behavior shipped full image bytes on every repeat while still tagging them with `uuid` keys — silently broken benchmarks. Fail loudly until multi-turn dedup lands. Flag description and CLI docs updated to spell out the single-turn-only restriction. F5 — `tests/unit/endpoints/conftest.py` was passing `is_final_turn` and `url_index` through `create_request_info` with values identical to the `RequestInfo` defaults, and no test exercises non-default values. Drop the dead pass-throughs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Qi Wang <qiwa@nvidia.com>
54fa1b7 to
82ee292
Compare
82ee292 to
92c2e92
Compare
92c2e92 to
10206ec
Compare
ajcasagrande
left a comment
There was a problem hiding this comment.
Overall: Clean, narrowly-scoped, well-guarded opt-in feature. Pydantic validators + the config-level chat-endpoint gate + the multi_turn NotImplementedError are all appropriate, and the 95 targeted unit tests pass. Verified end-to-end with the real aiperf CLI against a request-logging server: dedup and cache-only-reference emission behave exactly as documented — repeated-within-turn retained, cross-turn stripped to url:"", explicit cache-only references passed through.
Two Low-severity findings below, both confirmed at runtime and independently adversarially re-verified. Neither is a correctness bug or a merge blocker.
Fix order (optional polish): F1 (metrics semantics) → F2 (silent degradation). Both are documentation/UX, not code-correctness.
Working well: strict length validation on Image.uuids, the narrow _extend_image_parts override that leaves audio/video untouched, and the hard gates preventing use outside chat + single_turn.
10206ec to
6735f0d
Compare
Signed-off-by: furionw <qiwa@nvidia.com>
6735f0d to
ed29c63
Compare
ajcasagrande
left a comment
There was a problem hiding this comment.
Approving. Re-verified on ed29c63f (current head) — both findings from my earlier review are resolved, and the one outstanding bot flag is obsolete.
Findings resolved:
- F1 (cache-only refs counted as images): documented as intended in
docs/metrics-reference.md—num_imagesis now defined as logical image references, explicitly noting cache-only URLs count. Good by-design call. - F2 (flag-off UUID passthrough): fixed.
--uuid-and-stripnow gates only AIPerf-side dedup; authored UUIDs (including cache-only refs) always pass through on the chat endpoint. VerifiedImage(uuids=[...])with strip off renders{"url": "", "uuid": ...}, locked in bytest_authored_uuids_pass_through_when_strip_disabled.
CodeRabbit "Major" lifecycle concern: obsolete — _mm_uuid_sessions per-request state is gone; stripping is a deterministic load-time pass in the loader, so retry-re-strip and unbounded-growth no longer apply.
Verification: 97/97 pass across the touched test files (chat cache-mode, stripped-media, endpoint validator, single_turn, multi_turn).
Working well: strict UUID length/empty-string validation, the narrow _extend_image_parts override that leaves audio/video untouched, and the hard gates keeping this to chat + single_turn.
Signed-off-by: furionw <qiwa@nvidia.com>
Why
AIPerf currently resends repeated images, preventing cache-aware sliding-window benchmarks. The endpoint—not AIPerf—is authoritative for UUID cache state: a UUID-only reference may be valid because another run, process, or prewarm populated it. Dataset-authored UUIDs therefore pass through unchanged, while this opt-in path strips only content AIPerf previously observed. Operators still need adequate cache capacity and session-to-replica affinity.
Effect
Full image:
{"image_url":{"url":"image"},"uuid":"u1"}Cache-only reference:
{"image_url":{"url":""},"uuid":"u1"}What Change
--uuid-and-stripcontrol only AIPerf-managed deduplication.Test Plan
uv run pre-commit run --all-files