Arvados

20825-cwl-separate-runner @ f165ffe5ba28815b965fa4f92ceed99fc53e0776

developer-run-tests: #3826

20825-cwl-separate-runner @ df566fda61e84d8d99ac133ecc3dbc8726dff40f

developer-run-tests: #3854

• All agreed upon points are implemented / addressed.
  1. launches subworkflows in separate runners -- done
  2. set custom names for the sub-runners -- done
  3. can reuse whole workflows -- done
• Anything not implemented (discovered or discussed during work) has a follow-up story.
  • the "nice to have" error handling was not implemented, added as #21050
  • also related to streamlining launching large scatters is #21051
• Code is tested and passing, both automated and manual, what manual testing was done is described
  • added an integration test that launches a subworkflow with this feature. the test verifies it doesn't crash, but currently doesn't include a check that the subworkflow was launched with a separate runner.
  • was used to launch a test job of 500 samples, it worked very well
• Documentation has been updated.
  • TBD
• Behaves appropriately at the intended scale (describe intended scale).
  • This significantly improves scaling. Previously, launching a batch of (say) 500 samples would require a separate invocation of arvados-cwl-runner each time, taking 6-10 seconds per sample. With this new feature, all of the overhead of starting arvados-cwl-runner is eliminated, so launching each workflow takes about 0.2s per sample. This is a 30-50x improvement.
• Considered backwards and forwards compatibility issues between client and server.
  • The subworkflow runners should behave exactly as if they had been launched at the command line, respecting runner level hints like ProcessProperties and WorkflowRunnerResources
• Follows our coding standards and GUI style guidelines.
  • yes

The motivation for this branch is to be able to launch large number of per-sample subworkflows as quickly as possible, by eliminating the overhead of re-parsing the workflow each time. In a test, this allowed me to launch 500 samples in roughly 2 minutes, where previously this could take upwards of an hour.

Overall I think it LGTM, I just have one nitpicky comment.

In commit 927e6781a1bb21e5ef1d887b89916685601b8fd4 in arvcontainer.py, you raise a generic Exception.

I learned it was bad to raise a generic exception, here's a quote from the python docs (https://docs.python.org/3/tutorial/errors.html)

"Exception can be used as a wildcard that catches (almost) everything. However, it is good practice to be as specific as possible with the types of exceptions that we intend to handle, and to allow any unexpected exceptions to propagate on."

I looked in our coding standards and didn't see anything, and was just wondering if we had a standard around that. If we are fine with that, I think it LGTM.

Alex Coleman wrote in #note-15:

Overall I think it LGTM, I just have one nitpicky comment.

In commit 927e6781a1bb21e5ef1d887b89916685601b8fd4 in arvcontainer.py, you raise a generic Exception.

I learned it was bad to raise a generic exception, here's a quote from the python docs (https://docs.python.org/3/tutorial/errors.html)

"Exception can be used as a wildcard that catches (almost) everything. However, it is good practice to be as specific as possible with the types of exceptions that we intend to handle, and to allow any unexpected exceptions to propagate on."

I looked in our coding standards and didn't see anything, and was just wondering if we had a standard around that. If we are fine with that, I think it LGTM.

You're right, there's no reason that couldn't be a WorkflowException which is used generically for workflow logic errors but is an improvement over the Python base Exception. Done and merged. Thanks!

Peter Amstutz wrote in #note-11:

• Documentation has been updated.
  • TBD

What is even the point of having a branch review checklist if you can just fob off any point as "TBD"?