-
Notifications
You must be signed in to change notification settings - Fork 5
fix: ML processing that is causing jobs to fail for medium-large jobs (issue #782) #934
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
Conversation
✅ Deploy Preview for antenna-preview canceled.
|
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.
Pull Request Overview
This PR fixes a performance issue where ML processing jobs were failing or running extremely slowly for medium-large jobs due to duplicate log handlers being added repeatedly. The fix prevents JobLogHandlers from being added multiple times to the same logger.
- Added a check to prevent duplicate JobLogHandler instances from being added to job loggers
- Updated README with Docker setup notes and troubleshooting tips
- Added VS Code debugging configuration for the celeryworker service
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
ami/jobs/models.py | Added duplicate handler check in logger property to prevent performance degradation |
README.md | Enhanced setup documentation with Docker version requirements and troubleshooting notes |
requirements/local.txt | Added commented debugpy dependency for development debugging |
docker-compose.yml | Added commented debugging configuration for celeryworker service |
.vscode/launch.json | Added VS Code remote debugging configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
_logger = logging.getLogger(f"ami.jobs.{self.pk}") | ||
|
||
# Only add JobLogHandler if not already present | ||
if not any(isinstance(h, JobLogHandler) for h in _logger.handlers): |
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.
I see the culprit! what a catch.
I have this branch deployed on staging now, running this job If all goes well I will push to live! docker compose started for me on first try with the new debugger dependency with the following commands:
I haven't tried setting a breakpoint in vscode yet |
Summary
Avoid adding JobLogHandlers multiple times during the
logger
property accessList of Changes
ami/jobs/models.py
celeryworker
Related Issues
Fixes #782
Detailed Description
I was watching the queue worker logs (celery), and I noticed what appeared to be many repeated entries. E.g.:
Furthermore, the number of entries increased by 7 on each iteration:
The problem is that the
.logger
property on theJob
adds aJobLogHandler
when read. Since this property is accessed ~7 times per iteration in the job logic, there are 7 handlers added per iteration. This means that every time the code logs, each of these handlers gets called and this number grows as 7n^2 . Since these JobLogHandler objects write to the DB, there are 7n^2 writes happening. At some point this slows to a crawl and the bigger the job the slower it gets.How to Test the Changes
Running processing jobs with 300+ images.
Screenshots
Successful job processing 369 images in ~6 mins. Prior to the change the job ran for over an hour and was not even half-way done.
Checklist