Restart policy: resolve restarts #286
Conversation
New statestore method placeholders: - add_task_traceback - resolve_task_restarts The compute api will add a Task Traceback and resolve restarts for returned failed Tasks. When a list of restart patterns are added, restarts are resolved.
* Renamed add_task_traceback to add_protocol_dag_result_ref_traceback * Added tests for add_protocol_dag_result_ref_traceback
Implemented half of the resolve_task_restarts test
With this decorator, if a transaction isn't passed as a keyword arg, one is automatically created (and closed). This allows a chaining behavior where many method calls share a single transaction object.
ianmkenney
force-pushed
the
feature/iss-277-restart-policy_resolve_restarts
branch
from
59a14b5
to
c5084de
Compare
ianmkenney
force-pushed
the
feature/iss-277-restart-policy_resolve_restarts
branch
from
c5084de
to
8a6f980
Compare
|
Reviewing today! Thanks @ianmkenney! |
There was a problem hiding this comment.
Great work @ianmkenney! There are some comments to address, but overall this is looking really slick!
Once you've addressed them, ping me for another review! I think it's likely we can merge after that.
| @@ -173,6 +178,17 @@ def transaction(self, ignore_exceptions=False) -> Transaction: | |||
| else: | |||
| tx.commit() | |||
|
|
|||
| def chainable(func): | |||
| def inner(self, *args, **kwargs): | |||
There was a problem hiding this comment.
Can you add a functools.wraps to this? That will preserve the docstring when using the methods we apply this to (even though this is deep down the stack, and not user-facing).
There was a problem hiding this comment.
I used the update_wrapper function instead rather than use another decorator. I think it does roughly the same thing.
* Removed custom tokenization * Implemented _defaults to allow default tokenization to work
cancel_map has been changed from a defaultdict to a base dict and instead using the dict.get method to return None. Additionally added a set of all task/taskhub pairs that is later used to determine what should be canceled. I've also added grouping on taskhubs so the number of calls to cancel_tasks is minimized.
Also expanded test to check behavior of the task that was meant to be waiting.
We don't want to change `_defaults()` from what's done in the base class unless we have real default values to leave out of the hash.
There was a problem hiding this comment.
@ianmkenney much better, thank you! Good to merge into #280, though the unit tests for Tracebacks will fail without including source_keys and failure_keys in init.
| @@ -137,40 +137,40 @@ def test_from_dict(self): | |||
| assert trp_reconstructed.taskhub_scoped_key == original_taskhub_scoped_key | |||
|
|
|||
|
|
|||
| class TestTraceback(object): | |||
| class TestTracebacks(object): | |||
There was a problem hiding this comment.
These tests don't currently pass, since we don't pass in source_keys or failure_keys in Tracebacks upon init.
ianmkenney
deleted the
feature/iss-277-restart-policy_resolve_restarts
branch
No description provided.