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.

Sign up for GitHub

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

[chore]: Pypdfium2 compatibility fix #1239

Merged
merged 3 commits into from
Jul 10, 2023
Merged

[chore]: Pypdfium2 compatibility fix #1239

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
09231ad
provide compatibility with pypdfium2 >= 4.0.0
felixT2K Jul 4, 2023
ac5edc2
change to pdfpage
felixT2K Jul 4, 2023
b70de51
add numpy deprecation fix
felixT2K Jul 10, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 3 additions & 8 deletions doctr/io/pdf.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
# This program is licensed under the Apache License 2.0.
# See LICENSE or go to <https://opensource.org/licenses/Apache-2.0> for full license details.

from pathlib import Path
from typing import Any, List, Optional

import numpy as np
Expand Down Expand Up @@ -31,16 +30,12 @@ def read_pdf(
scale: rendering scale (1 corresponds to 72dpi)
rgb_mode: if True, the output will be RGB, otherwise BGR
password: a password to unlock the document, if encrypted
kwargs: additional parameters to :meth:`pypdfium2.PdfDocument.render_to`
kwargs: additional parameters to :meth:`pypdfium2.PdfPage.render`

Returns:
the list of pages decoded as numpy ndarray of shape H x W x C
"""

if isinstance(file, Path):
file = str(file)

# Rasterise pages to numpy ndarrays with pypdfium2
pdf = pdfium.PdfDocument(file, password=password)
renderer = pdf.render_to(pdfium.BitmapConv.numpy_ndarray, scale=scale, rev_byteorder=rgb_mode, **kwargs)
return [img for img, _ in renderer]
pdf = pdfium.PdfDocument(file, password=password, autoclose=True)
return [page.render(scale=scale, rev_byteorder=rgb_mode, **kwargs).to_numpy() for page in pdf]
Copy link
Contributor

Choose a reason for hiding this comment

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

No, please don't do this. This will result in a major slowdown compared to the multi-page renderer, as mentioned in the other thread.

Copy link
Contributor

Choose a reason for hiding this comment

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

😅 but what would you suggest ? As mentioned using the render from PdfDocument instead of the PagePdf seems to be broken for bytes input... which is a bit confusing 😁

What we need for the moment is a workaround to be compatible with your current and future versions... the io module refactoring to generators is another topic :)

Copy link
Contributor

@mara004 mara004 Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

just write a tempfile, as I already said, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sry i missed this part in your comment 🙈 do you know how big the impact is ? I think there shouldn't be much difference from current usage !?
I would really don't like to start with tricks like this just to ensure compatibility if we probably rewrite the io module after the next release one way or another.

Copy link
Contributor

@mara004 mara004 Jul 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

No problem. The performance advantage depends on use case and host system (or config): If len(pdf) >= n_processes, the multi-page renderer will be factor n_processes faster (well, minus process pool setup time).

If you only use it with single-page PDFs, then the multi-page renderer is actually slower ATM. (It should really handle that as special case and render the one page directly instead of setting up the pool. I wanted to change that a while ago but got distracted by other tasks...)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @mara004 👋 ,
Thanks for sharing your findings.
Ok that was just a guess on my part, as I noticed similar behavior in another context before. 😅
But nice if you can take some insight from the test for your library as well. 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

I think too. Until we go ahead with generators we can keep this as robust solution for the moment. :)

Copy link
Contributor

@mara004 mara004 Jul 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Simply using linear rendering should be OK for doctr for now.

However, it turns out the situation is more complicated for pypdfium2's CLI, which I use for testing.
I figured the problem was that we had a fairly expensive PIL save call in the main process, not in workers, which meant the process pool queued up results in memory without limit while the generator in the main process lacked behind. (That may also be something to take into account for the upcoming refactoring of doctr...)
Now when including the save call in workers parallelization is still a major advantage for our CLI.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I tried to catch up on the discussion, I'm fine to stay out of tempfile hacks. Also, a bumping pypdfium2 version is fine to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the green light @odulcy-mindee.

(Not that it matters now, but as already mentioned I wouldn't call tempfiles a hack in that case. With multiprocessing data transfer is just a necessity, and writing a tempfile once is much better than piping a large bytes object with each job.)

2 changes: 1 addition & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ dependencies = [
"scipy>=1.4.0,<2.0.0",
"h5py>=3.1.0,<4.0.0",
"opencv-python>=4.5.0,<5.0.0",
"pypdfium2>=3.3.0,<4.0.0",
"pypdfium2>=4.0.0,<5.0.0",
"pyclipper>=1.2.0,<2.0.0",
"shapely>=1.6.0,<3.0.0",
"langdetect>=1.0.9,<2.0.0",
Expand Down