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

split up prediction to avoid overly large batches (causing OOM) #116

Merged
merged 6 commits into from
Oct 7, 2024

Conversation

bertsky
Copy link
Contributor

@bertsky bertsky commented Sep 12, 2024

No description provided.

@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 82.56881% with 19 lines in your changes missing coverage. Please review.

Project coverage is 69.50%. Comparing base (4adf09f) to head (842bd92).

Files with missing lines Patch % Lines
ocrd_calamari/recognize.py 82.56% 10 Missing and 9 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #116      +/-   ##
==========================================
- Coverage   71.07%   69.50%   -1.58%     
==========================================
  Files           5        5              
  Lines         204      223      +19     
  Branches       50       57       +7     
==========================================
+ Hits          145      155      +10     
- Misses         48       53       +5     
- Partials       11       15       +4     
Flag Coverage Δ
69.50% <82.56%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@bertsky bertsky mentioned this pull request Sep 16, 2024
@bertsky
Copy link
Contributor Author

bertsky commented Sep 17, 2024

Assuming we can soon rely on Calamari-OCR/calamari#361, this PR should be changed to re-use the batch_size kwarg of predict_raw instead.

@mikegerber mikegerber self-assigned this Sep 19, 2024
@mikegerber
Copy link
Collaborator

Also includes part of #104

@bertsky
Copy link
Contributor Author

bertsky commented Sep 27, 2024

Also includes part of #104

You mean removes that part?

That's because I merged your upstream fixes into Calamari v1.0.7 so we won't need the workaround.

@bertsky
Copy link
Contributor Author

bertsky commented Sep 27, 2024

Assuming we can soon rely on Calamari-OCR/calamari#361, this PR should be changed to re-use the batch_size kwarg of predict_raw instead.

Actually, Calamari1's predict_raw with delegated batch_size param is still inferior to Calamari2's setup with length bucketing based on the corresponding feature of TF2. The problem is that even a single long line (esp. if it is also low in height and thus needs to be scaled up for target height) can extremely increase the memory requirements for the entire batch due to zero padding. So as long as we use Calamari1 we should probably rather do our own "bucketing", or at least split up batches with huge lengths to avoid OOM...

@mikegerber
Copy link
Collaborator

You mean removes that part?
That's because I merged your upstream fixes into Calamari v1.0.7 so we won't need the workaround.

#104 is about removing the workaround, so it overlaps with/is superseded by this PR.

@bertsky
Copy link
Contributor Author

bertsky commented Sep 27, 2024

Actually, Calamari1's predict_raw with delegated batch_size param is still inferior to Calamari2's setup with length bucketing based on the corresponding feature of TF2. The problem is that even a single long line (esp. if it is also low in height and thus needs to be scaled up for target height) can extremely increase the memory requirements for the entire batch due to zero padding. So as long as we use Calamari1 we should probably rather do our own "bucketing", or at least split up batches with huge lengths to avoid OOM...

Implemented in bertsky@45e20b1

@bertsky
Copy link
Contributor Author

bertsky commented Sep 27, 2024

#104 is about removing the workaround, so it overlaps with/is superseded by this PR.

ah, yes, but it does not point to 1.0.7 but to the commit ref – so we should merge both PRs

@mikegerber
Copy link
Collaborator

#104 is about removing the workaround, so it overlaps with/is superseded by this PR.

ah, yes, but it does not point to 1.0.7 but to the commit ref – so we should merge both PRs

Should probably have written: "Note to self:", my comment just served as a reminder for me to have a look at both PRs.

@bertsky bertsky mentioned this pull request Sep 27, 2024
Copy link
Collaborator

@mikegerber mikegerber left a comment

Choose a reason for hiding this comment

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

LGTM.

I'll merge it after a manual test.

@mikegerber
Copy link
Collaborator

Manual test looks OK, merging!

@mikegerber mikegerber merged commit d9cde1f into OCR-D:master Oct 7, 2024
1 check passed
@mikegerber
Copy link
Collaborator

@stweil This may fix the OOM you have seen, but it's hard to tell without the data!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants