-
Notifications
You must be signed in to change notification settings - Fork 249
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
[Core] restart save process #2283
Conversation
pass | ||
|
||
def ExecuteFinalize(self): | ||
pass |
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.
Minor suggestion (maybe for a future PR): When I have had to use restart, it was often because I was running on a cluster with some limit on run-time for the cases. In such situations, I would just estimate how many time steps I could run in a single batch, then launch a job for that and ensure that the model part was serialized at the end of the job. I think it would make sense that the process allows this, which could be achieved by having an extra save on ExecuteFinalize
(maybe only if explicily requested via json?).
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.
Sounds fine to me, we can speak tmr abt this
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.
The new features are OK but the new test fails on my machine
======================================================================
FAIL: test_save_restart_process (test_restart.TestRestart)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/jcotela/Kratos/kratos/tests/test_restart.py", line 265, in test_save_restart_process
self.assertTrue(os.path.isfile(base_file_name + str(i) + ".rest"))
AssertionError: False is not true
----------------------------------------------------------------------
Thanks for the review @jcotela I guess you ran the all the tests with |
@jcotela I fixed the issue, it was as suspected a path problem |
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.
Tests run now (yes, I am using the main test script)
As discussed, to work with #2135
did some minor cleanup in the restart utility, now it works quite nicely :D
FYI @mpentek