broadinstitute / cromwell

Scientific workflow engine designed for simplicity & scalability. Trivially transition between one off use cases to massive scale production environments
http://cromwell.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
996 stars 361 forks source link

Uninitialized optionals not initializing to `none` at task level #2946

Open mcovarr opened 6 years ago

mcovarr commented 6 years ago

Part of the #2942 work involved converting the config backend to use the WOM API. The config backend uses WDL to define its commands but it does not have a workflow (it's a WdlNamespace with a bunch of tasks). This conversion mostly went smoothly with the exception of uninitialized task optionals like docker_user found in the Local backend's submit-docker:

        runtime-attributes = """
        String? docker
        String? docker_user
        """
        submit = "/bin/bash ${script}"
        submit-docker = """
        docker run \
          --cidfile ${docker_cid} \
          --rm -i \
          ${"--user " + docker_user} \
          --entrypoint /bin/bash \
          -v ${cwd}:${docker_cwd} \
          ${docker} ${script}
        """

Evaluating that ${"--user " + docker_user} expression currently blows up with no useful diagnostics in the absence of an explicit docker_user input. To hack around this I changed the config backend to force in none inputs for all optional declarations in a task, but this would have the effect of clobbering any initialized optionals:

String? docker_user = "mobydock"

With the #2942 changes docker_user would be forced to none and "mobydock" would be lost (at sea).

It's not clear why this is happening when using the WOM API at a task level and not at the workflow level. There may be some none-initialization done at the workflow level that should get pushed down to tasks.

cjllanwarne commented 6 years ago

@mcovarr was this fixed by the optional-inputs fixup in #2952?

mcovarr commented 6 years ago

shrug? I'd love to check this but it's not a 30 blocker and I'm working on a 30 blocker. 🙂

kshakir commented 6 years ago

As of current develop (74edee4c5338d25283c0fcf72ddd016b8bf5c4e1) this is not fixed. We should create a unit test to check this somewhere.