Feature #8442
closed[Crunch2] arvados-cwl-runner submits to crunch2 containers API
Added by Peter Amstutz over 8 years ago. Updated about 8 years ago.
Description
Requirements:
Provide portable data hashes for Docker images.
Submits container requests
Will run from shell node / crunch v1 cwl-runner
Related issues
Updated by Peter Amstutz about 8 years ago
- Assigned To set to Peter Amstutz
- Story points set to 2.0
Updated by Peter Amstutz about 8 years ago
- Status changed from New to In Progress
Updated by Peter Amstutz about 8 years ago
- Target version changed from 2016-06-08 sprint to 2016-06-22 sprint
Updated by Tom Clegg about 8 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.
Updated by Peter Amstutz about 8 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.
Updated by Brett Smith about 8 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.
Updated by Peter Amstutz about 8 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).
Updated by Tom Clegg about 8 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
?
Updated by Peter Amstutz about 8 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.
Updated by Radhika Chippada about 8 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
Updated by Radhika Chippada about 8 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.
Updated by Peter Amstutz about 8 years ago
Updated by Peter Amstutz about 8 years ago
- Status changed from In Progress to Resolved
- % Done changed from 75 to 100
Applied in changeset arvados|commit:e4b1a745a97af1d65bb1e03f770b34457003eae2.