fix: EmbeddingBasedDocumentSplitter can't split exactly-two-group docs - #12434
fix: EmbeddingBasedDocumentSplitter can't split exactly-two-group docs#12434Nimra3261 wants to merge 3 commits into
Conversation
_find_split_points compares each consecutive-pair distance against a percentile threshold computed over all distances. With exactly two sentence groups there is only one distance, and np.percentile of a single-element array always returns that same element - so `distance > threshold` reduced to `distance > distance`, always False, regardless of percentile setting (including the most aggressive, percentile=0.0). Any short document with a clear topic break that happens to tokenize into exactly two sentence groups silently failed to split. Special-case the single-distance scenario: treat the lone gap as a split point unless the two groups are identical. Verified this doesn't change behavior for documents with >=3 groups. Added tests proving fail-then-pass, plus a release note per project convention.
|
@Nimra3261 is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
|
| # split, regardless of how dissimilar the groups are or how low `percentile` is set. Instead, treat the | ||
| # lone gap as a split point unless the two groups are identical. | ||
| if len(distances) == 1: | ||
| return [1] if distances[0] > 0 else [] |
There was a problem hiding this comment.
I ran this against 6c2bfb8 in a clean python:3.11-slim container. The diagnosis matches, but distances[0] > 0 only excludes bit-identical embeddings, so in practice every two-group document now splits, and percentile stops applying to it.
Two near-identical groups ([1.0, 0.0] and [1.0, 0.001], cosine distance 5e-07), through run() on "The cat sat on the mat. The cat sat on the rug." with sentences_per_group=1, min_length=0:
percentile=1.0 main c7cb46c0 -> 1 chunk PR 6c2bfb84 -> 2 chunks
percentile=0.95 main c7cb46c0 -> 1 chunk PR 6c2bfb84 -> 2 chunks
_find_split_points at PR head, percentile=1.0: two groups -> [1], three groups -> []
At percentile=1.0 the threshold is the max distance, so nothing exceeds it and a document of three or more groups is never split. A two-group document is now always split at that same setting.
If the intent is to split when the lone gap is meaningful, that needs an absolute distance, because a percentile over one sample carries no information. An explicit floor here (a parameter, or a module constant) would say so, and the docstring could note that percentile does not apply to a single gap. I only tested with mock embeddings, not a real embedder.
There was a problem hiding this comment.
Good catch, and confirmed — I reproduced it locally: at percentile=1.0 (the setting that should mean "almost never split"), two near-duplicate groups (cosine distance ~5e-7) still split under distances[0] > 0, while the same near-duplicate pair as part of 3+ groups correctly did not. So percentile was being silently ignored for exactly the two-group case.
Went with your suggested approach: np.percentile on a single-element list always returns that element, so distance > threshold is inherently self-referential and can never be True for n=1 — there's no distribution to make percentile meaningful there, no matter how the comparison is framed. So this now falls back to a fixed absolute cosine-distance floor (_MIN_SPLIT_DISTANCE_FOR_SINGLE_GAP = 0.01) for the two-group case only, documented as an explicit exception in both the code and the percentile param docstring. Added a regression test covering near-duplicate groups across percentile 0.0–1.0. Pushed as 2451545.
There was a problem hiding this comment.
Confirmed at 2451545 in a clean python:3.11-slim container. Near-duplicate groups no longer split at any percentile, and the two-group split this PR exists to deliver still happens once the gap clears the floor.
floor: 0.01
[1] two near-duplicate groups (cosine distance 5.0e-07), _find_split_points
percentile 0.0 / 0.5 / 0.95 / 1.0 -> [] [] [] []
[2] two groups at increasing separation, percentile=1.0
5.0deg distance=0.003805 -> []
8.0deg distance=0.009732 -> []
10.0deg distance=0.015192 -> [1]
90.0deg distance=1.000000 -> [1]
[3] three near-duplicate groups: percentile 0.95 -> [1], 1.0 -> [] (percentile path unchanged)
The floor works out to roughly 8.1 degrees of angular separation. I am still on mock embeddings, so I have not checked where a real embedder puts the distance for a genuine topic change.
There was a problem hiding this comment.
You're right to flag that — I tested it against a real local model (all-MiniLM-L6-v2, mean-pooled) instead of mocks, and the 0.01 floor doesn't hold up:
same-topic paraphrase pairs (should NOT split): cosine distance 0.085 - 0.273
genuinely different-topic pairs (should split): cosine distance 0.975 - 1.084
0.01 is 10-30x too small for this model — it only filters near-bit-identical text, not real paraphrasing, so in practice the two-group case was still "always split" with extra steps. And a fixed constant can't generalize across embedders anyway: some models compress cosine similarity into a much narrower band than others, so a floor tuned to one model's distribution could easily sit outside the useful range for another (either always triggering or never triggering, depending on which way the compression goes).
Given that, in 075cb53 I replaced the hardcoded floor with an explicit min_distance_for_two_groups parameter (default None = never split, matching the original pre-fix behavior). It's opt-in and documented as something to calibrate against your own embedder's distance distribution rather than a value the component can reasonably pick for you. Two-group splitting isn't automatic anymore, but it's now actually usable when you turn it on, instead of quietly ignoring percentile by default.
…roup docs The previous fix for the two-group case (distances[0] > 0) split on any nonzero distance, which meant percentile was silently ignored whenever a document tokenized into exactly two sentence groups: even at the most conservative percentile=1.0, two near-duplicate groups would still split, while the same near-duplicate pair as part of 3+ groups correctly would not. percentile is mathematically undefined for a single-element distribution, so this now falls back to a fixed absolute cosine-distance floor instead of comparing the single distance to itself or to zero.
Empirically verified against a real local embedding model (MiniLM) that a fixed absolute cosine-distance floor doesn't generalize: same-topic paraphrase pairs land around 0.09-0.27 distance while genuine topic changes land around 0.98-1.08 for that model, so the previous floor of 0.01 barely filtered anything beyond near-bit-identical text. Different embedding models compress or spread cosine distances very differently, so there is no single constant that works across embedders. Replaced it with `min_distance_for_two_groups`, defaulting to None (never split a two-group document, matching prior behavior) and documented as something users calibrate against their own embedder's distance distribution rather than a value this component can pick for them.
3 participants
Related Issues
Proposed Changes:
_find_split_pointscompares each consecutive-pair embedding distance against a percentile threshold computed over all distances in the document. With exactly two sentence groups there is only one distance to compare, andnumpy.percentileof a single-element array always returns that same element — sodistance > thresholdreduced todistance > distance, which is alwaysFalse, at everypercentilesetting including the most aggressive (0.0). Any short document with a clear topic break that happens to tokenize into exactly two sentence groups silently failed to split, with no error or warning.Fix: special-case the single-distance scenario in
_find_split_points— treat the lone gap as a split point unless the two groups are identical (distance == 0). Documents with ≥3 groups are unaffected; verified the existing multi-group test scenario is unchanged.How did you test it?
Added three unit tests: two directly on
_find_split_points(splits when groups differ, doesn't split when identical), and one end-to-end throughrun()with a real two-sentence-group document. Reverting the fix, the two positive-case tests fail (assert [] == [1],assert 1 == 2); with the fix, the full test file passes (41 passed, 10 skipped needing API keys). Ranruff check,ruff format --check, andmypyon the changed source file — all clean.Notes for the reviewer
This PR was implemented with the help of an AI assistant. I reviewed the diagnosis and fix, and independently re-ran the full test suite (reverted-fix-fails / fix-restored-passes) and lint/type checks myself before opening this PR.
Checklist
fix:) for my PR title.