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
[FEATURE] Checkpoint.save()
#9676
Conversation
✅ Deploy Preview for niobium-lead-7998 canceled.
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #9676 +/- ##
========================================
Coverage 82.55% 82.55%
========================================
Files 511 511
Lines 46381 46395 +14
========================================
+ Hits 38288 38301 +13
- Misses 8093 8094 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
try: | ||
store.update(key=key, value=self) | ||
except gx_exceptions.StoreBackendError as e: | ||
raise ValueError(f"Could not find existing Checkpoint '{self.name}' to update") from e |
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 think this error is too specific - for instance, if the cloud handler returns a 400 due to a field of an unexpected type.
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.
Hmm what error do we want to catch then? The other domain objects don't do error handling here but I don't want to raise a store backend error to the user (feels like that is more internal?)
@@ -21,9 +18,8 @@ | |||
|
|||
# TODO: Add analytics as needed | |||
class CheckpointFactory(Factory[Checkpoint]): | |||
def __init__(self, store: CheckpointStore, context: AbstractDataContext): |
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.
🥳
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.
left one comment around error handling
except gx_exceptions.StoreBackendError as e: | ||
name = key.to_tuple()[0] | ||
raise ValueError(f"Could not find existing Checkpoint '{name}' to update") from e |
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.
isn't this the same issue as before? There are other valid errors -- such as validation -- Cloud backend can throw, so this error message is too specific.
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.
blocking because i believe error handling is still misleading
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.
thanks for addressing changes!
invoke lint
(usesruff format
+ruff check
)For more information about contributing, see Contribute.
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!