-
Notifications
You must be signed in to change notification settings - Fork 0
pdf-processor #19
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
pdf-processor #19
Changes from 3 commits
5086552
108a36a
95646fb
5d3b0cd
ba32b9f
e0619c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1 @@ | ||
| .env |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,23 +1,187 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import io | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import pdfplumber | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from langchain_core.documents import Document | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from minio import Minio | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from app.core.config import settings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def pdf_to_document(minio_url: str) -> Document: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Placeholder function - to be implemented later. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger = logging.getLogger(__name__) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _sanitize_cell(cell) -> str: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Safely convert a cell value to string.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if cell is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(cell, (str, int, float, bool)): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return str(cell) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return str(cell) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return repr(cell) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def _extract_tables_safely(page, page_num: int) -> str: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """Extract tables from a page with robust error handling.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| table_text = "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| tables = page.extract_tables() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not isinstance(tables, (list, tuple)): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning(f"Page {page_num}: extract_tables() returned non-iterable type {type(tables)}, skipping tables") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return "" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for table_idx, table in enumerate(tables): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not isinstance(table, (list, tuple)): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning(f"Page {page_num}, Table {table_idx}: table is not iterable, skipping") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for row_idx, row in enumerate(table): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not isinstance(row, (list, tuple)): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning(f"Page {page_num}, Table {table_idx}, Row {row_idx}: row is not iterable, skipping") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| table_text += " | ".join(_sanitize_cell(cell) for cell in row) + "\n" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning(f"Page {page_num}, Table {table_idx}, Row {row_idx}: error processing row: {e}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| This function will: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 1. Download the PDF file from MinIO using the provided URL | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 2. Parse the PDF content | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| 3. Convert it to a LangChain Document object | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning(f"Page {page_num}: error extracting tables: {e}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return table_text | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def get_minio_client() -> Minio: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return Minio( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| endpoint=settings.minio_endpoint, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| access_key=settings.minio_access_key, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| secret_key=settings.minio_secret_key, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| secure=settings.minio_secure, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
coderabbitai[bot] marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def pdf_to_document( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| object_name: str, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bucket_name: str | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| minio_client: Minio | None = None, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) -> list[Document]: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
JuanPalbo marked this conversation as resolved.
JuanPalbo marked this conversation as resolved.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Load a PDF file from MinIO and return a list of Document objects. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Each page becomes a separate Document with metadata. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Args: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| minio_url: URL pointing to the PDF file in MinIO | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| object_name: Path/name of the PDF object in the bucket | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bucket_name: Name of the MinIO bucket (defaults to settings.minio_bucket) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| minio_client: Optional MinIO client (creates one if not provided) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Returns: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Document: LangChain Document object containing the PDF content | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| List of Document objects, one per page | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| """ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+90
to
+106
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if bucket_name is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bucket_name = settings.minio_bucket | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if minio_client is None: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| minio_client = get_minio_client() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Validate object_name | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if not object_name or not object_name.strip(): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValueError("object_name cannot be empty or whitespace") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| documents: list[Document] = [] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Raises: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| NotImplementedError: This function is not yet implemented | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Download the PDF from MinIO into memory | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Note: For very large files, consider streaming to disk instead of loading entirely into memory | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| response = minio_client.get_object(bucket_name, object_name) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.error(f"Failed to get object from MinIO - bucket: '{bucket_name}', object: '{object_name}': {e}") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValueError(f"Failed to retrieve '{object_name}' from bucket '{bucket_name}': {e}") from e | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise ValueError(f"Failed to retrieve '{object_name}' from bucket '{bucket_name}': {e}") from e | |
| raise ValueError(f"Failed to retrieve '{object_name}' from bucket '{bucket_name}'.") from e |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Loading entire PDF into memory could cause issues with large files. For very large PDFs (e.g., hundreds of megabytes), calling response.read() will load the entire file into memory at once, which could lead to memory issues or performance degradation. Consider processing the PDF in a streaming fashion if large files are expected, or add file size limits with appropriate error handling.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The entire PDF is loaded into memory with response.read(), and then a warning is issued if the file is larger than 100MB. Consider checking the file size before loading it into memory to avoid potential out-of-memory errors. You can use the Content-Length header from the response object to determine the file size before reading it.
| except Exception as e: | |
| logger.error(f"Failed to get object from MinIO - bucket: '{bucket_name}', object: '{object_name}': {e}") | |
| raise ValueError(f"Failed to retrieve '{object_name}' from bucket '{bucket_name}': {e}") from e | |
| try: | |
| pdf_bytes = response.read() | |
| # Optional: warn if file is very large (e.g., > 100MB) | |
| # Check Content-Length header before reading into memory | |
| content_length = None | |
| if hasattr(response, "getheader"): | |
| content_length = response.getheader("Content-Length") | |
| elif hasattr(response, "headers") and "Content-Length" in response.headers: | |
| content_length = response.headers["Content-Length"] | |
| if content_length is not None: | |
| try: | |
| file_size_bytes = int(content_length) | |
| file_size_mb = file_size_bytes / (1024 * 1024) | |
| if file_size_mb > 100: | |
| logger.warning(f"Large PDF (Content-Length): {file_size_mb:.1f} MB for '{object_name}' - loading into memory") | |
| except Exception as e: | |
| logger.warning(f"Could not parse Content-Length header for '{object_name}': {e}") | |
| else: | |
| logger.info(f"Content-Length header not found for '{object_name}', proceeding to read file into memory") | |
| except Exception as e: | |
| logger.error(f"Failed to get object from MinIO - bucket: '{bucket_name}', object: '{object_name}': {e}") | |
| raise ValueError(f"Failed to retrieve '{object_name}' from bucket '{bucket_name}': {e}") from e | |
| try: | |
| pdf_bytes = response.read() | |
| # Optional: warn if file is very large (e.g., > 100MB) (fallback if Content-Length was not available) |
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent filename extraction logic that's duplicated. The expression object_name.split("/")[-1] is repeated in both functions at lines 75 and 110. This could be extracted into a helper function or variable to improve maintainability and ensure consistency if the logic needs to change. Also, this approach doesn't handle edge cases like Windows-style paths or objects without a filename component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick | 🔵 Trivial
Filename extraction could be more robust.
Using object_name.split("/")[-1] works for Unix-style paths but could be more explicit and handle edge cases better.
Consider using pathlib for clearer intent:
+from pathlib import PurePosixPath
+
# ... in the metadata dict:
- "filename": object_name.split("/")[-1],
+ "filename": PurePosixPath(object_name).name,This makes the intent clearer and handles edge cases like trailing slashes.
🤖 Prompt for AI Agents
In RAGManager/app/services/pdf_processor.py around line 75, the filename
extraction using object_name.split("/")[-1] is fragile; replace it with a
pathlib-based approach: import pathlib at the top and use
pathlib.PurePosixPath(object_name).name (or pathlib.Path if you prefer OS-native
behavior) to get the filename safely and handle trailing slashes and edge cases,
and ensure the import is added if missing.
Copilot
AI
Dec 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filename extraction using object_name.split("/")[-1] is duplicated in both pdf_to_document and pdf_to_single_document functions. Consider extracting this into a helper function or variable to avoid duplication and make the code more maintainable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this being used?
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,7 +10,7 @@ | |
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def process_pdf_pipeline(minio_url: str) -> int: | ||
| def process_pdf_pipeline(object_name: str) -> int: | ||
| """ | ||
| Orchestrates the PDF processing pipeline. | ||
|
|
||
|
|
@@ -21,20 +21,20 @@ def process_pdf_pipeline(minio_url: str) -> int: | |
| 4. Store in database (to be implemented) | ||
|
|
||
| Args: | ||
| minio_url: URL pointing to the PDF file in MinIO | ||
| object_name: Path/name of the PDF object in the MinIO bucket | ||
|
|
||
| Returns: | ||
| int: document_id of the created document (mock value for now) | ||
|
|
||
| Raises: | ||
| NotImplementedError: If any of the pipeline stages are not yet implemented | ||
| """ | ||
| logger.info(f"Starting PDF processing pipeline for URL: {minio_url}") | ||
| logger.info(f"Starting PDF processing pipeline for object: {object_name}") | ||
|
|
||
| try: | ||
| # Stage 1: PDF to Document | ||
| logger.info("Stage 1: Converting PDF to LangChain Document") | ||
| document = pdf_to_document(minio_url) | ||
| document = pdf_to_document(object_name) | ||
|
||
| logger.info("Stage 1 completed successfully") | ||
|
|
||
| # Stage 2: Document to Chunks | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.