feat: Add link_format to PyPDFToDocument and PDFMinerToDocument (#10677) - #12273
Conversation
|
@DhanushPillay is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||||||||
julian-risch
left a comment
julian-risch
left a comment
There was a problem hiding this comment.
Thank you for opening this PR @DhanushPillay ! I spotted two issues that we need to fixe before merging:
-
One bad annotation drops the whole document. Link extraction runs inside the try/except that guards the entire conversion, and both implementations raise on shapes that occur in real PDFs. Please wrap per-annotation extraction in its own try/except so that one bad annotation only affects that link and not the whole document.
-
PDFMiner refactor significantly increases peak memory usage because the new code materialises every PDFPage and LTPage before converting. On react_paper.pdf mrmoty usage is 8.4 MB on main and 60 MB here, scaling with page count. Yielding (LTPage, PDFPage) pairs from a generator keeps the annotations available. That could also remove the zip(..., strict=False).
b958463 to
7f209a0
Compare
|
Thanks for the review @julian-risch, really appreciate the catches! I've pushed a fix for both issues:
Let me know if this looks good to merge or if there's anything else! |
Follow-up to the review of deepset-ai#12273. - `PyPDFToDocument` resolved and iterated `/Annots` outside the `try`, so a page whose `/Annots` is an unresolvable reference lost the whole document with `TypeError: 'NoneType' object is not iterable` - the same file converts fine with `link_format="none"`. Link extraction now lives in `_extract_links`, which guards the resolution of the annotation array as well as each individual annotation. The page is indexed rather than using `get`, because only indexing resolves an indirect reference to the array. - Move `resolve1` to the `LazyImport` block of `pdfminer.py` instead of importing it inside `_convert_page` on every page, and repoint the tests that patched `pdfminer.pdftypes.resolve1` at the module under test. - Type `_iter_pages` as `Iterator[tuple[Any, Any]]` and document it. - The `link_format` docstrings promised `[text](address)`, which no PDF converter produces: link annotations carry no anchor text, so the address is used as the link text and the links are appended at the end of the page. - Add a `fixes:` release note for the new `PDFMinerToDocument.to_dict`. Without it, `component_to_dict` falls back to reading same-named attributes, and since the layout parameters live in `self.layout_params` they were silently serialized with their defaults, so `char_margin=0.5` round-tripped as `char_margin=2.0`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
julian-risch
left a comment
julian-risch
left a comment
There was a problem hiding this comment.
Looks good to me now. Thanks again!
julian-risch
changed the title
2 participants
Related Issues
Proposed Changes:
This PR adds a
link_formatparameter to bothPyPDFToDocumentandPDFMinerToDocumentcomponents, matching the existing functionality inDOCXToDocument.DOCXLinkFormatto a reusableLinkFormatEnum inhaystack/components/converters/utils.py.PyPDFToDocumentto parse page/Annotsand append formatted links to the end of the extracted page text.PDFMinerToDocumentto natively iterate overPDFPageobjects (usingPDFPageAggregatorandPDFPageInterpreter) instead of usingextract_pages. This allows access to the page annotations, from which hyperlink URIs are extracted and safely appended to the text.link_formatEnum.How did you test it?
PyPDFToDocumentto verify initialization, serialization, and mock extraction of link annotations.PDFMinerToDocumentto verify initialization, serialization, and mock extraction of link annotations.Notes for the reviewer
DOCXLinkFormathas been aliased toLinkFormatto maintain backward compatibility for users currently importing it directly from the docx module.PyMuPDFto extract links due to its AGPL license, adhering to the discussion in the original issue.