Feat/upstage dpe v2 - #1
visionNoob wants to merge 2 commits into
Conversation
![codesolarlght[bot]](https://avatars.githubusercontent.com/in/4604176?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔆 CodeSolar review 🟡 Minor notes
Adds Upstage Document Parse (upstage_dpe_v2) provider integration with pipeline registration, HTTP client, PDF rasterization, HTML/markdown normalization, layout adapter, and label mapper. The provider sends PDFs to Upstage's API, rasterizes them to 300 DPI PNGs, and normalizes the response into ParseBench's internal format.
Findings: 🟠 Medium 1
solar-pro4 (openrouter) · 14 files · +1181/-30 · 345.4s · CodeSolar
Comment /codesolar on this PR to re-run the review.
| assert result.output.markdown == "\n\n".join(e["content"]["html"] for e in markup) | ||
| assert result.output.markdown.count("<table>") == 3 | ||
| assert body == before | ||
| assert result.output.layout_pages[0].items[1].value == "bold old H 2 x 2 & text" |
There was a problem hiding this comment.
🟠 Medium · test — Test assertion expects incorrect plain-text extraction from subscript/superscript HTML
The test asserts result.output.layout_pages[0].items[1].value == "bold old H 2 x 2 & text", but _element_content extracts text via BeautifulSoup(source_html).get_text(" ", strip=True). For <p><b>bold</b> <s>old</s> H<sub>2</sub> x<sup>2</sup> & text</p>, the stripped text nodes join to "bold old H2 x2 & text" (no spaces around the subscript/superscript digits). The expected value should be "bold old H2 x2 & text". The assertion as written will fail.
🔆 CodeSolar
e0a7393 to
128ce06
Compare
There was a problem hiding this comment.
🔆 CodeSolar review ✅ Looks good
🔄 2nd review of this pull request · 1 new commit(s) since the last one · 0 of 1 earlier findings fixed · 1 still open — see the earlier comments
Adds Upstage Document Parse (upstage_dpe_v2) provider with pipeline registration, HTTP client integration, layout adapter, label mapper, and tests. The provider sends PDFs to Upstage's API, normalizes the response to ParseBench output schemas, and registers layout detection support. Hardcoded 1000x1000 page dimensions in the normalize path are the main design question, since Upstage returns normalized [0,1] coordinates but the actual page size is not extracted from the response.
✅ No issues worth flagging.
solar-pro4 (openrouter) · 10 files · +607/-26 · 250.0s · CodeSolar
Comment /codesolar on this PR to re-run the review.
There was a problem hiding this comment.
🔆 CodeSolar review 🟡 Minor notes
🔄 2nd review of this pull request · 1 new commit(s) since the last one · 0 of 1 earlier findings fixed · 1 still open — see the earlier comments
Adds Upstage Document Parse (upstage_dpe_v2) provider, pipeline registration, layout adapter, label mapper, schema entry, and tests. The provider calls Upstage's /v1/document-digitization endpoint with hardcoded enhanced-mode config, normalizes elements into pages + layout pages, and maps categories to the benchmark ontology. Two issues: (1) page dimensions are hardcoded to 1000×1000 in the provider, so layout bboxes are scaled against an arbitrary size rather than the real PDF page size; (2) num_pages uses or 1 while the page-iteration loop uses or 0, so a missing/zero usage.pages yields a 1-page cost charge with no pages emitted.
Findings: 🟠 Medium 1
solar-pro4 (openrouter) · 10 files · +620/-26 · 266.5s · CodeSolar
Comment /codesolar on this PR to re-run the review.
| layout_pages.append( | ||
| ParseLayoutPageIR( | ||
| page_number=page_number, | ||
| width=1000, |
There was a problem hiding this comment.
🟠 Medium · bug — Page dimensions hardcoded to 1000×1000, producing wrong layout bboxes
ParseLayoutPageIR is constructed with width=1000, height=1000 (lines 202-203). The Upstage API returns normalized [0,1] coordinates (validated in _layout_segment at line 56), and UpstageLayoutAdapter scales them by page.width/page.height (adapters.py lines 1760-1763) to produce pixel bboxes. Because the page size is fixed at 1000×1000 regardless of the actual PDF page dimensions, every layout prediction gets incorrect absolute coordinates. The test at test_upstage.py:87 asserts bbox == [100,200,800,600], which only holds under the 1000×1000 assumption. The API response should supply real page dimensions (or they should be read from the source PDF) instead of this constant.
🔆 CodeSolar
128ce06 to
0c0d09a
Compare
There was a problem hiding this comment.
🔆 CodeSolar review ✅ Looks good
🔄 4th review of this pull request · 1 new commit(s) since the last one · 0 of 1 earlier findings fixed · 1 still open — see the earlier comments
Adds the Upstage Document Parse (upstage_dpe_v2) provider, pipeline registration, layout adapter, label mapper, schema entry, and tests. The provider renders PDFs/images to PNG and calls the Upstage API. A previous finding about hardcoded 1000×1000 page dimensions remains unfixed.
✅ No issues worth flagging.
solar-pro4 (openrouter) · 10 files · +700/-26 · 794.1s · CodeSolar
Comment /codesolar on this PR to re-run the review.
There was a problem hiding this comment.
🔆 CodeSolar review 🔴 Changes recommended
🔄 5th review of this pull request · 1 new commit(s) since the last one
Adds an Upstage Document Parse (upstage_dpe_v2) provider with pipeline registration, HTTP client, PDF rasterization, element normalization, layout adapter, and label mapper. Two correctness issues found: multi-page PDFs only upload the first page, and layout page dimensions are hardcoded to 1000×1000 instead of the actual rendered image size.
Findings: 🔴 High 1 · 🟠 Medium 1
solar-pro4 (openrouter) · 11 files · +1031/-26 · 272.1s · CodeSolar
Comment /codesolar on this PR to re-run the review.
| try: | ||
| if len(document) == 0: | ||
| raise ProviderPermanentError(f"Cannot render empty PDF: {path}") | ||
| image = document[0].render(scale=float(dpi) / 72).to_pil() |
There was a problem hiding this comment.
🔴 High · bug — Multi-page PDFs only upload the first page to the API
In _upload_payload, the PDF branch renders only document[0] (line 102) and sends that single PNG to the API. For any multi-page PDF the remaining pages are silently dropped — the API receives no data for them, yet normalize still iterates over usage.pages and emits empty pages for 2..N. The test test_pdf_is_rendered_to_300_dpi_png only exercises a single-page PDF, so this path is untested. Either the provider must render and upload every page (and the API must support multi-image uploads or per-page calls), or the pipeline must be documented as single-page-only.
🔆 CodeSolar
| layout_pages.append( | ||
| ParseLayoutPageIR( | ||
| page_number=page_number, | ||
| width=1000, |
There was a problem hiding this comment.
🟠 Medium · bug — Layout page dimensions are hardcoded to 1000×1000 instead of actual rendered image size
ParseLayoutPageIR is constructed with width=1000, height=1000 (lines 223–224) regardless of the actual rendered image dimensions. The UpstageLayoutAdapter then multiplies the normalized [0,1] segment coordinates by these hardcoded values (adapters.py lines 1759–1763) and reports image_width/image_height of 1000 (lines 1779–1780). The PNG sent to the API is rendered at 300 DPI, so a US Letter page becomes ~2550×3300 px — the layout bboxes are therefore in a wrong coordinate space. Other layout adapters (e.g. DatabricksAiParseLayoutAdapter) use the page's real width/height from the source data. The fix is to capture the rendered image dimensions in _upload_payload and propagate them into the layout pages.
🔆 CodeSolar
2 participants
No description provided.