-
Notifications
You must be signed in to change notification settings - Fork 36
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
Poll Based Waiting for Job Completion #670
Poll Based Waiting for Job Completion #670
Conversation
b46345d
to
ae86890
Compare
`Experiment` was given a `wait` method that takes a collection of Launched Job IDs and will wait until the launch reaches a terminal state by either completing or erroring out. Implements a polling based solution.
ae86890
to
7c633a6
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## smartsim-refactor #670 +/- ##
=====================================================
+ Coverage 40.45% 43.28% +2.82%
=====================================================
Files 110 110
Lines 7326 7053 -273
=====================================================
+ Hits 2964 3053 +89
+ Misses 4362 4000 -362
|
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.
By the mystical powers vested in me, I declare this pull request to be a work of sheer brilliance!
:param timeout: The minimum amount of time to spend polling all jobs to | ||
reach one of the supplied statuses. If not supplied or `None`, the | ||
experiment will poll indefinitely. | ||
:param interval: The minimum time between polling launchers. |
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.
Is param interval just for us to use for testing? Seems like user cannot define
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.
Exactly! It was something that, right now, could be hard coded, but if in the future we wanted to make it variable we can change the parameter. Totally willing to remove if we think the excess complexity is unnecessary in a YAGNI way!
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 wouldn't mind leaving it in and keeping it in the docs
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 say keep if but not in docstring
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.
looooooks great, lmk if there is anything you want me to take a closer look out and I can test out some stuff :)
:param timeout: The minimum amount of time to spend polling all jobs to | ||
reach one of the supplied statuses. If not supplied or `None`, the | ||
experiment will poll indefinitely. | ||
:param interval: The minimum time between polling launchers. |
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 wouldn't mind leaving it in and keeping it in the docs
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.
Only one small question/comment otherwise LGTM!
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.
LGTM! Thanks for adding all the doc strings to the tests!
Experiment
was given await
method that takes a collection of Launched Job IDs and will wait until the launch reaches a terminal state by either completing or erroring out. Implements a polling based solution.