Arvados

8442-crunch-run-enable-net 2ce42bc LGTM

8442-dispatch-support-api 86170c2 is already being obsoleted by #9374. I'm guessing this is motivated by a bug -- perhaps that if you specify a bool for API, then the other constraints don't get loaded because the JSON object isn't a map[string]int64 -- and the bug is holding up testing for 8442? If so, LGTM, but otherwise I'd rather not create merge conflicts for #9374.

8442-cwl-crunch2 is ready for review.

This adds the --crunch2 flag which submits container requests. This passes the CWL conformance tests except those which test for currently unimplemented features of standard input redirection and CreateFileRequirement.

This branch is a large change because it includes refactoring the code into multiple files, so there are substantial chunks in the diff that are actually just from code moving around but not actually changing very much.

Peter Amstutz wrote:

This adds the --crunch2 flag which submits container requests. This passes the CWL conformance tests except those which test for currently unimplemented features of standard input redirection and CreateFileRequirement.

What happens when we introduce Crunch 3? I won't bikeshed this endlessly but I think a name like --containers might be less trouble over the long run.

Maybe there's a separate story suggested here that the API server should advertise in the discovery document whether it's running Crunch v2, so clients can default to submitting containers when that's true, and jobs or pipelines otherwise.

Brett Smith wrote:

Peter Amstutz wrote:

This adds the --crunch2 flag which submits container requests. This passes the CWL conformance tests except those which test for currently unimplemented features of standard input redirection and CreateFileRequirement.

What happens when we introduce Crunch 3? I won't bikeshed this endlessly but I think a name like --containers might be less trouble over the long run.

Maybe there's a separate story suggested here that the API server should advertise in the discovery document whether it's running Crunch v2, so clients can default to submitting containers when that's true, and jobs or pipelines otherwise.

I had the same thought, auto-detecting which API to use is a better user experience. But for development we still need flags to override default behavior. I prefer to avoid a generic flag name like "--containers" because it conflicts with the generic use of the word (for example, cwltool already has flag --no-containers, which means something entirely unrelated to the crunch v2 containers API).

I'm not keen on promoting the term "crunch2" from internal dev-speak to public API. The containers API is /arvados/v1/containers so the term crunch2 seems likely to end up in some awkward corner quickly, and more so when /arvados/v2/containers appears.

I sort of like the "discover whether containers are supported here" idea, but even with a site-wide setting I think we'd want an override for testing, so for now I think we should default to jobs (currently the only thing supported anywhere) and have a flag for containers.

Leaving the door open for "specify job when container becomes the default" might be a nice touch, even though I prefer to believe nobody will ever want to do that.

How about something like --submit=container?

Tom Clegg wrote:

I sort of like the "discover whether containers are supported here" idea, but even with a site-wide setting I think we'd want an override for testing, so for now I think we should default to jobs (currently the only thing supported anywhere) and have a flag for containers.

Leaving the door open for "specify job when container becomes the default" might be a nice touch, even though I prefer to believe nobody will ever want to do that.

How about something like --submit=container?

--submit is already taken (it means submit a workflow runner job, instead of running the workflow locally) but --api=jobs and --api=containers would be fine.

Review at 2435a72a:

init.py

• This comment: “Execute a CWL tool or workflow, submit crunch jobs, wait for them to complete, and report output.” Can we say “processes” (similar to work_unit) here instead of jobs?

• init -> self.jobs : Should this be called “self.processes” instead since both job and container typed objects are added to it?

• “Overall job status is %s” -> process status?

• Throughout the code, can you please revisit any “job” references to make sure to use the word “process” when it is not an arvados Job record? I think using the word “job” to only refer to Job type objects will greatly help avoid any confusion.

• Most of the args parsed have help message. Would you be able to add help msg for conformance_test and job_order?

• Instead of --crunch1 and --crunch2, can we offer something like --crunch-version with acceptable values 1 or 2 for now, and call self.crunch2 as self.crunch_version?

arvcontainer.py

• Should this comment say “container” instead of job? "Submit and manage a Crunch job for executing a CWL CommandLineTool"

• Is self.command_line coming from cwltool?

• should we be hardcoding output_path, cwd, mounts?

• In the following, should “vwd = …” after the raise be removed?
  

  if self.generatefiles:
              raise UnsupportedRequirement("Generate files not supported")
  
              vwd = arvados.collection.Collection(api_client=self.arvrunner.api_client)
              ...
  

done.py

• We are getting manifest text from api server at line 18 to set it to the collection being created at line 32. Seems wasteful and like a potential performance improvement area.

1 failure and 2 errors in sdk/cwl with run-tests

At cd206dc9

• Tests are passing now

• jobs -> processes and crunch2 update are helpful. Thanks.

• The arvcontainer.py comments are not all addressed yet (the comment and dead code etc)

• It appears that it is possible to rename crunch2 as work_api in arvtool.py as well?

Thanks.

Radhika Chippada wrote:

At cd206dc9

• The arvcontainer.py comments are not all addressed yet (the comment and dead code etc)

Done.

• It appears that it is possible to rename crunch2 as work_api in arvtool.py as well?

Done.

Now @ 691a08f

LGTM