Gracious kill hook #914
Gracious kill hook #914
Conversation
b8c8c37 to
2306fa7
Compare
2306fa7 to
9ebe509
Compare
|
With #911 in, checking on this one: it looks like the code is largely completed. It will need some conflict resolution (which is hopefully easy to accomplish -- probably some are cases of "added two functions in the same location of the old code"). Main question: Is this close enough (and do you, @hejung, have time enough to finish it up) that it is worth holding off on the 1.5 release until this is included? Thematically, it would be nice to include it with the other work on hooks in 1.5, but if it will take more than a week to get done, it might be better to include it in 1.6. |
I cherry-picked the commit that introduces the
I would say it is done. |
Codecov Report
@@ Coverage Diff @@
## master #914 +/- ##
==========================================
+ Coverage 81.47% 81.51% +0.04%
==========================================
Files 140 140
Lines 15362 15399 +37
==========================================
+ Hits 12516 12553 +37
Misses 2846 2846
Continue to review full report at Codecov.
|
dwhswenson
left a comment
There was a problem hiding this comment.
Looks good. Very tiny change to keep the line lengths down -- you may want the link listed in my comment instead of the one in the single-click suggestion -- and some unused as clauses in tests.
As to logging vs. writing to an output stream:
-
The general advantage of logging is that it allows the user to control where/whether/how the output is reported. We limit user options on that in progress reporting because it might mess up our tools for refreshing the output (although users can still silence/redirect output by changing the
output_stream, giving at least some choice). This use case seems reasonable forloggingin serial. -
I think that, in order for Dask to report task logging, you need to set up the logging inside the task, and do so in a way that gets the information back to you (i.e., use a unique file name). (I remember investigating this in detail at one point, but don't remember exactly what I could/couldn't do -- in any case, logs from remote processes are at best a headache to process). However,
GraciousKillprobably won't be running inside a remote Dask process, since each Dask process may be running on a different job from your queuing system (so it's hard forGraciousKillto track time remaining).
In other words, this probably isn't an issue: the GraciousKillHook will be very useful for some workflows/simulation types (e.g., running TPS or other simulations where we can't do multiple shooting moves as once), but it probably won't be used in combination with Dask-based workflows.
|
Done! (I also removed the unnecessary |
|
LGTM. Merging! (And I will start the 1.5.0 release process soon.) |
This builds on #911 and #755. It should only be merged after them.
This adds a
GraciousKillHookthat writes the current simulation state to storage and ends the simulation loop by raising aGraciousKillErrorwhen a given maximum walltime is reached.You can also pass a custom callable as
final_callat hook construction, in case you want to do anything else than closing the storage when the simulation is terminated.Example usage (assuming your steps are not superfast the code below should end the simulation after something sligthly smaller than 3 min 34 s):