Fix bug and Improve error handling in Figures tasks and pipeline #312
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The pipeline function, bulk_calculate_course_progress_data, does a lot of work and a potential point of failure to fail all the metrics for the course for the day on collecting just one metric. So this commit traps for exceptions when the function "bulk_calculate_course_progress_data" is called and sets the course daily metrics 'average_progress' to None if it fails. Being 'None' helps identify pipeline erors rather than setting progress to zero. Added test to make sure we are logging when it fails
Added exception handling and logging to populate_single_cdm, the call to update_enrollment_data in 'populate_daily_metrics' and exception handling for each site so that one site does not fail the remaining sites in the chain * Updated tests to handle when update_enrollment_data raises an exception * Updated tests to handle when a site level exception is raised
This function is running into 'CourseNotFound' exceptions. Because of a poorly place paranthesis in the logger message, the function then threw a "TypeError: str() takes at most 1 argument (2 given)" which THEN caused the whole daily metrics pipeline to fail because I forgot to wrap the new code in exception handling
johnbaldwin
changed the title
thraxil
approved these changes
Dec 17, 2020
|
Thanks @thraxil ! |
OmarIthawi
reviewed
Dec 17, 2020
figures/backfill.py
Outdated
Comment on lines
70
to
71
| ' CourseEnrollment ID='.format(str(rec.course_id), | ||
| rec.id)) |
There was a problem hiding this comment.
Must: An additional {} is needed. I've added a suggestion to fix the issue.
Suggested change
| ' CourseEnrollment ID='.format(str(rec.course_id), | |
| rec.id)) | |
| errors.append( | |
| 'CourseNotFound for course "{course}". CourseEnrollment ID={rec}'.format( | |
| course=str(rec.course_id), | |
| rec=rec.id, | |
| ) | |
| ) |
figures/backfill.py
Outdated
| @@ -67,7 +67,7 @@ def backfill_enrollment_data_for_site(site): | |||
| enrollment_data.append((obj, created)) | |||
| except CourseNotFound: | |||
| errors.append('CourseNotFound for course "{}". ' | |||
There was a problem hiding this comment.
See the related suggestion on the line below.
Suggested change
| errors.append('CourseNotFound for course "{}". ' |
There was a problem hiding this comment.
@OmarIthawi Thanks, I've got a code cleanup effort to do. When I hit the pipeline logging, adding format vars and making sure all exceptions are tested will be part of that
There was a problem hiding this comment.
@OmarIthawi I saw I had a missing '{}' in the string, so adding vars to the format . Thanks!
|
Tox passed locally, force merging. Maybe I regret it. We'll see |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixed the bug, poorly placed parentheses within exception handling in the new code
Add pipeline exception handling - course daily metrics
The pipeline function, bulk_calculate_course_progress_data, does a lot
of work and a potential point of failure to fail all the metrics for the
course for the day on collecting just one metric.
So this commit traps for exceptions when the function
"bulk_calculate_course_progress_data" is called and sets the course daily
metrics 'average_progress' to None if it fails. Being 'None' helps
identify pipeline erors rather than setting progress to zero.
Added test to make sure we are logging when it fails
Added exception handling and logging to populate_single_cdm, the call to
update_enrollment_data in 'populate_daily_metrics' and exception
handling for each site so that one site does not fail the remaining
sites in the chain
https://appsembler.atlassian.net/browse/RED-1709