Conversation
There was a problem hiding this comment.
No issues found across 4 files
Shadow auto-approve: would auto-approve. Bug fix in HTML parsing: previously dropped <details>/<summary> content is now extracted, with tests pinned; low-risk mapping change.
Re-trigger cubic
7b06863 to
67191e5
Compare
|
High — a leading image or block wrapper makes a
Please add summary-specific traversal that preserves the title role for the summary label across leading images and ordinary block wrappers, while keeping the details body and post-summary tail outside the title and avoiding a duplicate title for a nested heading. The regression should assert exact element types/order and two separate chunks; covering link/emphasis and page metadata on the label would also protect the annotations carried by this path. (authored by codex) |
Both tags were mapped to `RemovedBlock`, so `partition_html()` dropped every disclosure widget along with everything inside it. Pages that build their FAQ or documentation out of accordions lost all of their questions and answers, and the caller got a shorter element list rather than an error. `<details>` is an ordinary block container, so it becomes `Flow` like `<div>`. `<summary>` is the widget's heading -- the spec calls it "a summary, caption, or legend for the rest of the contents" -- so it becomes `Heading` and emits a `Title`. That is what the reporter asked for, and it is what makes the fix useful downstream: `chunk_by_title()` opens a new section at each `Title`, so a question stays attached to its own answer instead of blending into its neighbours. `<summary>` was listed twice, under both phrasing and the removed-form group; there is now a single mapping. Resolves Unstructured-IO#3919. Signed-off-by: Rohit Behera <126186063+r0h1tb@users.noreply.github.com>
…pper A <summary> whose label followed an <img> (an expand/collapse chevron) or sat inside a <p> or <div> came out as Text or NarrativeText, because Heading only applies Title to its leading text. chunk_by_title() then saw no boundary and merged that entry into the previous one. Give <summary> its own element class that makes every run of label text a Title, descending into plain wrappers while images, nested headings and the like keep their own types.
67191e5 to
51dabe8
Compare
|
Good catch, thanks. Fixed in 51dabe8, and rebased onto main since the branch had gone stale.
New tests assert the exact element types and order plus two separate chunks for the icon case, and cover the |
cragwolfe
left a comment
There was a problem hiding this comment.
Finding
- [P1] Preserve heading hierarchy for summary-generated Titles —
unstructured/partition/html/parser.py:475-483.Summary._iter_label_elements()now emits every summary run asTitle, but_ElementAccumulator._category_depth()maps a non-h1–h6Title to depth 0. The sharedset_element_hierarchy()stack then treats each summary as a top-level heading. I reproduced this on the exact head51dabe8436708a28cb1713a90ad7ca00f78805b9:<h2>FAQ</h2><details><summary>Q1</summary><p>A1</p></details><h2>Shipping</h2><p>x</p>produces Q1 withparent_id=Noneinstead of FAQ, and incorrectly assigns Shipping under Q1; nested Outer/Inner summaries are also siblings instead of parent/child. This silently regressesparent_id/section hierarchy for existing headings and affects downstream HTML/ontology/RAG consumers.
Please fix the hierarchy semantics for summary-derived Titles, preserving real heading levels and making nested details/summary relationships explicit, then add regression assertions for parent IDs and category depths (including a heading after a details block and nested details). The current extraction and focused text/type tests do not catch this.
Verification: uv run --locked reproduction against the exact PR head confirmed the incorrect parent IDs; all reported exact-head CI checks were green, but CI does not cover this hierarchy case.
(authored by codex)
## Problem
`<dl>`, `<dt>` and `<dd>` are mapped to `RemovedBlock`, so
`partition_html()` drops definition lists and everything inside them.
Documentation gets hit hardest: Sphinx renders every documented
function, class and attribute as a `<dl>`, so partitioning an API
reference page returns the headings and nothing else.
```python
partition_html(text=(
'<h1>API reference</h1><dl class="py function"><dt>connect(host, port=5432)</dt>'
"<dd><p>Open a connection to the database server.</p></dd></dl>"
))
# [Title('API reference')]
```
Glossaries and key/value lists disappear the same way, with no error.
## Fix
Map `<dl>` to `ListBlock` and `<dd>` to `ListItemBlock`, which the
parser already anticipates: the `ListBlock` docstring says "maybe a
`<dl>` element at some point", and the list-depth code already counts
`dl` ancestors for `dd` items. `<dt>` becomes a plain `BlockItem`, so a
term gets its type from its text like any paragraph. The v2 (ontology)
parser already keeps definition lists.
## Tests
Two new tests in `test_partition.py`: a glossary (exact element types,
text and `category_depth`) and a nested Sphinx-style API reference (all
text kept, in order). Without the fix they return `[('Title',
'Glossary', 1)]` and `['API reference']`. The html, chunking, documents,
md, text and email tests go from 1201 passed on main to 1203, with no
failures.
This touches the same mapping table as #4451 (`<details>`/`<summary>`),
so whichever lands second will need a small rebase.
<!-- This is an auto-generated description by cubic. -->
<a
href="https://cubic.dev/pr/Unstructured-IO/unstructured/pull/4502?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="Review in cubic"
src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>
<!-- End of auto-generated description by cubic. -->
Problem
<details>and<summary>were both mapped toRemovedBlock, sopartition_html()discarded the whole subtree. On the FAQ page from the issue, every question and answer disappears and the caller gets a shorter element list rather than an error:Fix
<details>is an ordinary block container, so it becomesFlow, like<div>.<summary>becomesHeading. The spec calls it "a summary, caption, or legend for the rest of the contents", the issue asks for the questions as titles, and it is what makes the fix useful downstream —chunk_by_title()opens a new section at eachTitle, so a question stays attached to its own answer instead of blending into the next entry.<summary>was also listed twice, under both phrasing and the removed-form group; there is now one mapping.Scope
Only these two tags.
<dl>,<figure>and the form/nav tags stay inRemovedBlock:<figure>/<figcaption>removal is asserted byDescribeRemovedBlock.it_is_skipped_during_parsing, so it is a deliberate decision, not an oversight.<dl>/<dt>/<dd>also drop their content, andListBlock's docstring already says "maybe a<dl>element at some point". That looks worth doing, but reclassifying it changes the ancestor chain that_category_depthwalks (there is a parametrised case attest_parser.py:298relying on<dl><dd>nesting), so it belongs in its own PR rather than riding along here.Tests
Four tests in
test_unstructured/partition/html/test_partition.py. With the parser change reverted on this branch:Also checked by hand and behaving: unclosed
<details>,<summary>not the first child, two<summary>siblings, empty<summary>,<summary>outside any<details>, attributes on<details>, inline markup inside<summary>,<details>inside a table cell and inside a list item, 20-deep nesting, and a JSON round-trip.Suite before 146 failed / 2531 passed, after 146 failed / 2535 passed — the same 146 IDs, all pre-existing here (pandoc and the ML extras aren't installed locally). 14 modules that cannot be collected without those extras were excluded from both runs.
Note: this bumps to
0.26.4, as does #4449 — whichever lands second needs the version and changelog re-pointed.