Repository navigation
fix: stop copying a chunk's orig_elements twice on every serialization - #4472
Conversation
Repro-first proofBugfix — ElementMetadata.to_dict and _fix_metadata_field_precision each deep-copied a chunk's whole orig_elements list and discarded the copy Reproduced the broken state
Failing test (red)
Fix
Proof it's resolved
One probe, both states, interleaved in a single process, min of N (bench2.py): Auto-generated from this branch's |
|
Strong review done (2026-09-05, fable + gpt-pro) -- verdict: two blocking findings, both fixed in |
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
dbc99bd to
895e676
Compare
tabossert
left a comment
There was a problem hiding this comment.
The code changes look correct: to_dict() now deep-copies only the non-separately-serialized fields while keeping declared key order, and _fix_metadata_field_precision() skips the copy when nothing needs rounding and mints the id before copying, so element_id is stable across serializations. Approving on the code, but this cannot merge until the conflict below is resolved.
Blocking
- Merge conflict with main in
CHANGELOG.mdandunstructured/__version__.py(the only two conflicting files;git merge-treeagainst current main). This PR has0.27.11-dev0, but main is at__version__ = "0.27.16"with 0.27.11 through 0.27.16 already released. Keeping this side when resolving would move__version__backwards and file the entry under an already-shipped 0.27.11. The changelog enforcer andmake check-version(scripts/version-sync.sh) only check that the files changed and that no release is duplicated, so CI would not catch a lower version. Fix: merge or rebase onto main, move the entry into a new## 0.27.17-dev0section above## 0.27.16, set__version__ = "0.27.17-dev0", and runmake check-versionbefore pushing. The code files need no changes (none of them have moved on main since the merge-base).
Non-blocking
-
unstructured/staging/base.py:492: the docstring says "the caller's element is not modified", but_ = element.idat l.506 assigns and caches a uuid on the caller's element when it has none. That mutation is intended (the CHANGELOG says ids are now minted on the caller's element, and the inline comment above l.506 explains why), but the docstring contradicts both. Someone trusting it could drop_ = element.idas a no-op and bring back unstable ids for elements with coordinates ordetection_class_prob. Suggest: "the caller's element is never rounded, but if it has no id, a uuid is minted and cached on it soelement_idstays stable across calls." -
test_unstructured/documents/test_elements.py:458: the docstring ofand_it_emits_the_separately_serialized_fields_in_their_declared_positionnameskey_value_pairsas one of the fields whose position must be kept, but the fixture never sets it and the expected key list does not include it. It is the only one of the fourSEPARATELY_SERIALIZED_FIELD_NAMESthe test does not pin. The placeholder rebuild treats all four the same way today, but ifkey_value_pairsis ever special-cased, a key-position change (and changed bytes from consumers that do not sort keys) would go unnoticed. Suggest addingkey_value_pairs=[FormKeyValuePair(...)](with aTextcustom_element) to the fixture and putting"key_value_pairs"between"filetype"and"languages"in the expected list, or drop it from the docstring.
Questions
unstructured/staging/base.py:488: was rounding the serialized dicts considered instead of copying Elements? All three callers (l.257, 454, 477) run this helper and thenelements_to_dicts(), and all dump withsort_keys=True.coordinates.to_dict()emitspointsunchanged anddetection_class_probpasses through as-is. Both fixes this helper has needed (nestedorig_elementsids, and the mint-before-copy ordering for_ = element.id) come from copying an Element just to round two values. Roundingd["metadata"]["coordinates"]["points"]afterelements_to_dicts()(1 decimal whensystem == "PixelSpace", which matches the currentisinstancesince there are no subclasses, else 2) andd["metadata"]["detection_class_prob"](5 decimals) would copy nothing, mint ids on the caller's element naturally, drop the newCoordinatesMetadataimport, and should produce identical bytes. The two tests attest_unstructured/staging/test_base.py:1016and:1031that call the private helper would need rewriting. If you rejected this, a line in the PR description saying why would help.
`ElementMetadata.to_dict()` deep-copied every metadata field and then replaced `coordinates`, `data_source`, `orig_elements` and `key_value_pairs` with their serialized form, so those copies were built and discarded. Separately, `_fix_metadata_field_precision()` copied every element in order to round coordinates and `detection_class_prob`, which most elements do not carry. On a chunk, `orig_elements` holds every source element of that chunk, so a single `to_dict()` duplicated the document twice over. Measured on this branch: `to_dict()` on chunks is 4x to 6x faster, `elements_to_json()` on chunks is 6x to 10x faster, and `elements_to_json()` on elements with no `orig_elements` is about 3x faster. `to_dict()` on plain elements is unchanged. Behavior change worth calling out: `Element.id` mints a uuid on first access and caches it on that element. The discarded copies took those new ids with them, so serializing one chunk twice reported different `element_id`s for the same source elements each time. Those ids are now stable across calls. Elements given an explicit or hash-derived id were never affected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AWf2tTaaSQCbfxVV75BZEe
… copy Both strong-review legs caught the same gap independently. The previous commit only stopped the copy for elements with neither `coordinates` nor `detection_class_prob`; every other element still went through `deepcopy(element)`, the copy minted the uuid, and the original stayed unset, so consecutive serializations still disagreed on `element_id`. That is every hi_res-partitioned element, so the claim of stable ids was false for the common case. Mint the id on the caller's element before copying. Adds the coordinates and detection_class_prob variants of the stability test, both of which fail without the mint. Also corrects the CHANGELOG heading to `0.27.6-dev0` so it matches `__version__`. `scripts/version-sync.sh -c` takes the first semver in the changelog and rewrites `__version__.py` with it, so `## 0.27.6` against `0.27.6-dev0` would have failed `make check` in CI. The precedent is commit 4fe4097, whose heading is `## 0.27.5-dev0`; the release commit is what strips the suffix from both. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AWf2tTaaSQCbfxVV75BZEe
…fields Popping coordinates, data_source, orig_elements and key_value_pairs before the deep copy moved them to the end of the dict when they were added back. Element.to_dict() output is written unsorted by consumers such as the ingest chunker, so the reorder changed their output. Leave those fields in place as placeholders and deep-copy only the rest, which restores main's key order and keeps the copy of orig_elements skipped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
_fix_metadata_field_precision() deep-copied any element carrying coordinates or detection_class_prob, and with it the whole orig_elements subtree, so the nested elements minted a fresh id on every elements_to_json() call. Shallow copy the element and its metadata instead, and build a new CoordinatesMetadata rather than rounding the points in place, so the caller's element stays unrounded and its orig_elements are serialized as they are. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…g returns Adds the constant to the "To add a field" checklist, since a new specially serialized field has to be listed there and the test iterating the constant cannot catch one that is missing. Documents that _fix_metadata_field_precision() may return the caller's own element. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
94f9369 to
8ec7898
Compare
|
@tabossert late answer, and this merged, so here is the record. Blocking, the CHANGELOG and version conflict: resolved. Rebased onto main, the entry sits in a new Non-blocking 1, the contradictory docstring: not fixed. Non-blocking 2, Question 1, rounding the serialized dicts instead of copying Elements: no, that was not tried, and the PR body carries no line saying why. Reading it now your version looks better on the merits, for the reason you give: both corrections this helper has needed (nested |
What & why
Problem: Anyone chunking a document was paying to photocopy the whole thing twice on every serialization, then bin both copies.
ElementMetadata.to_dict()deep-copies every metadata field and then replacescoordinates,data_source,orig_elementsandkey_value_pairswith their serialized form, so the copies of those four are built and thrown away. Separately,_fix_metadata_field_precision()copies every element in order to roundcoordinatesanddetection_class_prob, which most elements do not have. On a chunk,orig_elementsholds every source element of that chunk, so a singleto_dict()duplicated the document twice over. In a profiled local pipeline over 45,000 elements,copy.deepcopyand its helpers accounted for roughly 40% of total run time.There is a correctness consequence too.
Element.idmints a uuid on first access and caches it on that element. Because the copies were the objects that got serialized, they took the freshly minted ids with them and the originals stayed unset, so serializing one chunk twice reported differentelement_idvalues for the same source elements each time.Change: In
to_dict(), deep-copy only the fields that are not separately serialized and rebuild the dict in its original key order, so the four re-serialized fields keep their position. In_fix_metadata_field_precision(), return the element untouched when it has neithercoordinatesnordetection_class_prob; mint the element's id before the copy that remains; and make that copy shallow, rebuildingCoordinatesMetadatainstead of rounding the caller's points in place, soorig_elementsis not duplicated and its nested ids stay stable.Blast radius: 3/5 -- two functions on the shared serialization path that every caller of
to_dict(),elements_to_json(),elements_to_dicts()andorig_elementsinherits; small, self-contained, and revert-safe.Linked ticket
none
Impact
Library users: chunk-heavy serialization gets materially faster, and repeated serialization of the same element now reports stable
element_idvalues for itsorig_elements. Measured on this branch, one probe run in both states, interleaved in a single process, minimum of N:to_dict, noorig_elementsto_dict, 10orig_elementsto_dict, 40orig_elementselements_to_json, noorig_elementselements_to_json, 10orig_elementselements_to_json, 40orig_elementsThe first row is the one that did not improve and reads slightly worse: it pays four extra dict pops and has no
orig_elementsto skip. The machine was under heavy load during timing, so treat the magnitudes as approximate and that row as indistinguishable from noise.Wire contract / clients: the serialized dict is unchanged in structure and in every value except
element_idfor elements that had none assigned, which was previously regenerated on each call. This reacheselements_to_json()andelements_to_ndjson()as well as the ids insideorig_elements. A caller that recorded those ids and expected a later serialization to produce the same ones was already getting different values every time; it now gets the same ones. This holds for an element that carriesorig_elementstogether with its owncoordinatesordetection_class_probas well, because the precision fix no longer deep-copies theorig_elementssubtree. Elements given an explicitelement_id, or one assigned byid_to_hash(), were never affected either way.Metadata key order is preserved. An earlier revision of this branch popped the four separately-serialized fields before the copy and appended them afterwards, which moved
coordinates,data_source,orig_elementsandkey_value_pairsto the end of the dict.elements_to_json(),elements_to_ndjson()and the base64orig_elementsall serialize withsort_keys=Trueso their bytes never changed, butElement.to_dict()andelements_to_dicts()are order-sensitive for a caller that writes them unsorted, and unstructured-ingest's chunker does exactly that. That turnedtest_ingest_srcred on a fixture diff. The current revision rebuilds the dict in the original order and a test pins it.Shared callers that inherit this:
ElementMetadata.to_dict()is reached fromElement.to_dict(), and therefore fromelements_to_dicts()(and itsconvert_to_isd/convert_to_dictaliases),elements_to_json(),elements_to_base64_gzipped_json()andelements_to_ndjson()._fix_metadata_field_precision()is called byelements_to_base64_gzipped_json()(base.py:256),elements_to_json()(base.py:453) andelements_to_ndjson()(base.py:475).Risk / rollback
Low. Two functions, no signature or schema change, revert the commit to back it out. The one deliberate behavior change is the
element_idstability described above.How it was verified
Ran locally on Python 3.13 against this branch. Each of the three new tests was run first against the unfixed sources restored from
HEADwith the tests in place, to confirm it fails for the reason claimed, and then against the fix.Suites run:
test_unstructured/chunking,test_unstructured/documentsandtest_unstructured/staginggive 796 passed and 25 skipped on the current head, with one collection error intest_unstructured/staging/test_huggingface.pyfor a missingtransformers, which is an uninstalled optional dependency rather than a failure.make check-ruffis clean. The widertest_unstructuredtree passes apart from thepartitionandmetricstrees, which needunstructured_inference, andcleaners/test_translate.pyplus the benchmark test, which fail on missingsentencepieceandpytest-benchmarkin my environment and fail the same way without this change.Note on the environment, because it bit me: without the
csv,docx,tsvandxlsxextras installed,test_unstructured/staging/test_base.pyandtest_unstructured/chunking/test_basic.pydo not collect at all, and the run reports a confident 564 passed while silently skipping the file the precision tests live in.Reviewed by fable and GPT-5.5 Pro before this leaves draft. Both independently found that the first commit left ids unstable for any element carrying
coordinatesordetection_class_prob, which is every hi_res-partitioned element, so its stability claim was false for the common case. My own test could not see it, because I built the fixture from bareTextelements with no coordinates. Fixed, with both variants now covered by a parametrized regression test. fable separately caught that theCHANGELOG.mdheading has to carry the-dev0suffix to match__version__orscripts/version-sync.shfailsmake check: the precedent is commit4fe4097, whose heading is## 0.27.5-dev0, and the release commit is what strips the suffix from both.Not verified:
scripts/version-sync.sh -ccould not run locally, since it needs GNU sed 4.3 and macOS ships BSD sed, so CI is the first real check that the heading and__version__agree. I also have not measured this on a GPU or OCR-heavy end-to-end partition, where model inference dominates and this saving is proportionally much smaller.Proof
Repro. Profiled a 201-document, 45,000-element local pipeline with the real chunker under cProfile.
copy.deepcopywas 10.783 s exclusive over 180,000 calls in a 40.018 s run, and with_deepcopy_list,_deepcopy_dict,_keep_aliveand_deepcopy_atomicthe deepcopy machinery totalled about 16.6 s. Reduced to a standalone reproduction of the id half:Decoding the base64 payload on three consecutive calls gave three different
element_idvalues for the same nested element:2ba29618-...,4e806db8-...,7070f009-....Failing tests, run against the unfixed sources with the new tests in place.
After the fix, same tests, same command:
Second round, after review. With the id-mint line removed from
staging/base.py:With it restored:
2 passed, 66 deselected in 1.33s. All three element kinds checked directly:Suites for the touched areas:
The differential probe, one command run in both states. This is the table under Impact; the row that did not move is the point of showing all six.
Still shaky. The timings were taken under load average 55, so their direction and rough scale are solid and the precise multiples are not. The 0.94x row cannot be separated from noise at that load.
Third round, after review. Three more tests, each run against the unfixed sources with the test in place.
Key order,
test_unstructured/documents/test_elements.py:Nested id stability under the precision fix,
test_unstructured/staging/test_base.py, both variants failing at the old head with differing uuids across twoelements_to_json()calls:All three pass after the fix.
The third new test,
test_fix_metadata_field_precision_rounds_a_copy_and_leaves_the_callers_element_unrounded, passes at both heads, because the olddeepcopyalready protected the caller. A test that cannot fail is worth nothing, so it was mutation-checked instead: with the shallow copy plus in-place rounding ofpointsit fails on(1.23, 2.35) != (1.23456, 2.34567).Not re-measured. The timing table above was taken before the key-order rebuild was added. That rebuild is one extra dict comprehension over the same fields, measured at 1.47 ms against 1.42 ms on a 40-element chunk, so the table's magnitudes still stand, but it is not a fresh run.
Not run. The ingest shell fixtures (
test-ingest-src.sh) were not run locally. The key-order claim rests on a directto_dict()probe againstorigin/mainacross three metadata shapes, not on the CI fixture diff, so CI is the first real check of it.Dependencies / merge order
none