[chore]: Pypdfium2 compatibility fix #1239
Conversation
felixdittrich92
added
topic: build
|
@mara004 temp fix for compatibility |
Codecov Report
@@ Coverage Diff @@
## main #1239 +/- ##
==========================================
- Coverage 95.66% 95.66% -0.01%
==========================================
Files 154 154
Lines 6877 6873 -4
==========================================
- Hits 6579 6575 -4
Misses 298 298
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| 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] |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
😅 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 :)
There was a problem hiding this comment.
just write a tempfile, as I already said, no?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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. 👍
There was a problem hiding this comment.
I think too. Until we go ahead with generators we can keep this as robust solution for the moment. :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
felixdittrich92
changed the title
felixdittrich92
changed the title
| 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] |
There was a problem hiding this comment.
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.
This PR: