-
Notifications
You must be signed in to change notification settings - Fork 805
Add defensive programming checks for progress setting and calculation #13328
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
Add defensive programming checks for progress setting and calculation #13328
Conversation
Build Artifacts
|
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.
Code changes lgtm overall. Just a question about maybe raising an error when the values given to update_progress are bad.
Will leave approval to QA
logger.warning( | ||
f"Tried to set invalid progress values on job {self.job_id} for task {self.func} with progress: {progress} and total_progress {total_progress}" | ||
) |
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.
Would it be helpful here to raise a ValueError here? I can see myself missing the warning in the logs if I were coming at this while debugging in progress updating on the front-end or something. Not a blocking comment - just a thought.
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.
My reason for only logging is that given we have seen this in the wild (as per the issue) I don't want to cause existing jobs to fail just because of a progress tracking error.
Variety of import/update/delete scenarios where performed with few resources and complete channels on both Ubuntu and Windows, no issues or regressions observed in the UI, task manager page or the console. |
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.
Manual QA passes, ready to go! 💯
Summary
References
Fixes #13321
Reviewer guidance
Do a quick smoke test of content import for a new channel to ensure against regressions in task calculation