Fix job runner stdout missing newline #6081
Conversation
|
Thanks @jennan ! @oliver-sanders (UK team) noted overnight that he ran into this some time ago, but wasn't sure it was a bug so added a note on the need for newline in the handler API documentation: https://github.com/cylc/cylc-flow/blob/master/cylc/flow/job_runner_handlers/documentation.py#L426-L428 However, now that you've tracked down the offending code, by inspection (and also by comparison with similar in Most of the current job-runners don't provide an explicit @jennan - can you tweak this PR to:
|
|
@jennan - no cylc-doc PR is needed for this. It could do with a change log entry though, just in case anyone else is working on job runners at the moment (unlikely but not impossible!). Just add a file |
When the stdout returned by the job runner submit method doesn't end with a newline, the `sys.stdout.write` call in `jobs_submit` do not add it and this cases submission to fails (at least to be recorded as failed by the calling process when running a workflow). Looking at how newlines are handled in the JobRunnerManager.job_kill method, it looks like this a typo in the jobs_submit method.
hjoliver
force-pushed
the
fix_job_runner_newline
branch
from
28cfe66 to
32aed6a
Compare
| @@ -89,7 +89,7 @@ def submit(cls, job_file_path, submit_opts): | |||
| exc.filename = "nohup" | |||
| return (1, None, str(exc)) | |||
| else: | |||
| return (0, "%d\n" % (proc.pid), None) | |||
| return (0, str(proc.pid), None) | |||
There was a problem hiding this comment.
(This newline was compensating for the bug, pre-fix).
| @@ -424,8 +424,7 @@ def submit( | |||
| ret_code: | |||
| Subprocess return code. | |||
| out: | |||
| Subprocess standard output, note this should be newline | |||
| terminated. | |||
| Subprocess standard output. | |||
There was a problem hiding this comment.
(We now add the newline automatically)
|
@oliver-sanders - I've finished this off, and simplified the affected code a bit. Not sure how to go about testing it. |
Have had a go: hjoliver#59 |
….add_tests Fix job runner newline.add tests
|
Merged and pushed your new test @oliver-sanders - this is good to go now. |
|
Great, the other coverage hole was indeed a false negative, Codecov now says 100% of diff hit. |
Kia ora,
While implementing a new job runner, I realised that the if the job runner
submitmethod returns its stdout text without a newline, then a workflow with more than one task will fail. After further investigations with @hjoliver , we discovered that this is due to text handling inJobRunnerManager.jobs_submit, that doesn't properly add a newline to the text message if it is missing. This PR implements a fix.You can reproduce the bug by removing the newline of the second output of the background job runner submit method (see https://github.com/cylc/cylc-flow/blob/master/cylc/flow/job_runner_handlers/background.py#L92) and try to run a workflow with at least 2 tasks.
I am not sure how to turn this into a test to reproduce the bug. Any guidance on this would be welcome 🙂.
[UPDATE (HO)] Short explanation: without this fix, if a job-runner submit method returns job-ID without a newline, it causes the
cylc jobs-submitstdout to be misformatted (two lines concatenated) such that the scheduler can't parse it correctly - for one out of the multiple jobs submitted.Check List
CONTRIBUTING.mdand added my name as a Code Contributor.?.?.xbranch.