Arvados

Review comments:

• crunch_scripts/cwl-runner:
  • This print statement “print cwltool.main.versionstring()” : not sure it’s something you forgot to remove. If not, can it be converted into a log statement with some description?
  • Can you please add a detailed description / comment about this script at the top of the file? Most importantly describing how the relationship between this and sdk/cwl/arvados-cwl/__init__.py works would be good to have here.
  • In “np = argparse.Namespace()” : I could not tell why it is named as “np”. May be args (if it does not conflict with other things or jobArgs or something …

• sdk/cwl/arvados_cwl/__init__.py
• Can you add a few comments, most importantly for the newly added RunnerJob? I also noticed that several others such as ArvadosJob and ArvCwlRunner could use some brief descriptions

• sdk/python/arvados/commands/run.py
  • typo in comment at line 113 “pathes”

• There were many places where variable names could use camel casing and “_” to separate words, but your call

• Would it be too hard to write some tests at least for the updates in init.py ?

• Does this need any documentation updates?

Radhika Chippada wrote:

Review comments:

• crunch_scripts/cwl-runner:
  • This print statement “print cwltool.main.versionstring()” : not sure it’s something you forgot to remove. If not, can it be converted into a log statement with some description?

That's to print the versions of several key packages to help with provenance/debugging. Converted to a logging statement.

• Can you please add a detailed description / comment about this script at the top of the file? Most importantly describing how the relationship between this and sdk/cwl/arvados-cwl/__init__.py works would be good to have here.

Added comment.

• In “np = argparse.Namespace()” : I could not tell why it is named as “np”. May be args (if it does not conflict with other things or jobArgs or something …

Renamed to "args".

• sdk/cwl/arvados_cwl/__init__.py
• Can you add a few comments, most importantly for the newly added RunnerJob? I also noticed that several others such as ArvadosJob and ArvCwlRunner could use some brief descriptions

Added brief documentation strings to all the classes in that module.

• sdk/python/arvados/commands/run.py
  • typo in comment at line 113 “pathes”

Fixed.

• There were many places where variable names could use camel casing and “_” to separate words, but your call

I don't want to do this right now because of risk that in could introduce typos and hard-to-find bugs.

• Would it be too hard to write some tests at least for the updates in init.py ?

I had tests but failed to commit them. Thanks for that. Test added.

• Does this need any documentation updates?

Probably, but not in this story. I think this needs to get merged and squared away before hitting the documentation. Also as discussed I'm continuing to work on the CWL user guide this sprint.

Thanks.

Reviewed at f7e07c3a28d4f4e1adbd957749bd825e44a2e523

• Thanks for the comments. Made it easier to understand the code.

• In sdk/cwl/arvados_cwl/__init__.py :
  • The argument description for —local is a bit confusing. Can we skip “and submits jobs” here?
  • In the argument description for —submit, can we clarify that the job is submitted to (arvados?) server?

Tweaked the help text to try and make it a bit clearer.

• versionstring(): Is the ordering of strings as intended here? (for the last 4) do we want it to be: "arvados-python-client", arvpkg⁰.version, "cwltool", cwlpkg⁰.version instead?

You're right I messed that up. Thanks for catching that.

• sdk/cwl tests are failing (failures=1, errors=2) with run-tests script

Fixed.

• (If comments are allowed) I think adding a link in tests/*/*.cwl files briefly explaining what they are intended for might be helpful

Added comments.

• Can you please rename “tests/inp” as “tests/input” or “tests/inputs” ?

Done.

• (Since python is still new to me, I urge) Can you please revisit the indentations in test_submit.py at lines 37, 38, and 42? I am thinking in api.collections().create.assert_has_calls there are 5 mock.call(…) statements but they are not all lined up and I had difficulty understanding at first. Thanks.

Done.

Now at 9d035125ea1f8b6bd32fa6f862b32ba9308ebc66