Skip to content

perf(html): carry parsed fragments through merging - #4418

Open
KRRT7 wants to merge 4 commits into
Unstructured-IO:mainfrom
KRRT7:perf-html-carry-parsed-fragments
Open

perf(html): carry parsed fragments through merging#4418
KRRT7 wants to merge 4 commits into
Unstructured-IO:mainfrom
KRRT7:perf-html-carry-parsed-fragments

Conversation

@KRRT7

@KRRT7 KRRT7 commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Summary

Carry parsed ontology fragments alongside HTML elements during ontology conversion so adjacent inline elements can be checked for mergeability without reparsing their serialized HTML.

Performance

The inline merge path now parses each element fragment once and reuses the parsed representation across merge checks, avoiding repeated BeautifulSoup parsing for long HTML element streams.

Compatibility

The standalone can_unstructured_elements_be_merged() helper retains its existing behavior and parsing interface.

Testing

  • 339 HTML tests passed
  • Ruff checks passed

Review in cubic

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Shadow auto-approve: would not auto-approve because issues were found.

Re-trigger cubic

Comment thread unstructured/partition/html/transformations.py Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 3 files (changes from recent commits).

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

Comment thread unstructured/partition/html/transformations.py Outdated

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 1 file (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="unstructured/partition/html/transformations.py">

<violation number="1" location="unstructured/partition/html/transformations.py:290">
P3: `_can_ontology_elements_be_merged` silently changed its contract: it no longer treats empty fragment lists as non-mergeable, relying entirely on callers to pre-filter. Both current call sites happen to guard correctly, but the docstring doesn't document this precondition, so a future caller could get an incorrect `True` for degenerate empty-fragment inputs.</violation>
</file>

Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.

Re-trigger cubic

next_depth: int,
) -> bool:
"""Check mergeability using ontology fragments that have already been parsed."""
if current_depth != next_depth:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3: _can_ontology_elements_be_merged silently changed its contract: it no longer treats empty fragment lists as non-mergeable, relying entirely on callers to pre-filter. Both current call sites happen to guard correctly, but the docstring doesn't document this precondition, so a future caller could get an incorrect True for degenerate empty-fragment inputs.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At unstructured/partition/html/transformations.py, line 290:

<comment>`_can_ontology_elements_be_merged` silently changed its contract: it no longer treats empty fragment lists as non-mergeable, relying entirely on callers to pre-filter. Both current call sites happen to guard correctly, but the docstring doesn't document this precondition, so a future caller could get an incorrect `True` for degenerate empty-fragment inputs.</comment>

<file context>
@@ -283,7 +287,7 @@ def _can_ontology_elements_be_merged(
 ) -> bool:
     """Check mergeability using ontology fragments that have already been parsed."""
-    if current_depth != next_depth or not current_ontology_elements or not next_ontology_elements:
+    if current_depth != next_depth:
         return False
 
</file context>

Copy link
Copy Markdown
Contributor

Severity: High — the optimization eagerly reparses and retains every non-layout subtree, including content that can never be merged

Location: unstructured/partition/html/transformations.py:172-178, unstructured/partition/html/transformations.py:220-232, unstructured/partition/html/transformations.py:293-298

Description: _ontology_to_unstructured_elements() now serializes every non-layout ontology element and immediately reparses that HTML into a second ontology subtree. This happens before determining whether the element has an adjacent merge candidate or is structurally eligible for inline merging.

Why this is risky: The parsed fragments are full recursive OntologyElement/Pydantic object graphs and are retained alongside the output elements through the enclosing conversion. Elements with children, tables, forms, media, and other non-inline types are guaranteed to fail _can_ontology_elements_be_merged(), so their additional parse and retained object graph provide no benefit. In particular, a layout containing only one large child previously performed no mergeability parse because the combine loop never compared a pair; it now reparses the entire child unconditionally.

Concrete failure scenario: A request containing a single multi-thousand-row table causes the already-parsed table to be serialized and rebuilt as a duplicate ontology tree while the original tree and serialized HTML remain live. With many such requests processed concurrently, the extra CPU and peak memory can produce severe latency, worker OOM termination, and retry amplification. This makes an important production document shape materially worse despite the change being presented as a performance optimization.

Missing high-value tests: Add a differential performance and peak-memory test for a single large non-mergeable subtree and for mixed inline/block streams. Assert that fragments with no neighbor, with children, or with a categorically non-mergeable type do not incur mergeability parsing/model construction. Include a concurrent-worker stress case rather than testing only long mergeable inline runs.

Verdict: Not Merge.

(authored by Codex)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants