Repository navigation
fix(cache): distinguish full input content in cache keys - #678
linjiaye929 wants to merge 2 commits into
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
| monkeypatch.setattr(item, "get_result", infer) | ||
| await cache.aget_complete(item) |
There was a problem hiding this comment.
Concurrent image reads lack coverage
The new tests finish hashing before simulated inference starts, and their images are already loaded. Production runs both paths concurrently on the same image, including lazy images returned by Image.open. The new image.tobytes() call reads pixels, so these tests cannot catch problems caused by both workers first loading that image. Add a regression test with a lazy image and overlapping cache lookup and preprocessing.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Confirmed with lazy PNG inputs: overlapping first reads could crash Pillow decoding and leave image_embed pending. Fixed in d0d72fa: cached requests decode off-loop before dispatch, with a thread lock that stays held if the awaiter is cancelled. Added native BatchHandler/diskcache overlap tests using the real CLIPImageProcessor; encode_core is an explicit deterministic fixture, with no model download. Direct-image/bytes, repeat-hit, shared-image/cancellation, invalid-pixel and uncached controls pass; 45 focused tests pass locally. AI-assisted follow-up.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52b71caa36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| state: Any = content | ||
| if isinstance(content, (EmbeddingSingle, ReRankSingle)): | ||
| # JSON preserves query/document boundaries without pickle string memoization. | ||
| state = (type(content), json.dumps(content.to_input())) |
There was a problem hiding this comment.
Preserve non-BMP distinctions in text cache keys
For direct Python-engine calls, a non-BMP code point and an explicit UTF-16 surrogate pair are distinct str inputs (for example, "\U0001D11E" and "\uD834\uDD1E"), but default json.dumps serializes both as "\ud834\udd1e". _pre_hash therefore produces the same key for the same input class, so after one input is cached the other can receive its embedding or rerank score instead of being processed independently; use an injective string encoding that does not fold these two representations.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in d0d72fa. Text keys now use ensure_ascii=False, preserving distinct non-BMP characters and explicit surrogate pairs without losing query/document boundaries or string-alias independence. Added 12 native diskcache cases across embedding, classification and both rerank fields, including lone-surrogate controls and repeat hits. All 45 cache/image tests pass locally. AI-assisted follow-up.
Summary
The vector disk cache currently uses
AbstractSingle.str_repr()as the key. Those strings are also used to estimate scheduling lengths, so distinct requests can share a key:("ab", "c")and("a", "bc")both become"abc".When an earlier entry wins the cache/inference race, the later request receives the earlier request's result.
This change gives the cache content-aware, input-type-specific SHA-256 keys. Text and rerank inputs use JSON to preserve boundaries and avoid string-aliasing differences. Images include mode, size, pixels, RGBA palette and metadata; audio includes sampling rate, dtype, shape and C-order sample bytes. The resulting identity is serialized with fixed pickle protocol 4 before hashing. No pickle deserialization is added.
Key construction runs in the cache read worker. The same key is reused for the asynchronous write. Scheduling strings and token-usage estimates are unchanged.
Compatibility and cost
content-v1byte namespace. Legacy string-key entries remain on disk but are not read, so the cache warms again. Existing expiry/eviction handles old entries; this change does not delete them.Validation
Linux, Python 3.12.14, diskcache 5.6.3, NumPy 1.26.4, Pillow 12.3.0, and CPU-only PyTorch 2.8.0.
From
libs/infinity_emb:040269bgive 15 failed, 12 passed. Failures cover the distinct-input collisions, legacy-key isolation and single key computation.ruff check .passes. Formatting and codespell pass for the changed files;git diff --checkpasses.mypy infinity_embreports the same 22 missing-optional-dependency errors on this branch and unmodified main, with identical output. There are no errors incaching_layer.py.make precommit, downloaded-model, accelerator and server end-to-end suites were not run in this minimal CPU environment.Checklist
AI assistance was used to prepare this contribution.