fix(embedding): split overlength text before inference - #2599
Conversation
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
|
The remaining BO767 runtime gate has passed on the accuracy-first production stack containing this patch.
This closes the draft gate. Timing was considered only after exact identity and receipt parity passed; the result used to release this PR is correctness, not a speed claim. |
7a4ca55 to
61781a0
Compare
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
61781a0 to
8b22fdf
Compare
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Oversized-input correctness qualification — full BO767 gate passedHead Incidence:
Targeted validation at
Full BO767 normal batch path at
Scope remained accuracy-first: no batching, Ray scheduling, model configuration, extraction/chunking defaults, or retrieval settings changed. Caveat: this fresh extraction produced one fewer pre-embedding row than the earlier incidence snapshot (83,340 vs. 83,341), upstream of embedding and consistent with known independent-run extraction variability. The full-control comparison is same-host and configuration-equivalent at the relevant ingestion/model settings, but it is not a same-stream replay; strict row/ranked-list identity is not claimed. Conclusion: the oversized-input path is rare in these corpora, but the two real BO767 cases now pass end-to-end without truncation, row loss, or attribution loss. This is production protection, not evidence that normal traffic routinely overflows. |
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
| chunk_id: str, | ||
| chunk_index: int, | ||
| chunk_count: int, | ||
| start_token: int, |
There was a problem hiding this comment.
Could we place this under one namespaced mapping, such as metadata["embedding_split"]? These top-level fields include the generic content key and expand the record-wide metadata contract, which makes collisions with extraction and VDB metadata more likely. A nested object would keep split provenance cohesive and easier to evolve.
There was a problem hiding this comment.
Addressed in fdf6dd1. Split provenance now lives in one metadata["embedding_split"] mapping containing exact content, parent/child IDs, order/count, and token range. Because this contract is introduced by this PR, I updated the producer, consumers, docs, and tests directly without a legacy flat-key path.
| ) | ||
| ) | ||
| expanded.append(child) | ||
| return expanded |
There was a problem hiding this comment.
Could we split pure admission/planning from DataFrame expansion and provenance mutation? EmbeddingInputPolicy currently owns tokenizer policy, token planning, row copying, ID derivation, metadata mutation, and pandas construction. Separating a pure split plan from its pandas adapter would make it easier to test and reuse outside this runtime.
There was a problem hiding this comment.
Addressed in fdf6dd1. EmbeddingInputPolicy.plan() now returns immutable, side-effect-free EmbeddingSplitPlan / EmbeddingSplitChild values. The separate prepare_embedding_inputs() pandas adapter owns row copying, stable ID derivation, provenance attachment, and DataFrame construction.
| } | ||
|
|
||
| _CONTENT_PROVENANCE_METADATA_KEYS = ( | ||
| "chunk_index", |
There was a problem hiding this comment.
The generic VDB record adapter now recognizes embedding-specific split children and validates embedding coverage. Is there an existing embedding-boundary helper this can reuse or move behind? Keeping VDB conversion ingestion-agnostic avoids future embedding-policy changes requiring synchronized edits in record conversion.
There was a problem hiding this comment.
Addressed in fdf6dd1. records.py no longer imports split-key lists or interprets/copies individual split fields. It calls the single embedding_record_content() helper, which keeps the split layout behind the embedding schema seam. I retained the generic missing-embedding check at VDB dispatch because that is the point that prevents an incomplete partial write.
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class EmbeddingPreparationResult: |
There was a problem hiding this comment.
Prefix formatting is duplicated between this admission policy and the local embedder implementations that prepare query and passage strings. Since admission counts tokens after prefixing, these need to stay byte-for-byte aligned with actual inference. Could we extract a shared embedding-input formatter so a future prefix-rule change cannot make preflight accounting disagree with the backend?
There was a problem hiding this comment.
Addressed in fdf6dd1. format_embedding_input() is now the shared formatter used by admission, local vLLM prefix application, the local HF text embedder, and the legacy local runtime adapter. The explicit prefix_if_missing argument preserves the HF backend rule while keeping the formatted string identical to what admission counts.
…ding-input-overflow-main-20260827 Signed-off-by: Jacob Ioffe <jioffe@nvidia.com> # Conflicts: # docs/docs/extraction/vdbs.md # nemo_retriever/tests/test_service_packaging.py
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
| nim_http_max_concurrent: int = 32, | ||
| input_type: str = "passage", | ||
| request_timeout_s: float | None = None, | ||
| embedding_input_policy: EmbeddingInputPolicy | None = None, |
There was a problem hiding this comment.
[P2] Consolidate the duplicate embedding runtime
nemo_retriever.common.modality.pipeline still exports a second embed_text_main_text_embed implementation. That path calls create_text_embeddings_for_df() directly, never accepts or applies embedding_input_policy, and hard-codes truncate="END", so callers of the exported compatibility entry point can still silently truncate the overlength text this PR is intended to preserve. It also duplicates the grouping, prefixing, error, and output-column logic being changed here. Could we make that module a thin compatibility wrapper around this implementation (or remove its export if it is truly dead) so there is one admission/runtime path?
There was a problem hiding this comment.
Addressed in 3656624. I confirmed repository-wide that no caller imports the old common.modality.pipeline embedding export; the CPU/GPU actors and tests already use models.inference.runtime. I removed the export and deleted the duplicate 188-line implementation rather than adding a compatibility wrapper. There is now one embedding runtime, so admission policy, no-truncation behavior, grouping, errors, and output cardinality cannot diverge between these two paths. Validation: 353 focused tests passed, plus Black, Flake8, import smoke, and git diff --check.
Signed-off-by: Jacob Ioffe <jioffe@nvidia.com>
Summary
Embedding processes several pages together. Today, one page that is too long can cause two kinds of data loss:
This PR splits only text that is too long before sending it to the embedding model. Normal inputs are unchanged. Nothing is truncated, every piece keeps its document and page information, and an incomplete result is rejected before anything is written to a vector database.
What we observed
This is protection for a rare production case, not a claim that these research datasets regularly overflow.
What changes for users
Technical contract, scope, and validation
Accuracy contract
truncate="NONE"after admission so a remote endpoint cannot silently change the decision.Implementation\n\n- The obsolete compatibility embedding runtime and export are removed; all embedding actors use the single canonical runtime.\n- One model specification owns the exact checkpoint revision, backend, input limits, and prompts for both warmed and cold local embedders.
metadata["embedding_split"].text_imagerows without an image is admitted without changing its existing route or batch size. True image inputs do not resolve text admission.Compatibility and scope
Validation
git diff --checkpass.Suggested review order
truncate="NONE"behavior.