Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
19 changes: 18 additions & 1 deletion Dockerfile

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since fra is one of our default languages, I'm assuming we'd want to include tesseract-ocr-fra here as well?

Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ RUN apt-get update -y && apt-get upgrade -y \
gcc \
libpq-dev \
libxml2-dev \
python3-dev \
python3-dev \
postgresql-client \
&& rm -rf /var/lib/apt/lists/

Expand Down Expand Up @@ -45,3 +45,20 @@ RUN pip install --no-cache-dir -e .
EXPOSE 8000

CMD ["gunicorn", "-w", "4", "-b", "0.0.0.0", "quiabo:app"]

FROM app AS worker

USER root

RUN apt-get update -y \
&& apt-get install -y --no-install-recommends \
tesseract-ocr \
tesseract-ocr-eng \
tesseract-ocr-spa \
tesseract-ocr-deu \
tesseract-ocr-chi-tra \
tesseract-ocr-ita \
tesseract-ocr-fra \
Comment on lines +56 to +61

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we should keep them for now for development. I'm guessing it's the chinese language data that's bloated the size.

&& rm -rf /var/lib/apt/lists/*

USER $APP_USER
6 changes: 4 additions & 2 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ x-quiabo-common:
QUIABO_CELERY__task_default_queue: ${QUIABO_CELERY__task_default_queue:-quiabo}
QUIABO_CELERY__task_ignore_result: ${QUIABO_CELERY__task_ignore_result:-false}


services:
db:
environment:
Expand All @@ -39,7 +38,7 @@ services:
redis:
condition: service_healthy
environment:
<<:
<<:
- *quiabo-common-environment
- *dbconfig
init: true
Expand All @@ -48,10 +47,12 @@ services:
- 8000:8000
volumes:
- ./quiabo:/app/quiabo:rw
- ./test:/app/test:rw

worker:
build:
context: .
target: worker
depends_on:
db:
condition: service_healthy
Expand All @@ -66,6 +67,7 @@ services:
command: celery -A quiabo.celery_app worker --loglevel INFO
volumes:
- ./quiabo:/app/quiabo:rw
- ./files:/app/files:rw

flower:
build:
Expand Down
Empty file added files/.keep
Empty file.
1 change: 1 addition & 0 deletions quiabo/celery.py
Original file line number Diff line number Diff line change
Expand Up @@ -32,5 +32,6 @@ def __call__(self, *args: object, **kwargs: object) -> object:
celery_app = Celery(app.name, task_cls=FlaskTask)
celery_app.config_from_object(app.config["CELERY"])
celery_app.set_default()
celery_app.autodiscover_tasks(["quiabo"], force=True)
app.extensions["celery"] = celery_app
return celery_app
28 changes: 28 additions & 0 deletions quiabo/tasks.py

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Should we verify that the output directory exists, or just trust that the caller has already validated it?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
"""Celery tasks for running OCR jobs."""

import subprocess
from pathlib import Path

from celery import shared_task


@shared_task(bind=True)
def run_tesseract_job(self, filelist: str, languages: list[str], output: str) -> dict:
"""Run Tesseract over the files listed in filelist and write output PDF."""

output = output.removesuffix(".pdf")

output_dir = Path(output).parent
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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is this because the task already has the metadata on it?

self.update_state(
state="STARTED",
meta={"filelist": filelist, "languages": languages, "output": output},
)

command = ["tesseract", "-l", "+".join(languages), filelist, output, "pdf"]
subprocess.run(command, check=True, capture_output=True, text=True)
Comment on lines +25 to +26

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

is there a reason why we're invoking tesseract ourselves using subprocess.run() rather than doing it through pytesseract?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


return {"output": f"{output}.pdf"}
1 change: 1 addition & 0 deletions test/fixtures/files/files.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
/app/test/fixture/files/testocr.png
Binary file added test/fixtures/files/testocr.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
84 changes: 84 additions & 0 deletions test/unit/test_tasks.py

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
"""Unit tests for the shared task defined in ``tasks.py``."""

from unittest.mock import Mock

from quiabo.tasks import run_tesseract_job


def test_shared_task_is_registered():
"""Verify that the Tesseract task is registered with Celery."""
task = run_tesseract_job

assert task.name
assert task.app.tasks[task.name].name == task.name
assert callable(task.run)


def test_shared_task_delay_delegates_to_apply_async(monkeypatch):
"""Verify that delaying the task delegates to ``apply_async``."""
apply_async = Mock(return_value="queued")
monkeypatch.setattr(run_tesseract_job, "apply_async", apply_async)

result = run_tesseract_job.delay("filelist.txt", ["eng", "spa"], "output")

assert result == "queued"
apply_async.assert_called_once_with(
("filelist.txt", ["eng", "spa"], "output"),
{},
)


def test_run_tesseract_job_updates_state_and_runs_tesseract(monkeypatch):
"""Verify that the task updates state and invokes Tesseract."""
update_state = Mock()
run = Mock()
monkeypatch.setattr(run_tesseract_job, "update_state", update_state)
monkeypatch.setattr("quiabo.tasks.subprocess.run", run)

result = run_tesseract_job.run(
"files.txt",
["eng", "spa"],
"output",
)

update_state.assert_called_once_with(
state="STARTED",
meta={
"filelist": "files.txt",
"languages": ["eng", "spa"],
"output": "output",
},
)
run.assert_called_once_with(
["tesseract", "-l", "eng+spa", "files.txt", "output", "pdf"],
check=True,
capture_output=True,
text=True,
)
assert result == {"output": "output.pdf"}


def test_run_tesseract_job_removes_pdf_suffix(monkeypatch):
"""Verify that an existing PDF suffix is removed before invocation."""
update_state = Mock()
run = Mock()
monkeypatch.setattr(run_tesseract_job, "update_state", update_state)
monkeypatch.setattr("quiabo.tasks.subprocess.run", run)

result = run_tesseract_job.run("files.txt", ["eng"], "output.pdf")

update_state.assert_called_once_with(
state="STARTED",
meta={
"filelist": "files.txt",
"languages": ["eng"],
"output": "output",
},
)
run.assert_called_once_with(
["tesseract", "-l", "eng", "files.txt", "output", "pdf"],
check=True,
capture_output=True,
text=True,
)
assert result == {"output": "output.pdf"}
Loading