write status.csv as soon as study graph is constructed and between sy…#477
Conversation
…nchronous login node jobs check for cancellation between synchronous login node jobs Signed-off-by: Alan Thomas Grier <grier1@llnl.gov>
| # updates, especially for login node jobs that complete | ||
| # synchronously. | ||
| if self._status_path: | ||
| self.write_status() |
There was a problem hiding this comment.
So I think this (and the cancellation) machinery should remain out at the conductor level, not in the job submission hot-loop inside the execution graph. Bench-marking the submission speed fix that's going into this release has the new floor at ~20-30 job submissions per second, and hitting the file system for this and cancellation that often is likely to cause problems (might even make the status cmd fail outright while jobs are being submitted due to io contention).
I've got an alternate local adapter + executor in the works to make it behave more like flux/slurm/etc schedulers (i.e. non-blocking) that should be a better path making things more responsive.
There was a problem hiding this comment.
I reverted the bulk of the changes, including the test file that was mostly for the cancellation changes. Now it's limited to a single addition of a status write at conclusion of initializing the conductor, along with a couple minor clerical edits.
There was a problem hiding this comment.
great, thanks for making those changes! Looks almost ready to go, just a few more general things things:
- throw in the changelog fragment; see here for details on that: https://github.com/llnl/maestrowf/blob/develop/CONTRIBUTING.md#changelog-fragments
- bump the dev version in the pyproject.toml
and see the other comment in conductor for one bug to fix
There was a problem hiding this comment.
I pushed up a new commit with these changes.
| self._study.store_metadata() | ||
| self._setup = True | ||
| # Write initial status.csv immediately after graph construction | ||
| self._exec_dag.write_status() |
There was a problem hiding this comment.
just need the path argument added here and think we'll be good to go
There was a problem hiding this comment.
In the other use of write_status() you have dag.write_status(os.path.split(pkl_path)[0]), which substituting pkl_path is dag.write_status(os.path.split(os.path.join(self._pkl_path, "{}.pkl".format(self._study.name)))[0]). I believe this should be equivalent to just self._pkl_path, provided there are no slashes in the self._study.name. Is that always true?
There was a problem hiding this comment.
should be.. but it's probably clearly to use output_path as the base instead of pkl_path like conductor's 'get_status' does
add changelog fragment bump dev version
This PR addresses #476
Currently the status.csv is not written (such as for synchronous login node jobs) until all of the first job step jobs are completed, nor can cancellation be issued or checked until that point. Proposed changes: