Feature #8442

[Crunch2] arvados-cwl-runner submits to crunch2 containers API

Added by Peter Amstutz over 5 years ago. Updated about 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
06/07/2016
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
1.0
Release:
Release relationship:
Auto

Description

Requirements:

Provide portable data hashes for Docker images.

Submits container requests

Will run from shell node / crunch v1 cwl-runner


Subtasks

Task #9288: Review 8442-cwl-crunch2ResolvedPeter Amstutz

Task #9287: Submit container requestsResolvedPeter Amstutz

Task #9399: Review 8442-crunch-run-enable-netResolvedPeter Amstutz

Task #9400: Review 8442-dispatch-support-apiClosedPeter Amstutz


Related issues

Related to Arvados - Feature #9307: [CWL] Arvados specific hint to use writable keep mountResolved10/05/2016

Associated revisions

Revision 0ac69cee
Added by Peter Amstutz over 5 years ago

Merge branch '8442-crunch-run-enable-net' refs #8442

Revision e4b1a745
Added by Peter Amstutz about 5 years ago

Merge branch 'origin-8442-cwl-crunch2' closes #8442

Revision ff6cb85a (diff)
Added by Peter Amstutz about 5 years ago

Update run-build-packages versions for schema_salad and cwltool, refs #8442

History

#1 Updated by Brett Smith over 5 years ago

  • Target version set to 2016-06-08 sprint

#2 Updated by Peter Amstutz over 5 years ago

  • Description updated (diff)

#3 Updated by Peter Amstutz over 5 years ago

  • Assigned To set to Peter Amstutz
  • Story points set to 2.0

#4 Updated by Peter Amstutz over 5 years ago

  • Description updated (diff)

#5 Updated by Peter Amstutz over 5 years ago

  • Status changed from New to In Progress

#6 Updated by Peter Amstutz over 5 years ago

  • Target version changed from 2016-06-08 sprint to 2016-06-22 sprint

#7 Updated by Peter Amstutz over 5 years ago

  • Story points changed from 2.0 to 1.0

#8 Updated by Tom Clegg over 5 years ago

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.

#9 Updated by Peter Amstutz over 5 years ago

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.

#10 Updated by Brett Smith over 5 years ago

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.

#11 Updated by Peter Amstutz over 5 years ago

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).

#12 Updated by Tom Clegg over 5 years ago

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?

#13 Updated by Peter Amstutz over 5 years ago

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.

#14 Updated by Radhika Chippada over 5 years ago

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

#15 Updated by Radhika Chippada about 5 years ago

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.

#16 Updated by Peter Amstutz about 5 years ago

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

#17 Updated by Radhika Chippada about 5 years ago

LGTM

#18 Updated by Peter Amstutz about 5 years ago

  • Status changed from In Progress to Resolved
  • % Done changed from 75 to 100

Applied in changeset arvados|commit:e4b1a745a97af1d65bb1e03f770b34457003eae2.

Also available in: Atom PDF