[MAINTENANCE] Standardize result object from Checkpoint action runs #10647
Conversation
✅ Deploy Preview for niobium-lead-7998 canceled.
|
for more information, see https://pre-commit.ci
…tations/great_expectations into m/_/better_reporting
cdkini
changed the title
cdkini
changed the title
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found.
Additional details and impacted files@@ Coverage Diff @@
## develop #10647 +/- ##
========================================
Coverage 80.37% 80.38%
========================================
Files 463 463
Lines 40113 40130 +17
========================================
+ Hits 32241 32258 +17
Misses 7872 7872
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…tations/great_expectations into m/_/better_reporting
…_expectations into m/_/better_reporting
| run_info: A dictionary containing information about the run. | ||
| """ | ||
|
|
||
| status: ValidationActionRunStatus |
There was a problem hiding this comment.
Should we include the action name (eg the class name) in this result object or will it be obvious since this is embedded inside something? Eg if I see not_run will I know that is associated with a pager duty action?
There was a problem hiding this comment.
It seems like the run_info dict has a key which is a string like <action_type>_result. If we put the action type in it's own field, it would make that info easier to parse out.
There was a problem hiding this comment.
It'll be obvious since this is only ever used in Checkpoint.run - my thinking is we can take the aggregated results (of these actions) and either log or append to the checkpoint result object.
Don't want to make too many changes here until we can plan better.
| """ | ||
|
|
||
| status: ValidationActionRunStatus | ||
| run_info: dict |
There was a problem hiding this comment.
Does the run_info vary from action to action or can we be more concrete about this?
There was a problem hiding this comment.
Ah yeah I don't love our current dicts but I'm trying to minimize changes. They're all pretty much {"action_type_in_snake_case": "success_or_failure"} so they could be simplified with a simple bool but I think I want to keep it this way for a few reasons:
- Minimize changes (for now)
- Allow for more complex
run_info(UpdateDataDocsleverages this and future custom actions might want to include more details?)
| return {"pagerduty_alert_result": "none sent"} | ||
| return ValidationActionResult( | ||
| status=ValidationActionRunStatus.NOT_RUN, | ||
| run_info={"pagerduty_alert_result": "none sent"}, |
There was a problem hiding this comment.
For ms teams, the equivalent value is None instead of "none sent". Can these be the same thing?
There was a problem hiding this comment.
Same note as above - don't love these dicts but doing what I can to minimize changes. If we can determine a consistent standard, I'd be happy to make changes across the board.
| @@ -427,6 +432,9 @@ def test_run_integration_success( | |||
| self, | |||
| checkpoint_result: CheckpointResult, | |||
| ): | |||
| if not os.environ.get("GX_MS_TEAMS_WEBHOOK"): | |||
There was a problem hiding this comment.
I don't like programmatically skipping tests. What has happened in the past is someone makes a change and we start skipping this in CI without realizing it. I'd rather just fail and require this env variable or have this moved to a different marker where we require this.
|
|
||
| action_results = self._run_actions(checkpoint_result=checkpoint_result) | ||
| # TODO(cdkini): Figure out how to handle action results | ||
| print(action_results) # We could add these to our checkpoint result or logger.warning? |
There was a problem hiding this comment.
This is where I'm unsure but the goal with this whole effort is to make action results more transparent - we currently silently fail or skip
There was a problem hiding this comment.
If we log, there's still a chance can lose track of it.
I'd lean towards updating the checkpoint result but we need to be careful about what we add there - don't want it becoming as cumbersome as its V0 counterpart.
We should have a consistent API for
ValidationAction.run- this new model is a lightweight wrapper around a status and the existing payload but should allow us to be better prepared for extensibility that users want.invoke lint(usesruff format+ruff check)For more information about contributing, visit our community resources.
After you submit your PR, keep the page open and monitor the statuses of the various checks made by our continuous integration process at the bottom of the page. Please fix any issues that come up and reach out on Slack if you need help. Thanks for contributing!