Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fixing pdf parsing #3349

Merged
merged 3 commits into from
Oct 14, 2024
Merged

fix: fixing pdf parsing #3349

merged 3 commits into from
Oct 14, 2024

Conversation

jacopo-chevallard
Copy link
Collaborator

Our default pdf parser is Unstructured, for which we were using the 'fast' strategy, which fails in parsing some pdf. We instead use the 'auto' strategy which is more flexible and powerful.

Closes TICK-86

Description

Please include a summary of the changes and the related issue. Please also include relevant motivation and context.

Checklist before requesting a review

Please delete options that are not relevant.

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented hard-to-understand areas
  • I have ideally added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

Screenshots (if appropriate):

…uctured) to use the 'auto' strategy instead of the 'fast' one
Copy link

linear bot commented Oct 8, 2024

@dosubot dosubot bot added the size:XS This PR changes 0-9 lines, ignoring generated files. label Oct 8, 2024
Copy link

vercel bot commented Oct 8, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
quivrapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Oct 14, 2024 0:57am

@dosubot dosubot bot added the area: backend Related to backend functionality or under the /backend directory label Oct 8, 2024
Copy link
Collaborator

@AmineDiro AmineDiro left a comment

Choose a reason for hiding this comment

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

Need to check the impact on infra and resource. Probably some tests to be added and ran on CI before merging

@@ -22,6 +22,6 @@ def from_yaml(cls, file_path: str):


class MegaparseConfig(MegaparseBaseConfig):
strategy: str = "fast"
strategy: str = "auto"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Need to check the impact of this on the worker

Copy link
Collaborator

Choose a reason for hiding this comment

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

@chloedia This was set for a reason in megaparse ? The OCR took too much time ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

good point, indeed the 'auto' option makes it significantly slower, even though, in principle, it should automatically revert to the 'fast' approach when possible... another (smarter) approach would be to first try with 'fast', then fallback to 'auto' if it fails... but still, we risk of parsing only partially some pdf files that would have been correctly (entirely) parsed with 'auto'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

(we should develop some heuristics on when a parsing has failed, besides the obvious case of an empty parsed document)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The heuristic I set up in previous project was to check the area of the "image" vs text in the page.
Parsers like fitz return the the bbox of image, I guess unstructured should return that. Nonnative text in pdfs is represented as an image. a threshold of 0.8 worked but it depends heavily on the document being processed.
Another heuristic is the existence of glyph encoding in the pdf. We should test that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes normally it already fallbacks to auto when fast is empty (according to unstructured) that s why I used it !

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:XS This PR changes 0-9 lines, ignoring generated files. labels Oct 14, 2024
Copy link
Collaborator

@AmineDiro AmineDiro left a comment

Choose a reason for hiding this comment

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

Great

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Oct 14, 2024
@StanGirard StanGirard merged commit 367242a into main Oct 14, 2024
11 checks passed
@StanGirard StanGirard deleted the fix/pdf_parsing branch October 14, 2024 13:21
StanGirard added a commit that referenced this pull request Oct 16, 2024
🤖 I have created a release *beep* *boop*
---


## 0.0.322 (2024-10-15)

## What's Changed
* feat: Add new documentation files by @StanGirard in
#3351
* fix: separate english and french ingredients by @chloedia in
#3358
* docs(core): init by @StanGirard in
#3365
* docs: quivr core storage by @AmineDiro in
#3366
* fix: fixing pdf parsing by @jacopo-chevallard in
#3349
* feat: Improve user credit calculation in get_user_credits by
@StanGirard in #3367
* fix unwanted parsing effect by @chloedia in
#3371
* add fallback on llamaparse by @chloedia in
#3374


**Full Changelog**:
v0.0.321...v0.0.322

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
StanGirard added a commit that referenced this pull request Oct 16, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.0.17](core-0.0.16...core-0.0.17)
(2024-10-16)


### Features

* **assistant:** cdp
([#3305](#3305))
([b767f19](b767f19))
* **assistants:** mock api
([#3195](#3195))
([282fa0e](282fa0e))
* introducing configurable retrieval workflows
([#3227](#3227))
([ef90e8e](ef90e8e))


### Bug Fixes

* fixing pdf parsing
([#3349](#3349))
([367242a](367242a))


### Documentation

* **core:** init ([#3365](#3365))
([bb572a2](bb572a2))
* **fix:** fixed warnings from griffe
([#3381](#3381))
([1a38798](1a38798))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: backend Related to backend functionality or under the /backend directory lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants