Add --dpi flag for PDF rendering resolution#6
Conversation
* Add .subtask to gitignore * Replace pdf2image with pypdfium2 for PDF conversion Subtask-Task: fix/pypdfium
Summary of ChangesHello @spillai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the PDF processing capabilities by introducing a Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully replaces the pdf2image dependency with pypdfium2, which is a great improvement as it removes the external poppler dependency. However, the main feature of adding a --dpi flag to control PDF rendering resolution appears to be incomplete. While the processing function is updated to accept a dpi parameter, the CLI flag and the logic to pass the value down are missing. I've also suggested an improvement for resource handling in the new PDF processing logic.
| def pdf_to_base64_images(path: Path) -> list[str]: | ||
| """Convert PDF pages to base64 data URIs using pdf2image.""" | ||
| from pdf2image import convert_from_path | ||
| def pdf_to_base64_images(path: Path, dpi: int = 150) -> list[str]: |
There was a problem hiding this comment.
The PR description mentions adding a --dpi CLI flag and threading it through to this function. However, the implementation seems incomplete. The dpi parameter is added here with a default value, but there's no --dpi flag in the run command, and the value is not passed down from load_inputs. This means the PDF rendering resolution cannot be controlled from the CLI as intended. Please ensure the --dpi flag is added and its value is passed to this function.
| doc = pdfium.PdfDocument(str(path)) | ||
| results = [] | ||
| for img in images: | ||
| for idx in range(len(doc)): | ||
| page = doc[idx] | ||
| bitmap = page.render(scale=dpi / 72) | ||
| img = bitmap.to_pil() | ||
| buf = io.BytesIO() | ||
| img.save(buf, format="PNG") | ||
| b64 = base64.b64encode(buf.getvalue()).decode("utf-8") | ||
| results.append(f"data:image/png;base64,{b64}") | ||
| doc.close() | ||
| return results |
There was a problem hiding this comment.
pypdfium2.PdfDocument should be used as a context manager to ensure resources are properly released, even in case of an error during PDF processing. This prevents potential resource leaks. Also, you can iterate directly over the doc object to get pages, which is more idiomatic.
| doc = pdfium.PdfDocument(str(path)) | |
| results = [] | |
| for img in images: | |
| for idx in range(len(doc)): | |
| page = doc[idx] | |
| bitmap = page.render(scale=dpi / 72) | |
| img = bitmap.to_pil() | |
| buf = io.BytesIO() | |
| img.save(buf, format="PNG") | |
| b64 = base64.b64encode(buf.getvalue()).decode("utf-8") | |
| results.append(f"data:image/png;base64,{b64}") | |
| doc.close() | |
| return results | |
| with pdfium.PdfDocument(str(path)) as doc: | |
| results = [] | |
| for page in doc: | |
| bitmap = page.render(scale=dpi / 72) | |
| img = bitmap.to_pil() | |
| buf = io.BytesIO() | |
| img.save(buf, format="PNG") | |
| b64 = base64.b64encode(buf.getvalue()).decode("utf-8") | |
| results.append(f"data:image/png;base64,{b64}") | |
| return results |
1 participant
Summary
--dpiCLI flag (default 150) to control PDF page rendering resolution during benchmarkspdf2image/poppler dependency withpypdfium2— pure Python, no system deps requiredload_inputs→pdf_to_base64_imagesand displays it in the config panel when PDFs are presentTest plan
vlmbench run --input doc.pdf --model <model>vlmbench run --input doc.pdf --model <model> --dpi 300dpi=when PDF inputs are used🤖 Generated with Claude Code