record-aip-id-for-lambda-response-exceptions #159
Conversation
Why these changes are being introduced: * After the last update to bulk-validate, it was discovered that if an exception occurred while parsing the lambda response, the uuid or S3 URI would not be recorded. How this addresses that need: * Add aip_uuid and aip_s3_uri to dataframe the Except block of validate_aip_bulk_worker to ensure they are recorded in case of exceptions while parsing the lambda response * Add corresponding CLI tests to show that the uuid and S3 URI are added to the results CSV in case of a lambda response exception * Update dependencies Side effects of this change: * None Relevant ticket(s): * NA
There was a problem hiding this comment.
Looks good! I can see how this will still include the AIP UUID / S3 URI in the bulk response, even if the lambda goes sideways and has an error.
I do get the sense there is a bit of friction between three parts in play, given the last few bugs and PRs have been around this area:
- what data the actual, deployed Lambda includes in the response (or cannot include, if it fails to return a response)
- the function
validate_aip_via_lambda(), that calls the lambda... - the "bulk worker" function
validate_aip_bulk_worker(), that callsvalidate_aip_via_lambda(), that calls the lambda...
Given that things are mostly functional, and edge cases exposed, it might be worth a ticket to see if those three things can be simplified or streamlined. Or, perhaps it's just a little finicky like this and these additions are the way to incrementally improve it!
All said, this change makes sense to me. Nice work!
| results_df.loc[row_index, "aip_uuid"] = aip_uuid | ||
| results_df.loc[row_index, "aip_s3_uri"] = s3_uri |
There was a problem hiding this comment.
This makes sense to me.
If the function validate_aip_via_lambda got updated to include the AIP uuid + S3 URI in the response from this past PR, it makes sense to capture that output in this bulk handler to write to the output.
Purpose and background context
If exceptions occur while parsing the lambda response, neither the
aip_uuidoraip_s3_uriwould be recorded in the output CSV. This can be caused large bags that raiseJSONDecodeErrorexceptions when the lambda fails to validate the bag.How can a reviewer manually see the effects of these changes?
A input file will be shared on Slack that exposes the issue. After setting the challenge secret for
Prod, run the following command with themainbranch:In the output CSV, see that the
aip_uuidcolumn is not set for the AIPs that experienced errors.Fetch
lambda-response-exceptionand run the following command:In the output CSV, see that the
aip_uuidcolumn is set for the AIPs that experienced errors.Includes new or updated dependencies?
YES
Changes expectations for external applications?
NO
Developer
Code Reviewer(s)