-
-
Notifications
You must be signed in to change notification settings - Fork 572
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 time to experiment termination conditions #4073
Add time to experiment termination conditions #4073
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4073 +/- ##
===========================================
- Coverage 99.55% 99.54% -0.01%
===========================================
Files 287 287
Lines 21753 21775 +22
===========================================
+ Hits 21657 21677 +20
- Misses 96 98 +2 ☔ 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.
Looks good to me. @brosaplanella can you confirm this isn't duplicating the functionality of the start_time
that you added?
No, that should be a different feature. |
For Lychee the whole projects page of LLNL is down, but the main page is still OK. Probably just a random outage |
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.
Changes look fine to me, just the time adjustment looks reusable
elif term.endswith("s"): | ||
end_time_s = term.split("s")[0] | ||
termination_dict["time"] = (float(end_time_s), "s") | ||
elif term.endswith("min"): | ||
end_time_s = term.split("min")[0] | ||
termination_dict["time"] = (float(end_time_s) * 60, "s") | ||
elif term.endswith("h"): | ||
end_time_s = term.split("h")[0] | ||
termination_dict["time"] = (float(end_time_s) * 3600, "s") |
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 assume we do this elsewhere too. Maybe we should have. function for handling time units. I don't want to block this PR, but can we open a ticket for it?
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.
Thanks for creating that issue. It sounds like there should be another change applied to use the function that Valentin mentioned in that issue. I can take that issue if nobody else wants to take it. Maybe next week I can take a stab at applying the fix.
Failure is gone now |
* add time termination lines * fix bug * add test * add more test coverage * update changelog --------- Co-authored-by: Agriya Khetarpal <[email protected]> Co-authored-by: Eric G. Kratz <[email protected]>
Description
This change is to add the ability to stop an experiment by setting a time delta as a termination condition. There is no current issue for this that I am aware of, but I can make one if needed. I would like to be able to do this to simulate an experiment only up until a given time.
Fixes # (issue)
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.
Key checklist:
$ pre-commit run
(or$ nox -s pre-commit
) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)$ python run-tests.py --all
(or$ nox -s tests
)$ python run-tests.py --doctest
(or$ nox -s doctests
)You can run integration tests, unit tests, and doctests together at once, using
$ python run-tests.py --quick
(or$ nox -s quick
).Further checks: