AP-859 add basic worker task - #2
Conversation
- (temporarily) mounts tests for quicker development - turns on autodiscovery of celery tasks - sketches out a run_tesseract_job with expected params - sample tests for the job - Note: meta params added to job's update_state. This is optional and can be omitted if we don't want it for debugging/tracking.
anarchivist
left a comment
anarchivist
left a comment
There was a problem hiding this comment.
i know you're still working on this, but it's looking good so far!
- adds tesseract to worker - also adds a couple languages for local dev/testing - mounts a files folder, but this will probably change? - updates tasks.py to actually use tesseract on a filelist - adds a couple of fixture files for future tests
- also handles an output ending in `.pdf`
steve-sullivan
left a comment
steve-sullivan
left a comment
There was a problem hiding this comment.
r+ w/a few questions and a tweak!
There was a problem hiding this comment.
Since fra is one of our default languages, I'm assuming we'd want to include tesseract-ocr-fra here as well?
There was a problem hiding this comment.
Should we verify that the output directory exists, or just trust that the caller has already validated it?
There was a problem hiding this comment.
It seems to me like a good idea to ensure the output directory exists, since that's a clear error that could have a much simpler message than CalledProcessError with a bunch of Tesseract output.
There was a problem hiding this comment.
Just to make sure I'm following this correctly — with subprocess.run monkeypatched here, you're not actually calling Tesseract to process the file, right? You've added an actual .png file, so is there an intent to have some integration(ish) testing later?
There was a problem hiding this comment.
I don't think we need to run integration tests, but I thought it would be helpful for local development to have a sample image with text available.
awilfox
left a comment
awilfox
left a comment
There was a problem hiding this comment.
r+wc agree with Steve's suggestions, after that this looks good to me as a first pass.
There was a problem hiding this comment.
It seems to me like a good idea to ensure the output directory exists, since that's a clear error that could have a much simpler message than CalledProcessError with a bunch of Tesseract output.
anarchivist
left a comment
anarchivist
left a comment
There was a problem hiding this comment.
rw+c (i think just Steve and Anna's remaining comment to verify that the output directory exists). plus a couple of comments - i don't think those are necessarily blocking, but i'd like us to make a conscious decision on them.
| if not output_dir.exists(): | ||
| raise FileNotFoundError(f"Output directory {output_dir} does not exist.") | ||
|
|
||
| # The state update does not need any meta, unless we want it for debugging or tracking. |
There was a problem hiding this comment.
is this because the task already has the metadata on it?
| command = ["tesseract", "-l", "+".join(languages), filelist, output, "pdf"] | ||
| subprocess.run(command, check=True, capture_output=True, text=True) |
There was a problem hiding this comment.
is there a reason why we're invoking tesseract ourselves using subprocess.run() rather than doing it through pytesseract?
There was a problem hiding this comment.
From what I understand, the way I do it here is basically equivalent to using pytesseract as pytesseract is essentially just a wrapper. In the interest of not blocking things, I'm going to merge this for now, and I'll take a look at using pytesseract in my follow-on work.
| tesseract-ocr-eng \ | ||
| tesseract-ocr-spa \ | ||
| tesseract-ocr-deu \ | ||
| tesseract-ocr-chi-tra \ | ||
| tesseract-ocr-ita \ | ||
| tesseract-ocr-fra \ |
There was a problem hiding this comment.
are we sure that we want to install these directly into the image? i thought we'd decided that we wanted to mount a tessdata directory with the .traineddata files into the container to keep the image size small. that said, these specific language data files only add about 100 MB to the image, so maybe there's a tradeoff about which we include in the image vs. which we have to bring in separately.
There was a problem hiding this comment.
I think we should keep them for now for development. I'm guessing it's the chinese language data that's bloated the size.
4 participants
Very basic reimplementation of the perl tesseract caller for a given filelist, list of languages, and output path