Skip to content

Commit b27d7ce

Browse files
committed
feat: improve the performance and add tests
1 parent 0745d39 commit b27d7ce

10 files changed

Lines changed: 895 additions & 7 deletions

File tree

.github/workflows/test.yaml

Lines changed: 182 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,182 @@
1+
name: Tests
2+
3+
on:
4+
push:
5+
6+
concurrency:
7+
group: ${{ github.workflow }}-${{ github.ref }}
8+
cancel-in-progress: true
9+
10+
jobs:
11+
unit-tests:
12+
name: Unit Tests
13+
runs-on: blacksmith-4vcpu-ubuntu-2404
14+
timeout-minutes: 20
15+
steps:
16+
- uses: actions/checkout@v6
17+
18+
- uses: actions/setup-python@v5
19+
with:
20+
python-version: "3.12"
21+
cache: pip
22+
23+
- name: Install system dependencies
24+
run: |
25+
sudo apt-get update
26+
sudo apt-get install -y --no-install-recommends \
27+
ffmpeg libimage-exiftool-perl libmagic1
28+
29+
- name: Install Python dependencies
30+
run: |
31+
pip install -r requirements.txt
32+
pip install pytest reportlab
33+
34+
- name: Run unit tests
35+
env:
36+
PYTHONPATH: ${{ github.workspace }}
37+
run: pytest tests/ -v
38+
39+
k8s-benchmark:
40+
name: PDF Benchmark (Kubernetes)
41+
runs-on: ubuntu-latest
42+
timeout-minutes: 45
43+
permissions:
44+
contents: read
45+
pull-requests: write
46+
env:
47+
IMAGE_TAG: ci-${{ github.sha }}
48+
NAMESPACE: markitdown-server
49+
steps:
50+
- uses: actions/checkout@v6
51+
52+
- name: Set up Docker Buildx
53+
uses: docker/setup-buildx-action@v3
54+
55+
- name: Build image
56+
uses: docker/build-push-action@v6
57+
with:
58+
context: .
59+
platforms: linux/amd64
60+
push: false
61+
load: true
62+
tags: markitdown-server:${{ env.IMAGE_TAG }}
63+
cache-from: type=gha
64+
cache-to: type=gha,mode=max
65+
66+
- name: Start minikube
67+
uses: medyagh/setup-minikube@v0.0.21
68+
with:
69+
driver: docker
70+
cpus: 4
71+
memory: 6000m
72+
addons: metrics-server
73+
74+
- name: Load image into minikube
75+
run: minikube image load "markitdown-server:${IMAGE_TAG}"
76+
77+
- name: Create secret
78+
run: |
79+
kubectl create namespace "$NAMESPACE" --dry-run=client -o yaml | kubectl apply -f -
80+
# PDF conversion never calls the LLM (that path is images only), so a
81+
# placeholder key is enough to satisfy the import-time lookup.
82+
kubectl create secret generic markitdown-server-secret \
83+
-n "$NAMESPACE" \
84+
--from-literal=OPENAI_API_KEY=ci-placeholder \
85+
--from-literal=ADMIN_API_KEY=ci-test-key \
86+
--dry-run=client -o yaml | kubectl apply -f -
87+
88+
- name: Deploy
89+
run: |
90+
cd k8s
91+
kustomize edit set image \
92+
"ghcr.io/sirily11/markitdown-server=markitdown-server:${IMAGE_TAG}"
93+
kubectl apply -k .
94+
# Drop the HPA for the benchmark. Conversion pegs the CPU, so the HPA
95+
# scales to maxReplicas mid-run and then holds there for its 300s
96+
# scale-down window. Two replicas behind the service make per-book
97+
# timing and peak-memory attribution meaningless, since requests and
98+
# the cgroup reads can land on different pods.
99+
kubectl delete hpa markitdown-server-hpa -n "$NAMESPACE" --ignore-not-found
100+
kubectl scale deployment/markitdown-server -n "$NAMESPACE" --replicas=1
101+
102+
- name: Wait for rollout
103+
run: |
104+
kubectl rollout status deployment/markitdown-redis -n "$NAMESPACE" --timeout=300s || true
105+
kubectl rollout status deployment/markitdown-server -n "$NAMESPACE" --timeout=300s
106+
107+
# No port-forward here on purpose: the benchmark restarts the pod between
108+
# books, which tears a forward down, so the script manages its own.
109+
- name: Benchmark Foundation books
110+
env:
111+
ADMIN_API_KEY: ci-test-key
112+
LOCAL_PORT: "8080"
113+
run: |
114+
set -o pipefail
115+
bash scripts/k8s-benchmark.sh | tee /tmp/benchmark.txt
116+
117+
- name: Build benchmark report
118+
if: always()
119+
run: |
120+
# On failure the script appends per-book pod logs, so cap the size:
121+
# a GitHub comment is rejected outright past ~65k characters.
122+
if [ -f /tmp/benchmark.txt ]; then
123+
head -c 55000 /tmp/benchmark.txt > /tmp/benchmark-trimmed.txt
124+
if [ "$(wc -c < /tmp/benchmark.txt)" -gt 55000 ]; then
125+
echo '... output truncated, see the k8s-logs artifact ...' \
126+
>> /tmp/benchmark-trimmed.txt
127+
fi
128+
else
129+
echo 'benchmark did not produce output' > /tmp/benchmark-trimmed.txt
130+
fi
131+
{
132+
echo '<!-- markitdown-pdf-benchmark -->'
133+
echo '## PDF conversion benchmark'
134+
echo
135+
echo '```'
136+
cat /tmp/benchmark-trimmed.txt
137+
echo '```'
138+
echo
139+
echo "Commit \`${GITHUB_SHA:0:7}\` · [workflow run](${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/actions/runs/${GITHUB_RUN_ID})"
140+
} > /tmp/benchmark-report.md
141+
cat /tmp/benchmark-report.md >> "$GITHUB_STEP_SUMMARY"
142+
143+
# Posted even when the benchmark fails -- a budget violation is exactly
144+
# the result worth surfacing on the PR.
145+
- name: Comment benchmark on PR
146+
if: always()
147+
env:
148+
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
149+
run: |
150+
# This workflow runs on push, not pull_request, so the PR number has
151+
# to be looked up from the branch.
152+
PR=$(gh pr list --head "$GITHUB_REF_NAME" --state open \
153+
--json number --jq '.[0].number // empty')
154+
if [ -z "$PR" ]; then
155+
echo "No open PR for branch $GITHUB_REF_NAME; skipping comment."
156+
exit 0
157+
fi
158+
# Edit the previous report rather than stacking a new comment on
159+
# every push; fall back to creating one on the first run.
160+
gh pr comment "$PR" --body-file /tmp/benchmark-report.md --edit-last \
161+
|| gh pr comment "$PR" --body-file /tmp/benchmark-report.md
162+
163+
- name: Collect pod diagnostics
164+
if: always()
165+
run: |
166+
mkdir -p /tmp/k8s-logs
167+
# Per-book logs captured during the run. The pod is restarted between
168+
# books, so these are the only copy of an earlier book's output.
169+
cp -r /tmp/benchmark-pod-logs /tmp/k8s-logs/per-book 2>/dev/null || true
170+
kubectl logs -n "$NAMESPACE" -l app=markitdown-server --tail=-1 \
171+
> /tmp/k8s-logs/server-final.log 2>&1 || true
172+
kubectl get pods -n "$NAMESPACE" -o wide > /tmp/k8s-logs/pods.txt 2>&1 || true
173+
kubectl describe pods -n "$NAMESPACE" -l app=markitdown-server \
174+
> /tmp/k8s-logs/describe.txt 2>&1 || true
175+
176+
- name: Upload diagnostics
177+
uses: actions/upload-artifact@v7
178+
if: always()
179+
with:
180+
name: k8s-logs
181+
path: /tmp/k8s-logs/
182+
retention-days: 7

converter.py

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,8 @@
99
import requests
1010
import tempfile
1111

12+
from pdf_chunk import convert_pdf_chunked, should_chunk
13+
1214
logger = logging.getLogger(__name__)
1315

1416
client = OpenAI(
@@ -93,9 +95,17 @@ def convert(url: str) -> DocumentConverterResult:
9395
downloaded_at = time.monotonic()
9496
logger.info("downloaded %s in %.3fs", url, downloaded_at - started)
9597

96-
md = MarkItDown(llm_client=client, llm_model=model)
9798
try:
98-
converted = md.convert(temp_file)
99+
# Large PDFs are converted chunk-by-chunk across worker processes.
100+
# Done whole-document, markitdown holds the parsed object graph for
101+
# every page at once, which pins the container memory limit and
102+
# serialises all the parsing onto one core.
103+
if should_chunk(temp_file):
104+
markdown = convert_pdf_chunked(temp_file)
105+
converted = DocumentConverterResult(markdown=markdown)
106+
else:
107+
md = MarkItDown(llm_client=client, llm_model=model)
108+
converted = md.convert(temp_file)
99109
finally:
100110
if os.path.exists(temp_file):
101111
os.remove(temp_file)

k8s/deployment.yaml

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -24,18 +24,29 @@ spec:
2424
image: ghcr.io/sirily11/markitdown-server:latest
2525
resources:
2626
limits:
27-
memory: "1024Mi"
28-
cpu: "500m"
27+
# PDF conversion is CPU-bound and now fans chunks out across
28+
# worker processes; the limit has to exceed PDF_MAX_WORKERS
29+
# cores or the workers just contend for the same slice.
30+
memory: "2048Mi"
31+
cpu: "2000m"
2932
requests:
3033
# Baseline RSS is ~120-140Mi; a 128Mi request pinned memory
3134
# utilization at ~100% and kept the HPA at max replicas.
32-
memory: "384Mi"
33-
cpu: "250m"
35+
memory: "512Mi"
36+
cpu: "500m"
3437
env:
3538
- name: REDIS_URL
3639
value: "redis://markitdown-redis:6379/0"
3740
- name: OPENAI_MODEL
3841
value: "google/gemini-2.5-flash-preview-05-20"
42+
# Keep in step with the CPU limit above: each worker saturates
43+
# roughly one core, and each costs ~150-200Mi of resident memory
44+
# for its own markitdown import, so this trades against the
45+
# memory limit too.
46+
- name: PDF_MAX_WORKERS
47+
value: "2"
48+
- name: PDF_PAGES_PER_CHUNK
49+
value: "20"
3950
envFrom:
4051
- secretRef:
4152
name: markitdown-server-secret

k8s/ingress.yaml

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@ metadata:
66
annotations:
77
cert-manager.io/cluster-issuer: letsencrypt-prod
88
nginx.ingress.kubernetes.io/proxy-body-size: "1000m"
9+
# Large PDFs take several minutes to parse; nginx's 60s default cut the
10+
# connection mid-conversion and returned a 504 to the caller.
11+
nginx.ingress.kubernetes.io/proxy-connect-timeout: "60"
12+
nginx.ingress.kubernetes.io/proxy-send-timeout: "600"
13+
nginx.ingress.kubernetes.io/proxy-read-timeout: "600"
914
nginx.ingress.kubernetes.io/affinity: "cookie"
1015
nginx.ingress.kubernetes.io/affinity-mode: "persistent"
1116
nginx.ingress.kubernetes.io/session-cookie-name: "mcp-router-affinity"

pdf_chunk.py

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,134 @@
1+
"""
2+
Page-chunked, multi-process PDF conversion.
3+
4+
markitdown's ``PdfConverter`` holds the parsed pdfplumber object graph for the
5+
*whole* document at once (and, for prose PDFs, then re-parses everything with
6+
pdfminer), so both peak memory and wall-clock grow with document size in a
7+
single process.
8+
9+
Splitting the PDF into page ranges and converting each range in its own process
10+
bounds both: a worker only ever holds N pages of parsed objects, and it returns
11+
that memory to the OS when it exits.
12+
13+
Both pdfplumber and pdfminer.six are pure Python, so this has to use processes —
14+
threads would serialise on the GIL and buy nothing.
15+
"""
16+
import logging
17+
import os
18+
import shutil
19+
import tempfile
20+
from concurrent.futures import ProcessPoolExecutor
21+
from typing import Optional
22+
23+
logger = logging.getLogger(__name__)
24+
25+
# Pages per chunk. Smaller chunks lower peak memory per worker but add
26+
# per-chunk parser startup overhead.
27+
PAGES_PER_CHUNK = int(os.environ.get("PDF_PAGES_PER_CHUNK", "20"))
28+
# Worker processes. Each holds ~140-190 MiB just for the markitdown import, so
29+
# this must be sized against the container memory limit, not just its CPU.
30+
MAX_WORKERS = int(os.environ.get("PDF_MAX_WORKERS", "2"))
31+
# Below this page count the process startup cost outweighs the parallelism.
32+
MIN_PAGES_FOR_PARALLEL = int(os.environ.get("PDF_MIN_PAGES_FOR_PARALLEL", "40"))
33+
34+
35+
def page_count(path: str) -> Optional[int]:
36+
"""
37+
Return the number of pages in ``path``, or None if it is not a readable PDF.
38+
39+
Used both to decide whether chunking is worthwhile and as the PDF sniff, so
40+
a corrupt or non-PDF file transparently falls back to the normal path.
41+
"""
42+
try:
43+
from pypdf import PdfReader
44+
45+
return len(PdfReader(path).pages)
46+
except Exception:
47+
return None
48+
49+
50+
def split_pdf(path: str, pages_per_chunk: int, out_dir: str) -> list[str]:
51+
"""
52+
Split ``path`` into single-file PDFs of ``pages_per_chunk`` pages each.
53+
54+
Chunks are written to ``out_dir`` and passed to workers by path rather than
55+
by value — pickling the bytes through the process pool would duplicate the
56+
document in memory, which is the thing this module exists to avoid.
57+
"""
58+
from pypdf import PdfReader, PdfWriter
59+
60+
if pages_per_chunk < 1:
61+
raise ValueError("pages_per_chunk must be >= 1")
62+
63+
reader = PdfReader(path)
64+
total = len(reader.pages)
65+
chunk_paths: list[str] = []
66+
67+
for index, start in enumerate(range(0, total, pages_per_chunk)):
68+
writer = PdfWriter()
69+
for page in reader.pages[start:start + pages_per_chunk]:
70+
writer.add_page(page)
71+
chunk_path = os.path.join(out_dir, f"chunk_{index:05d}.pdf")
72+
with open(chunk_path, "wb") as fh:
73+
writer.write(fh)
74+
chunk_paths.append(chunk_path)
75+
76+
return chunk_paths
77+
78+
79+
def convert_chunk(chunk_path: str) -> str:
80+
"""
81+
Convert one chunk file to markdown. Runs in a worker process.
82+
83+
Must stay importable at module level so the pool can pickle it. markitdown
84+
is imported lazily here so the parent does not pay for it when the chunked
85+
path is never taken.
86+
"""
87+
from markitdown._stream_info import StreamInfo
88+
from markitdown.converters._pdf_converter import PdfConverter
89+
90+
stream_info = StreamInfo(extension=".pdf", mimetype="application/pdf")
91+
with open(chunk_path, "rb") as fh:
92+
return PdfConverter().convert(fh, stream_info).markdown
93+
94+
95+
def convert_pdf_chunked(
96+
path: str,
97+
pages_per_chunk: int = PAGES_PER_CHUNK,
98+
max_workers: int = MAX_WORKERS,
99+
) -> str:
100+
"""
101+
Convert a PDF by splitting it into page ranges converted in parallel.
102+
103+
Chunk results are concatenated in page order, so output ordering matches a
104+
whole-document conversion.
105+
"""
106+
work_dir = tempfile.mkdtemp(prefix="pdf_chunks_")
107+
try:
108+
chunk_paths = split_pdf(path, pages_per_chunk, work_dir)
109+
logger.info("split %s into %d chunks of <=%d pages",
110+
path, len(chunk_paths), pages_per_chunk)
111+
112+
if len(chunk_paths) == 1:
113+
return convert_chunk(chunk_paths[0])
114+
115+
# "spawn" rather than the Linux default "fork": this runs inside a
116+
# FastAPI threadpool worker, and forking a process that holds threads
117+
# can deadlock if a lock is held at fork time.
118+
import multiprocessing
119+
120+
context = multiprocessing.get_context("spawn")
121+
workers = max(1, min(max_workers, len(chunk_paths)))
122+
with ProcessPoolExecutor(max_workers=workers, mp_context=context) as pool:
123+
# map() preserves input order, so pages stay sequential.
124+
parts = list(pool.map(convert_chunk, chunk_paths))
125+
126+
return "\n\n".join(part for part in parts if part and part.strip())
127+
finally:
128+
shutil.rmtree(work_dir, ignore_errors=True)
129+
130+
131+
def should_chunk(path: str, min_pages: int = MIN_PAGES_FOR_PARALLEL) -> bool:
132+
"""True if ``path`` is a PDF large enough to be worth chunking."""
133+
pages = page_count(path)
134+
return pages is not None and pages >= min_pages

0 commit comments

Comments
 (0)