ga4gh / task-execution-schemas

Apache License 2.0
80 stars 28 forks source link

Remove references to Docker. #23

Closed buchanae closed 7 years ago

buchanae commented 7 years ago

Docker is not the only container runtime. We should not preemptively hard-code our schema to docker. Other runtimes include rkt, runC, lxc, systemd-nspawn, openvz, and more.

The OCI is standardizing the container image format and runtime spec, which Docker is compatible with. Tasks should require only that the containers/images be in this format, and task execution servers should guarantee that containers will be run with an OCI compatible runtime, but there is no need require docker specifically. Docker is an implementation detail.

geoffjentry commented 7 years ago

👍

kellrott commented 7 years ago

If I accept this, will it break the current Bunny and Cromwell runners? Do we have a plan to coordinate with @ntijanic and @geoffjentry as we accept these 'schema-breaking' changes on the road to v1.0 ?

buchanae commented 7 years ago

Yes, this will probably break things.

TES support in Bunny, Cromwell, and Funnel is all alpha level, and this spec is pre version 1.0, so breaking things is expected.

Not sure what @geoffjentry and @ntijanic would prefer to do, but maybe closely following this repo and its pull requests is the best way to track changes? And/or we could send updates to the containers and workflows mailing list?

buchanae commented 7 years ago

After discussing this offline, maybe "containers" isn't the best name. Other ideas:

jbingham commented 7 years ago

@buchanae The possible names you list are for the array of sequentially executed sub-tasks that TES will run on the same VM, right?

I like the name subtask. It makes sense that a task execution service would have subtasks.

IIUC, the workflow execution engine will decide when to fuse together multiple "tasks" (Cromwell) or "steps" (CWL) into a single TES call. This is an optimal optimization step that doesn't change the output; it just reduces file copying and VM startup time, as well as perhaps billing. Is that right?

The other related concept is an array of tasks to be executed in parallel in separate instances of the same image. Ie, TES could natively support task arrays, like SGE and other schedulers. Is there any plan to support sharding like this?

buchanae commented 7 years ago

The possible names you list are for the array of sequentially executed sub-tasks that TES will run on the same VM, right?

Yes. This would replace DockerExecutor and Task.docker in the existing schema.

IIUC, the workflow execution engine will decide when to fuse together multiple "tasks" (Cromwell) or "steps" (CWL) into a single TES call. This is an optimal optimization step that doesn't change the output; it just reduces file copying and VM startup time, as well as perhaps billing. Is that right?

Yes.

The other related concept is an array of tasks to be executed in parallel in separate instances of the same image. Ie, TES could natively support task arrays, like SGE and other schedulers. Is there any plan to support sharding like this?

One option is to implement this in a client, e.g.:

import tesclient
import myapp

handles = []
for x in range(20):
  task = myapp.SleepTask(duration=x)
  handles.append(task)

tesclient.wait(handles)

Is a task array an unnecessary convenience when a loop is more explicit, obvious, and simple?

To argue against myself, having a bulk submission endpoint would be more efficient when you need to submit 10K tasks. But, there are other options to consider if efficiency during submission is the sole motivation.

geoffjentry commented 7 years ago

@jbingham Not objecting to the terminology here but note that task isn't the right terminology in the WDL world. For us task is the abstract definition (think a function) and the call is the concrete implementation (the function call) - job is the physical manifestation of the call of the task.

As per GA4GH definitions, to the extent that one cares, it'd be task and taskinstance (vs task and call in the WDL space), although I've always found TaskInstance to be unwieldy to use in real life.

jbingham commented 7 years ago

@geoffjentry Thanks for clarifying. My terminology isn't up to date :-)

geoffjentry commented 7 years ago

@jbingham NP, just being a pedant. My actual opinion was the "Not objecting to the terminology here" part :)

buchanae commented 7 years ago

I've updated to use "command" instead of "container" and rebased. Can I get some thumbs up/down so we can move forward with this?

jbingham commented 7 years ago

Should it be "commands", plural, since there can be multiple?

Otherwise, LGTM.

buchanae commented 7 years ago

@jbingham It is Task.commands but the message type is not Command

adamstruck commented 7 years ago

Suggestions from the GA4GH call today:

DockerExecutor -> Executor
Task.docker -> task.executors