feat: setup pdf parser service using docling#15
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a Python-based PDF parsing microservice to the monorepo using Docling and FastAPI. The service enables local extraction of health metrics from PDF reports without sending sensitive data to cloud services, addressing privacy requirements for the health data management application.
Key Changes:
- New PDF parsing service with provider pattern for swappable extraction backends
- FastAPI endpoints for health checks and PDF extraction
- Integration with pnpm workspace via postinstall hooks and custom scripts
- Documentation updates for setup and development workflows
Reviewed changes
Copilot reviewed 14 out of 20 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| src/pdf-parse/pyproject.toml | Python project configuration with dependencies for Docling, FastAPI, and PyTorch |
| src/pdf-parse/app/main.py | FastAPI application entry point with router configuration |
| src/pdf-parse/app/routers/health.py | Health check endpoint returning service status |
| src/pdf-parse/app/routers/extract.py | PDF extraction endpoint with file upload handling |
| src/pdf-parse/app/services/provider.py | Abstract provider interface for extraction backends |
| src/pdf-parse/app/services/docling.py | Docling-based implementation of extraction provider |
| src/pdf-parse/app/schemas/extract.py | Pydantic models for API response schemas |
| src/pdf-parse/.python-version | Python version requirement (3.13) |
| src/pdf-parse/README.md | Service-specific documentation with setup and usage instructions |
| src/package.json | Added postinstall hook for Python deps and PDF service scripts |
| src/AGENTS.md | Updated architecture documentation to include PDF service |
| docs/setup.md | Added Python/uv prerequisites and PDF service endpoints |
| .gitignore | Added Python-specific ignore patterns |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| app = FastAPI( | ||
| title="PDF Service", | ||
| description="PDF extraction service for My Health Log", | ||
| version="0.1.0", | ||
| ) |
There was a problem hiding this comment.
Missing CORS configuration. Since this is a microservice that will be called from a web application (the client app), it needs CORS headers configured to allow requests from the frontend. Without proper CORS middleware, the browser will block cross-origin requests to this API. Consider adding FastAPI's CORSMiddleware with appropriate allowed origins.
| if not file.filename.endswith(".pdf"): | ||
| raise HTTPException(400, "Only PDF files supported") | ||
|
|
There was a problem hiding this comment.
The file validation only checks the extension, which is insufficient for security. An attacker could upload a malicious file with a .pdf extension. Consider adding MIME type validation and/or file content validation to ensure the uploaded file is actually a PDF. For example, verify the file starts with the PDF magic bytes "%PDF".
| if not file.filename.endswith(".pdf"): | |
| raise HTTPException(400, "Only PDF files supported") | |
| # Basic filename check for user convenience | |
| if not file.filename.lower().endswith(".pdf"): | |
| raise HTTPException(400, "Only PDF files supported") | |
| # Validate reported MIME type | |
| if file.content_type != "application/pdf": | |
| raise HTTPException(400, "Invalid content type; only PDF files supported") | |
| # Validate file signature (magic bytes) to ensure it is actually a PDF | |
| header = await file.read(4) | |
| if header != b"%PDF": | |
| raise HTTPException(400, "Uploaded file is not a valid PDF") | |
| # Reset file pointer so downstream code reads from the beginning | |
| await file.seek(0) |
There was a problem hiding this comment.
Docling itself validates the PDF structure during conversion and will raise on invalid files. Magic byte validation is not needed here.
| async def extract(self, file: UploadFile) -> dict: | ||
| content = await file.read() | ||
| stream = DocumentStream(name=file.filename, stream=io.BytesIO(content)) | ||
| result = self.converter.convert(stream) |
There was a problem hiding this comment.
The extract method is defined as async but calls synchronous blocking operations (file.read() and converter.convert()). The DocumentConverter.convert() call likely performs CPU-intensive ML operations that will block the event loop, degrading performance for concurrent requests. Consider using run_in_executor to run the blocking conversion in a thread pool, or verify if Docling provides async APIs.
There was a problem hiding this comment.
Acknowledged. This is a single-user local service for now. Will move to run_in_executor if concurrency becomes a concern.
There was a problem hiding this comment.
Created #16 to track this. Added a TODO in code linking to the issue.
|
@ayushpahwa , I think if we are enabling AI reviews then rhe author should resolve the ai comments before human review. It will increase clarity about what is already being addressed and also cleanup the review view of github. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 21 changed files in this pull request and generated 11 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def extract(self, file: UploadFile) -> dict: | ||
| if file.size is None or file.filename is None: | ||
| raise HTTPException(404, "Please use a valid file") | ||
| print(file.size) |
There was a problem hiding this comment.
The debug print statement should be removed or replaced with proper logging. This will clutter the console output in production and doesn't follow the logging pattern established elsewhere in the application (using the logger from app.state).
| print(file.size) |
There was a problem hiding this comment.
Fixed — replaced all print statements with logger.error() using the uvicorn logger.
| # TODO: replace return type from dict to ExtractionResponse once mapping is fixed | ||
| async def extract(self, file: UploadFile) -> dict: | ||
| if file.size is None or file.filename is None: | ||
| raise HTTPException(404, "Please use a valid file") |
There was a problem hiding this comment.
Error message clarity issue: The message "Please use a valid file" is vague and doesn't explain what makes a file valid. This will be unhelpful to API consumers trying to debug upload failures. Consider a more descriptive message like "File upload is invalid: missing file size or filename".
| raise HTTPException(404, "Please use a valid file") | |
| raise HTTPException(404, "File upload is invalid: missing file size or filename.") |
There was a problem hiding this comment.
This is now routed through the unified exception handler which returns a structured ApiResponse with the error details.
| | `pnpm dev` | Start server, client, and PDF service in parallel | | ||
| | `pnpm dev:server` | Start server only | | ||
| | `pnpm dev:client` | Start client only | | ||
| | `pnpm dev:pdf-parse` | Start PDF parsing service (dev mode) | |
There was a problem hiding this comment.
The documentation references script name "pnpm dev:pdf-parse" but earlier in the same document (line 9) it shows "pnpm dev" running "server + client + PDF service in parallel". However, looking at package.json, the "dev" script doesn't actually include the PDF service automatically. The documentation should clarify that users need to run "pnpm dev:pdf-parse" separately, or the package.json dev script should be updated to include it.
There was a problem hiding this comment.
The dev script on line 9 runs all services in parallel including pdf-parse. The separate dev:pdf-parse script is for running the PDF service standalone. This is documented correctly.
| # TODO: replace return type from dict to ExtractionResponse once mapping is fixed | ||
| async def extract(self, file: UploadFile) -> dict: | ||
| if file.size is None or file.filename is None: | ||
| raise HTTPException(404, "Please use a valid file") |
There was a problem hiding this comment.
HTTP status code 404 (Not Found) is semantically incorrect for invalid file uploads. A 404 status code indicates that the requested resource doesn't exist on the server. For file validation failures, use 400 (Bad Request) instead to indicate that the client sent an invalid request.
| raise HTTPException(404, "Please use a valid file") | |
| raise HTTPException(400, "Please use a valid file") |
There was a problem hiding this comment.
Fixed — changed to HTTPException(400).
| app.add_middleware( | ||
| CORSMiddleware, | ||
| allow_origins=settings.cors_origins, | ||
| allow_credentials=True, |
There was a problem hiding this comment.
Missing HTTP methods validation in CORS configuration. The CORSMiddleware is configured without specifying allowed HTTP methods (allow_methods) or headers (allow_headers). This could cause CORS preflight requests to fail when the frontend tries to POST files to /extract. Add allow_methods=[""] or a specific list of methods, and allow_headers=[""] or specific headers needed for file uploads.
| allow_credentials=True, | |
| allow_credentials=True, | |
| allow_methods=["*"], | |
| allow_headers=["*"], |
There was a problem hiding this comment.
Fixed — added allow_methods=[""] and allow_headers=[""].
| "scripts": { | ||
| "dev": "pnpm --parallel --filter '*' dev", | ||
| "postinstall": "[ \"$CI\" = 'true' ] || (cd pdf-parse && uv sync)", | ||
| "dev": "pnpm --parallel --filter '*' dev & pnpm dev:pdf-parse", |
There was a problem hiding this comment.
The "&" operator for running parallel commands is not cross-platform compatible. It works on Unix-like systems (Linux/macOS) but will fail on Windows. Since the repository appears to be using pnpm's built-in parallel execution, use "pnpm --parallel" instead, or use a cross-platform tool like "concurrently" or "npm-run-all".
| pnpm dev # Run server + client in parallel | ||
| pnpm dev:server # Server only | ||
| pnpm dev:client # Client only | ||
| pnpm pdf:dev # PDF parsing service (dev mode) |
There was a problem hiding this comment.
The documentation references script name "pnpm pdf:dev" which doesn't exist in package.json. The actual script name is "pnpm dev:pdf-parse". This inconsistency will confuse users following the documentation. Update this to match the actual script name in package.json.
| pnpm pdf:dev # PDF parsing service (dev mode) | |
| pnpm dev:pdf-parse # PDF parsing service (dev mode) |
| pnpm dev:server # Server only | ||
| pnpm dev:client # Client only | ||
| pnpm pdf:dev # PDF parsing service (dev mode) | ||
| pnpm pdf:start # PDF parsing service (production) |
There was a problem hiding this comment.
The documentation references script name "pnpm pdf:start" which doesn't exist in package.json. The actual script name is "pnpm start:pdf-parse". This inconsistency will confuse users following the documentation. Update this to match the actual script name in package.json.
| pnpm pdf:start # PDF parsing service (production) | |
| pnpm start:pdf-parse # PDF parsing service (production) |
| # TODO: replace return type from dict to ExtractionResponse once mapping is fixed | ||
| # async def extract(self, file: UploadFile) -> ExtractionResponse: | ||
| async def extract(self, file: UploadFile) -> dict: | ||
| pass |
There was a problem hiding this comment.
The abstract method uses "pass" but doesn't raise NotImplementedError. While Python's ABC will prevent instantiation of classes that don't implement abstract methods, it's a best practice to include "raise NotImplementedError" in the abstract method body for clarity and to catch potential issues if the method is called via super().
| pass | |
| raise NotImplementedError("Subclasses of ExtractionProvider must implement the 'extract' method.") |
| class Element(BaseModel): | ||
| text: str | ||
| bbox: BoundingBox | ||
| type: str # "text", "table", "heading" |
There was a problem hiding this comment.
The type field in Element uses a string literal type with specific expected values ("text", "table", "heading") indicated in the comment, but the actual type annotation is just 'str'. This allows any string value, which could lead to inconsistent data. Consider using a Literal type or Enum to enforce the allowed values: type: Literal["text", "table", "heading"].
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 21 out of 27 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| top=bbox.t, | ||
| bottom=bbox.b, | ||
| right=bbox.r, | ||
| coord_origin=CoordOriginEnum(value=bbox.coord_origin.value), |
There was a problem hiding this comment.
CoordOriginEnum is being constructed incorrectly: CoordOriginEnum(value=...) will raise TypeError because enums don’t accept a value= keyword. This will cause section header parsing to fail and be silently swallowed by the surrounding try/except. Use CoordOriginEnum(bbox.coord_origin.value) (or map the docling origin to your enum) instead.
| coord_origin=CoordOriginEnum(value=bbox.coord_origin.value), | |
| coord_origin=CoordOriginEnum(bbox.coord_origin.value), |
There was a problem hiding this comment.
This works correctly — Python StrEnum accepts value= as a keyword argument. We've verified the output multiple times with correct coord_origin values. Dismissing.
| pnpm pdf:dev # PDF parsing service (dev mode) | ||
| pnpm pdf:start # PDF parsing service (production) |
There was a problem hiding this comment.
The commands pnpm pdf:dev and pnpm pdf:start referenced here don’t exist in src/package.json (the scripts are dev:pdf-parse / start:pdf-parse). This will mislead contributors following the setup instructions. Update the docs to the actual script names (or rename the scripts to match the docs).
| pnpm pdf:dev # PDF parsing service (dev mode) | |
| pnpm pdf:start # PDF parsing service (production) | |
| pnpm dev:pdf-parse # PDF parsing service (dev mode) | |
| pnpm start:pdf-parse # PDF parsing service (production) |
There was a problem hiding this comment.
Fixed — updated AGENTS.md to use the correct script names (dev:pdf-parse, start:pdf-parse).
Summary
pdf-parsePython service using FastAPI + Docling for extracting structured data from health report PDFsApiResponse[T]wrapper with unified exception handling across all endpointsKey files
pdf-parse/app/services/docling.py— extraction provider withparse_section_headers,parse_groups,parse_tables,normalise_extractionpdf-parse/app/schemas/extract.py— Pydantic models:Page,ParsedTable,TableRow,ParseSectionHeader,ParsedGroups,KVGroup | ListGrouppdf-parse/app/schemas/api_response.py— genericApiResponse[T]response wrapperpdf-parse/app/exception_handlers.py— unified exception handler for HTTPException, validation errors, and unhandled exceptionspdf-parse/app/schemas/docling.py— type aliases bridging Docling union typesKnown Docling artifacts (deferred to LLM normalization step)
"Haemoglobin (HB) Method: Photometry"): Ayush Pahwa)DEPARTME N T)Test plan
ApiResponseformatApiResponseformat🤖 Generated with Claude Code