feat(embeddings): split batch inputs on meaning units - #643
Conversation
Keep sender headers, paragraphs, sentences, and data-URI images as separate searchable parts with source offsets so naruon SKU and due-date retrieval do not share one mixed vector. The one-vector naruon reduce stays; meaning_units carries the unit-level spans. Grounded in Karpukhin et al. (2020) and Günther et al. (2024). No new runtime dependency. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: do not merge #643. Close it in favor of #652.
Reproduced on ec88eea (this checkout). Existing tests print ok; they do not lock the naruon default path. CodeRabbit CLI is installed but coderabbit auth login --agent is not authenticated in this environment, so this is a file-and-repro review.
What this PR does
Always-on split_meaning_units replaces _force_token_safe_chunks. A completed document still reduces to len(embeddings) == len(inputs), and adds a meaning_units side channel. The naruon fixture (alpha body / beta attachment / gamma attachment) has no headers, blank lines, or images, so test_batch_embeddings.py stays green while real AP mail changes.
Critical (reproduced)
-
Always-on split under default 280k/240k ceilings. The fixture AP email becomes 4 provider parts (
header_block, invoice paragraph, SKU paragraph,embedded_image) and those four vectors are token-weighted-averaged back intoembeddings[0]. On main that email is one embed. naruon mail-import traffic will multiply request count and cost on every message. The base64 image (15 heuristic tokens vs 20 for the invoice line) dominates the averaged document vector, so default-path search quality regresses while cost rises. -
embeddings[i]is no longer the original document vector. Karpukhin-style retrieval needs separately indexed units. This PR still writes one averaged vector to the field naruon parses today.meaning_unitsis undocumented in OpenAPI and unread by the naruon fixture. Shape contract holds; semantic contract does not. -
_pack_spansemits the first oversized sentence intact. Repro:("word "*20).strip()+". Short stop."atmax_tokens=3yields a 21-tokensentence_unitplus a 3-token neighbor. The overflow fallback runs only whenpack_start is not None. Provider 400s or silent oversize sends follow.
Warning (reproduced)
- Header false positive. `Note: please remit to treasury.
SKU-77 ships tomorrow.is split asheader_block+paragraph_unit. Any Name: value` line at the start of a document is treated as RFC822.
- Raw
data:imagebase64 is submitted as an embeddable part and then averaged by token weight. No OCR, no object tags, no skip. - Günther 2024 (late chunking) is the wrong paper for early-split-then-average. Late chunking exists to avoid this failure mode.
- Submit docstring at
cost_router.py:279-280still says each input becomes oneEmbeddingBatchRequest.
Why #652 is the landing vehicle
#643 (ec88eea) |
#652 (3f94151) |
|
|---|---|---|
| Default naruon path | Always splits, then averages | Token-budget path kept |
| Searchable units | Unread meaning_units side channel |
Opt-in chunking_strategy=meaning_units + chunk_units |
| Escape hatch | None | Omit / JSON null; unknown → 400 invalid_chunking_strategy |
| Token-budget splitter | Deleted | Kept |
| HTTP honesty | Missing | test_embeddings_meaning_units_http_honesty.py |
| Cost paper | None | Qu et al. (2025) |
Do not implement the opt-in design on this branch. That would be a third meaning-unit slice. Close #643. Review and land #652. Independent non-author APPROVE is still required on #652; this run will not self-approve or merge.
Next action
Close #643. On #652, keep omit/null as the naruon one-vector path. Do not fold honesty-stack PRs onto either tip.
Sent by Cursor Automation: Fix Issues
| max_tokens=max_tokens, | ||
| max_chars=max_chars, | ||
| count_tokens=self._count_embedding_tokens, | ||
| ) |
There was a problem hiding this comment.
Always-on: every /v1/batch/embeddings input now goes through split_meaning_units, and _force_token_safe_chunks is deleted.
Repro on this head: the fixture AP email under the default 280k/240k ceilings becomes 4 provider parts (header, invoice, SKU, data:image) instead of 1. Those four vectors are then token-weighted-averaged into embeddings[0]. The naruon contract test stays green because alpha body / beta attachment / gamma attachment have no headers, blank lines, or images.
Restore the token-budget splitter as the default. Gate meaning-unit expansion on an explicit request field (chunking_strategy=meaning_units), same as #652. Do not re-implement that opt-in on this branch — close this PR and land #652.
| "meaning_units": meaning_units, | ||
| "map_reduce": { | ||
| "strategy": "token_budgeted_embedding_parts_weighted_average", | ||
| "meaning_unit_strategy": "header_paragraph_sentence_image", |
There was a problem hiding this comment.
embeddings[i] is still a weighted average of the new parts. After the always-on split, that average includes the header block and the raw base64 image. The 1×1 PNG fixture is 15 heuristic tokens vs 20 for the invoice line; a real attachment will dominate the document vector naruon indexes today.
meaning_units is a new, OpenAPI-undocumented side channel. Nothing in this repo or the naruon fixture reads it. Shape (len(embeddings)==len(inputs)) holds; the semantic contract does not.
Default path must keep embeddings[i] as the whole-document (token-budget-only) vector. Unit vectors belong on an opt-in field, and must not be averaged into the naruon slot.
| max_chars=max_chars, | ||
| count_tokens=count_tokens, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
_fallback_split runs only when pack_start is not None. The first span that already exceeds max_tokens / max_chars is stored and flushed at the end as a single sentence_unit.
Repro:
text = ("word " * 20).strip() + ". Short stop."
split_meaning_units(text, model="t", max_tokens=3, max_chars=240000, count_tokens=count)
# -> sentence_unit tokens=21 + sentence_unit tokens=3If the current piece alone exceeds budget, call _fallback_split before starting a pack (same as the later-span branch). This bug is moot if #643 closes; #652 must not copy this pack loop.
|
|
||
| TokenCountFn = Callable[[str, str], int] | ||
|
|
||
| _HEADER_LINE_RE = re.compile(r"^[A-Za-z][A-Za-z0-9-]*:[ \t].+") |
There was a problem hiding this comment.
^[A-Za-z][A-Za-z0-9-]*:[ \t].+ treats any leading Name: value line as RFC822.
Repro: `Note: please remit to treasury.
SKU-77 ships tomorrow.→header_block+paragraph_unit`. Body copy that looks like a header is split off on every document, not just mail.
Use a closed header set (From|To|Cc|Bcc|Subject|…) and only when the leading block looks like email. #652 already does this; do not patch it here.
|
|
||
| ### Added | ||
|
|
||
| - Meaning-unit embeddings chunking for `/v1/batch/embeddings`: header, paragraph, sentence, and `data:image` units keep source offsets so naruon can search SKU lines and senders without mixing them into a due-date vector. The naruon one-vector-per-input reduce is unchanged; read `meaning_units` for unit-level search. |
There was a problem hiding this comment.
This changelog tells a buyer to read meaning_units for SKU-level search and claims the naruon reduce is unchanged. The reduce shape is unchanged; the reduced vector is not. Default-path mail now averages header + paragraphs + raw base64 into the one slot naruon indexes.
Do not ship this wording. The buyer next action belongs on #652: send chunking_strategy=meaning_units, then search chunk_units. Omit/null keeps the current one-vector contract.
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headec88eea7fa82b2b7cd8bfb0e9122e0e0d955d525. -
Head SHA:
ec88eea7fa82b2b7cd8bfb0e9122e0e0d955d525 -
Workflow run: 32162735983
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (7 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (7 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (6 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (6 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test (2 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (2 files)"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (7 files)"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Changed file (7 files)"]
R1 --> V1["required checks"]
Evidence --> S2["Docs (6 files)"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs (6 files)"]
R2 --> V2["docs review"]
Evidence --> S3["Test (2 files)"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test (2 files)"]
R3 --> V3["targeted test run"]
|
2 participants
Summary
/v1/batch/embeddingsinputs on meaning units (email header block, paragraph, sentence,data:imageURI) instead of word/midpoint packing.source_start/source_end/unit_kindso naruon can search a SKU line or sender without mixing it into a due-date vector.meaning_unitsfor unit-level search.docs/papers/README.md, PDFs vendored).Why this PR
Open honesty-stack PRs (#623, #621, #624, #628, #629, #631) already own their slices. Embeddings search quality had no landing vehicle: a real AP email with Invoice 1042 and SKU-77 was packed across the paragraph boundary.
Test plan
python3 tests/test_meaning_unit_chunking.pypython3 tests/test_batch_embeddings.py(naruon contract, oversized word split, char guard)python3 tests/test_api_contract.pypython3 tests/test_conventions.pypython3 tests/test_self_check.pyLanding
Independent of the honesty stack. Do not merge in parallel onto a stacked honesty tip. Reviewer
seonghobaerequested. Do not self-approve.