-
Notifications
You must be signed in to change notification settings - Fork 9
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 Protocol Errors
#441
Add Protocol Errors
#441
Conversation
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #441 +/- ##
=======================================
Coverage 98.51% 98.52%
=======================================
Files 38 39 +1
Lines 2158 2166 +8
=======================================
+ Hits 2126 2134 +8
Misses 32 32 ☔ View full report in Codecov by Sentry. |
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.
Couple of comments, otherwise it looks good to me!
gufe/protocols/errors.py
Outdated
|
||
|
||
# Protocol Results Errors | ||
class MissingProtocolUnitError(Exception): |
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.
Do you mean a missing UnitResult?
gufe/protocols/errors.py
Outdated
|
||
|
||
# Protocol Errors | ||
class ProtocolValidationError(Exception): |
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.
Should we be subclassing out of a "BaseGufeException" type? I'm thinking it might be useful to have a catch-all that isn't a Exception
in some cases.
For example - it might be good for execute_DAG to capture all of these and prevent trying to restart the DAG (i.e. if these are known deterministic failures, then we should assume they will happen over and over again).
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
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.
This is a great start @jthorton! Thank you for getting us going on more specific error modes for protocols!
@ijpulidos since this PR addresses #385, would you be willing to offer your feedback on this one? |
@hannahbaumann since you're working in detail on protocols, would you be willing to offer feedback on our choices here? |
pre-commit.ci autofix |
1 similar comment
pre-commit.ci autofix |
for more information, see https://pre-commit.ci
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.
Looks great @jthorton! Thanks for putting this together!
Fixes #385 by adding specific protocol execution and results errors.
Tips
Since this will create a commit, it is best to make this comment when you are finished with your work.
Checklist
news
entryDevelopers certificate of origin