Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Poll Based Waiting for Job Completion #670
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
Changes from 1 commit
7c633a6
45ef45b
cc12ebb
a6c5799
e3fe1f3
c624565
fb8715a
98b745f
d1eafc8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Check warning on line 49 in smartsim/_core/utils/helpers.py
smartsim/_core/utils/helpers.py#L49
Check warning on line 51 in smartsim/_core/utils/helpers.py
smartsim/_core/utils/helpers.py#L51
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.
why do you choose to pass in TERMINAL_STATUSES from within
wait
when you import it into experiment.py, why not instead inject into_poll_for_statuses
?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.
Just because in theory this could give us a little bit more flexibility
For instance, we might want to add a block to
Experiment.start
want to add a block to make sure that everything started is running a-la:but its all very speculative.
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