Repository navigation
Improve embedding fidelity - #1449
Conversation
|
Probe attachment 1/2 — chunking fidelity (kept out of the branch deliberately; attached for reviewers)
|
|
Probe attachment 2/2 — special-token server behavior (kept out of the branch deliberately; attached for reviewers)
|
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 14 days if no further activity occurs. If you are still working on this, please push a commit or leave a comment. Reviewers: please respond, or add the |
1d939f9 to
b1b5c73
Compare
b1b5c73 to
896ddbb
Compare
Improve embedding fidelity Signed-off-by: Edwin Yu <[email protected]>
896ddbb to
36a4595
Compare
marvinyu-memverge
left a comment
There was a problem hiding this comment.
Reviewed at d2004376. The measurement is the best part of this PR and I'd take the change on its strength -- greedy+weighted wins on all three encoders, and including the two structural checks (greedy+unweighted ranking last, balanced being weighting-indifferent to five decimals) is what makes it readable as signal rather than a lucky corpus. Two asks, both about coverage of the evidence rather than the change being wrong.
1. The special-token probe covers api.openai.com; the sanitizer it removes covered every backend this class serves.
The probe is thorough for what it tests -- 7 tokens x 3 cases x 3 models, all OK, so "OpenAI no longer 500s" is well established. But OpenAIEmbedder is pointed at non-OpenAI endpoints as a shipped, documented configuration: sample_configs/episodic_memory_config.cpu.sample has two of them under provider: openai -- ollama_embedder (nomic-embed-text) and openai_compatible_embedder (text-embedding-v4) -- and OpenAIEmbedderConf.model is described as "OpenAI Embeddings API-compatible model". Those tokenizers aren't OpenAI's and weren't probed. Since MemMachine stores chat transcripts, text containing a literal <|endoftext|> is more likely here than in most corpora, and the failure mode is a provider 4xx/5xx that exhausts retries into ExternalServiceAPIError -- an ingest failure that looks random because it's data-dependent.
Not asking you to probe Ollama and DashScope. Either is fine by me: keep the sanitizer and scope it to non-OpenAI base_url, or drop it as you have and say in the PR body that OpenAI-compatible backends are out of the probe's scope, so the next person who hits it knows where to look.
2. Neither half of the change is pinned by a test.
test_embed_oversized_input_with_no_max_input_length asserts call_count >= 2, which passes under balanced and greedy alike -- the diff updates its comment but not its assertion, so nothing in the suite distinguishes the old policy from the new. Your own grid says greedy+unweighted is the worst of the four configurations, so a later edit that reverts just the weighting lands in the worst cell and no test fails; the damage is cosine drift, with no error to notice. A small unit test with two chunks of very different length, asserting the merged vector is the length-weighted average and not the plain mean, would pin the half that's easiest to lose.
Observation, not an ask: max_input_length has no upper bound, and cluster_texts is still called with max_total_input_length_per_request (75,000) while chunking now uses effective_max. Above 75,000 that's the #1298 crash again, and greedy widens the window balanced used to cover: at max_input_length=80_000 a 100,000-char input gives balanced [50000, 50000] (fine) and greedy [80000, 20000] (ValueError -> 500). The default path is safe and your regression test still holds -- every shipped sample sets 2048, so I don't think anything reaches this today. Flagging it because the fix is one line (min(self._max_input_length or MAX, MAX)) if you want to close it while you're in here.
Verified while reading: the zero-chunk case is closed on all three embedders (chunk_text("") returns [], but OpenAI and Bedrock coerce or "." first and SentenceTransformer guards with and input_text, and max(len(chunk), 1) floors every weight); zip(..., strict=True) can't mismatch because unflatten_like already raises on a shape mismatch; the sweep is complete, with no embedder left on balanced+unweighted, so merged embeddings don't diverge by backend; and the branch is 0 behind main, so the #1646 lock refresh that touched these modules is absorbed.
A configured max_input_length above max_total_input_length_per_request (75,000 code points) can never be honored: no chunk larger than the cap fits in any request. Before this change such a limit reached cluster_texts with an oversized chunk and raised ValueError (HTTP 500), the MemMachine#1298 failure for that configuration. Greedy chunking widened which input lengths hit it: at max_input_length=80_000, balanced chunking crashed on 76,000-80,000 and 160,000-char inputs; greedy crashed on everything above 75,000. The regression test configures max_input_length=80_000 with a 100,000-char input; it fails before the clamp with the cluster_texts ValueError and passes after with two requests (75,000 + 25,000 chars). Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
|
Written by Claude Code from Edwin's positions. Paragraphs marked (Edwin) are his; those marked (Claude) are measurements or additions of mine that he has not separately asserted. 1. Sanitizer (Edwin) It was a bug on OpenAI's servers, 500s on inputs containing these tokens, and the sanitizer was a temporary workaround for it. Any worthwhile embedding endpoint handles special tokens in input text correctly; OpenAI got special treatment because of its size. If a third-party endpoint is chosen and fails on special tokens, the remedy is to stop using that endpoint, not for MemMachine to pre-edit input for every provider that might mis-tokenize. (Claude) The removed comment in the source said exactly that ("Tokens that cause 500 errors on text-embedding-3-small"), and the attached probe shows the bug is gone: all 66 cases (3 models x {control, 7 tokens x 3 input forms}) return 200. The sanitizer also wasn't free: it deleted the literal token from the text before embedding, so a stored transcript that quotes 2. Tests (Edwin) A test asserting "merged is the length-weighted average, not the plain mean" pins the code, not a behavioral contract. Any future change to the code requires the same change to the test, for the same reason, so the code and the test are bijective and the test catches nothing. There is also no point pinning one specific way to chunk text for embedding when a better approach may come later. (Claude) The contract (closeness to the whole-text embedding) is only observable against a real encoder. I checked whether the encoder the CI integration job already loads (all-MiniLM-L6-v2) could carry a test that fails before this change and passes after: it can't. At its 256-token window the before and after cells tie (balanced+unweighted 0.9222 vs greedy+weighted 0.9213 on a 10-text corpus, 10/30 wins; +0.007 at t=1.7 on a second corpus), though greedy+unweighted is clearly worst there too (0.9008). The measurement in the PR body is the evidence for the change; the body now says the fidelity change is not unit-pinned and why. 3. Observation (Edwin) Why is this an observation rather than an ask? Taking the fix since it's simple, with a test that fails before the change and passes after. (Claude) Clamped in 7dae4a2: |
|
Merged, so nothing here needs action - answering the question and closing out my end. On the observation. Reachability is the whole of it, and I read it off the producer rather than the threat model. On the sanitizer. Your point that it deleted the literal token from the text before embedding is a good one and it cuts against the argument I made. I used "MemMachine stores chat transcripts" as the reachability path for a mis-tokenizing endpoint, without weighing that the workaround was silently corrupting those same transcripts on the endpoint it did cover. Stating in the body that OpenAI-compatible endpoints are outside the probe's scope is the right resolution - it leaves the gap visible to whoever points the config at one. On the tests. The MiniLM check answers the ask rather than deflecting it. I asked for the change to be pinned; showing that the encoder CI already loads cannot produce a test that fails before and passes after is the fact I was missing, and the body saying the fidelity change is not unit-pinned and why is what I actually wanted out of it. |
Purpose of the change
Chunk-merged embeddings (texts longer than the model input limit are split, embedded per chunk, and averaged) reproduce the whole-text embedding more faithfully with greedy max-size chunks and a length-weighted average than with the current balanced chunks and unweighted mean. Measured on three encoders; numbers below.
The special-token sanitizer in the OpenAI embedder is removed. It was a workaround for a server-side bug at api.openai.com (text-embedding-3-small returned 500 on inputs containing tokens such as
<|endoftext|>), and that bug is gone: 7 tokens x 3 models x 3 input forms all return 200 with valid embeddings (probe script and results attached as a PR comment below). The workaround was not free: it deleted the literal token from the text before embedding, so a stored transcript that quotes<|endoftext|>was embedded as if it did not. It is also OpenAI-specific by design.OpenAIEmbedderis also used with OpenAI-compatible endpoints (Ollama, DashScope), which the probe does not cover; an endpoint that fails on a plain string containing such a token is broken at the endpoint, and the remedy is to not use it, not for MemMachine to pre-edit input for every provider that might mis-tokenize.max_input_lengthabove the OpenAI embedder's per-request cap (75,000 code points) is now clamped to the cap. A limit above the cap can never be honored (no chunk larger than the cap fits in any request), and before this change it reachedcluster_textswith an oversized chunk and raisedValueError(HTTP 500), the #1298 failure for that configuration. Greedy chunking widened which input lengths hit it: atmax_input_length=80_000, balanced chunking crashed on 76,000-80,000 and 160,000-char inputs; greedy crashed on everything above 75,000. No shipped sample config sets a limit above the cap.Description
chunk_text) instead of balanced chunks (chunk_text_balanced) in the OpenAI, SentenceTransformer, and Amazon Bedrock embedders.np.averageinstead of unweightednp.mean.or "."), which is independently load-bearing: the API rejects empty strings andchunk_text("")yields no chunks to average.max_input_lengthtomax_total_input_length_per_requestin the OpenAI embedder, with a regression test that fails before the clamp (ValueErrorfromcluster_texts) and passes after.Measurements
Method: 10 hand-written texts in distinct registers (fiction, technical docs, chat transcript, news, legal, academic, recipe, business memo, product review, encyclopedia), each short enough to embed whole — the whole-text embedding is the observable ground truth. Each text is chunked at limits 800 / 1600 / 3200 chars under each policy, chunks embedded, merged, and compared to the whole-text vector by cosine similarity. Single-chunk cells excluded; 28 cells per configuration per model. Probe script and raw per-cell results are attached as PR comments below (deliberately not committed to the branch).
This PR's configuration (greedy + length-weighted) vs current behavior (balanced + unweighted), paired over the same 28 cells:
Full split × weighting grid (mean cosine to whole-text embedding, higher = more faithful):
Two structural checks that this is signal, not noise:
Mechanism: to the extent an encoder mean-pools token states, the whole-text vector is approximately a length-weighted average over the text, which a merge approximates best when weights match chunk lengths and most characters sit in maximal-context chunks. The result holds on the OpenAI models even though their pooling is undocumented.
Caveat: per-cell variance is wider on the OpenAI models than on gemma (worst cell −0.019, best +0.048), so this is a consistent aggregate win rather than a uniform per-text one.
Type of change
How Has This Been Tested?
Test Results: Fidelity tables above; probe scripts and raw results attached as PR comments below; embedder unit tests pass (
server_tests/memmachine_server/common/embedder/). The fidelity change itself is not pinned by a unit test: its contract (closeness to the whole-text embedding) is only observable against a real encoder, and the measurement above is the evidence for it.Checklist
Maintainer Checklist