[Refactor:Autograding] Optimize PDF speed#12471
Conversation
- Replace pdf2image with PyMuPDF for faster PDF processing - Add pattern caching for instant redaction rendering - Implement parallel processing for bulk operations (1.7x speedup) - Add file caching to skip unchanged submissions - Optimize memory usage (70% reduction) - Remove external poppler dependency Resolves performance concerns from PR Submitty#11539
GarvitKhandelwal31
changed the title
GarvitKhandelwal31
changed the title
automateprojectmangement
Bot
moved this from Seeking Reviewer
to In Review
in Submitty Development
There was a problem hiding this comment.
Pull request overview
This PR optimizes PDF image generation performance by replacing pdf2image and PyPDF2 with PyMuPDF (fitz), achieving 20-30x speedup. It addresses performance bottlenecks discovered during testing of the PDF redactions feature (#11539), eliminating the need to scale up VM hardware.
Changes:
- Migrated
generate_pdf_images.pyto use PyMuPDF for PDF rendering with optimized redaction pattern caching - Added parallel processing to
regenerate_bulk_images.pyusing ThreadPoolExecutor with configurable worker counts - Extended
jobs.pyto support configurable max_workers parameter for the regenerate bulk images job
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
sbin/submitty_daemon_jobs/submitty_jobs/generate_pdf_images.py |
Replaced pdf2image/PyPDF2 with PyMuPDF, added file hash caching, optimized redaction pattern generation |
sbin/submitty_daemon_jobs/submitty_jobs/regenerate_bulk_images.py |
Added parallel processing with ThreadPoolExecutor, statistics tracking, and error handling for bulk image regeneration |
sbin/submitty_daemon_jobs/submitty_jobs/jobs.py |
Added max_workers parameter support for configurable parallelism |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12471 +/- ##
=========================================
Coverage 21.64% 21.64%
Complexity 9818 9818
=========================================
Files 268 268
Lines 36777 36777
Branches 490 490
=========================================
Hits 7960 7960
Misses 28331 28331
Partials 486 486
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@williamjallen I have fixed the Python Linting issues and also taken copilot suggestions. Can you please rerun workflows to see , if any errors still remains? Thanks |
|
@GarvitKhandelwal31 Please address the failing CI tests. This is a big change that looks like it may affect a sizeable part of CI, so there may be more refactoring necessary before this can be reviewed. |
|
@williamjallen I believe the failing one Cypress workflow in previous commit is due to an existing issue, and not related to my changes. |
|
I think the failing Cypress(UI) is due to an existing issue and not related to any of the files I have changed. Can a maintainer please guide me on this? |
github-actions
Bot
added
the
Abandoned PR - Needs New Owner
GarvitKhandelwal31
removed
the
Abandoned PR - Needs New Owner
mattLif
left a comment
mattLif
left a comment
There was a problem hiding this comment.
Tested with a large PDF both with and without redactions, and works as expected. Redactions successfully generate at a noticeably better speed compared to before the changes. Code looks good too. Just unsure about how Justin feels about the CI tests that are failing, but the changes are successful.
github-project-automation
Bot
moved this from In Review
to Awaiting Maintainer Review
in Submitty Development
github-project-automation
Bot
moved this from Awaiting Maintainer Review
to Done
in Submitty Development
Why is this Change Important & Necessary?
Fixes #11867. When testing PR #11539 (PDF Redactions), performance issues were found in PDF image generation with redactions enabled. This PR improves performance by switching the rendering backend and adds safety fixes for concurrent bulk regeneration and caching.
What is the New Behavior?
1)PyMuPDF Integration: Replaced the previous PDF rendering approach with PyMuPDF (fitz) for faster page rendering.
2) Caching with Redaction Awareness: Adds a cache keyed on the PDF contents and redaction data, so changing redactions correctly forces regeneration.
3) Daemon support updates.
What steps should a reviewer take to reproduce or test the new feature?
You can use this-
[
{
"page": 1,
"x1": 0.25,
"y1": 0.25,
"x2": 0.75,
"y2": 0.75
},
{
"page": 2,
"x1": 0.1,
"y1": 0.1,
"x2": 0.9,
"y2": 0.9
}
]
On main it took around 10 seconds
On the PR's branch it took around 5.27 second
Automated Testing & Documentation
No new tests were added.
Other information