From 8d4ec10fc26d93d282845c789cd61da79e4b2836 Mon Sep 17 00:00:00 2001 From: Peter Amstutz Date: Mon, 19 Dec 2016 13:39:46 -0500 Subject: [PATCH] 10497: Add source line reporting to errors and fix tests to work with CommentedMap/Seq behavior. --- sdk/cwl/arvados_cwl/__init__.py | 4 +- sdk/cwl/arvados_cwl/arvcontainer.py | 8 ++-- sdk/cwl/arvados_cwl/arvdocker.py | 73 +++++++++++++++-------------- sdk/cwl/arvados_cwl/arvjob.py | 3 +- sdk/cwl/arvados_cwl/arvworkflow.py | 34 ++++++++------ sdk/cwl/arvados_cwl/pathmapper.py | 8 ++-- sdk/cwl/arvados_cwl/runner.py | 5 +- sdk/cwl/tests/test_container.py | 9 ++-- sdk/cwl/tests/test_job.py | 7 +-- sdk/cwl/tests/wf/expect_packed.cwl | 43 +++++++++-------- sdk/cwl/tests/wf/scatter2_subwf.cwl | 50 +++++++++++--------- 11 files changed, 138 insertions(+), 106 deletions(-) diff --git a/sdk/cwl/arvados_cwl/__init__.py b/sdk/cwl/arvados_cwl/__init__.py index 25bddc002f..c1de2a8b36 100644 --- a/sdk/cwl/arvados_cwl/__init__.py +++ b/sdk/cwl/arvados_cwl/__init__.py @@ -18,6 +18,7 @@ from cwltool.errors import WorkflowException import cwltool.main import cwltool.workflow import schema_salad +from schema_salad.sourceline import SourceLine import arvados import arvados.config @@ -194,7 +195,7 @@ class ArvCwlRunner(object): def check_writable(self, obj): if isinstance(obj, dict): if obj.get("writable"): - raise UnsupportedRequirement("InitialWorkDir feature 'writable: true' not supported") + raise SourceLine(obj, "writable", UnsupportedRequirement).makeError("InitialWorkDir feature 'writable: true' not supported") for v in obj.itervalues(): self.check_writable(v) if isinstance(obj, list): @@ -629,6 +630,7 @@ def main(args, stdout, stderr, api_client=None, keep_client=None): arvargs.conformance_test = None arvargs.use_container = True arvargs.relax_path_checks = True + arvargs.validate = None return cwltool.main.main(args=arvargs, stdout=stdout, diff --git a/sdk/cwl/arvados_cwl/arvcontainer.py b/sdk/cwl/arvados_cwl/arvcontainer.py index dbbd83da2e..827e92d679 100644 --- a/sdk/cwl/arvados_cwl/arvcontainer.py +++ b/sdk/cwl/arvados_cwl/arvcontainer.py @@ -4,6 +4,8 @@ import os import ruamel.yaml as yaml +from schema_salad.sourceline import SourceLine + from cwltool.errors import WorkflowException from cwltool.process import get_feature, UnsupportedRequirement, shortname from cwltool.pathmapper import adjustFiles @@ -66,17 +68,17 @@ class ArvadosContainer(object): } if self.generatefiles["listing"]: - raise UnsupportedRequirement("InitialWorkDirRequirement not supported with --api=containers") + raise SourceLine(self.tool.get_requirement("InitialWorkDirRequirement")[0], None, UnsupportedRequirement).makeError("InitialWorkDirRequirement not supported with --api=containers") container_request["environment"] = {"TMPDIR": self.tmpdir, "HOME": self.outdir} if self.environment: container_request["environment"].update(self.environment) if self.stdin: - raise UnsupportedRequirement("Stdin redirection currently not suppported") + raise SourceLine(self.tool.tool, "stdin", UnsupportedRequirement).makeError("Stdin redirection currently not suppported") if self.stderr: - raise UnsupportedRequirement("Stderr redirection currently not suppported") + raise SourceLine(self.tool.tool, "stderr", UnsupportedRequirement).makeError("Stderr redirection currently not suppported") if self.stdout: mounts["stdout"] = {"kind": "file", diff --git a/sdk/cwl/arvados_cwl/arvdocker.py b/sdk/cwl/arvados_cwl/arvdocker.py index 7f6ab587d3..88c5dd2d4f 100644 --- a/sdk/cwl/arvados_cwl/arvdocker.py +++ b/sdk/cwl/arvados_cwl/arvdocker.py @@ -2,6 +2,8 @@ import logging import sys import threading +from schema_salad.sourceline import SourceLine + import cwltool.docker from cwltool.errors import WorkflowException import arvados.commands.keepdocker @@ -16,6 +18,8 @@ def arv_docker_get_image(api_client, dockerRequirement, pull_image, project_uuid if "dockerImageId" not in dockerRequirement and "dockerPull" in dockerRequirement: dockerRequirement["dockerImageId"] = dockerRequirement["dockerPull"] + if hasattr(dockerRequirement, 'lc'): + dockerRequirement.lc.data["dockerImageId"] = dockerRequirement.lc.data["dockerPull"] global cached_lookups global cached_lookups_lock @@ -23,45 +27,46 @@ def arv_docker_get_image(api_client, dockerRequirement, pull_image, project_uuid if dockerRequirement["dockerImageId"] in cached_lookups: return cached_lookups[dockerRequirement["dockerImageId"]] - sp = dockerRequirement["dockerImageId"].split(":") - image_name = sp[0] - image_tag = sp[1] if len(sp) > 1 else None - - images = arvados.commands.keepdocker.list_images_in_arv(api_client, 3, - image_name=image_name, - image_tag=image_tag) - - if not images: - # Fetch Docker image if necessary. - cwltool.docker.get_image(dockerRequirement, pull_image) - - # Upload image to Arvados - args = [] - if project_uuid: - args.append("--project-uuid="+project_uuid) - args.append(image_name) - if image_tag: - args.append(image_tag) - logger.info("Uploading Docker image %s", ":".join(args[1:])) - try: - arvados.commands.keepdocker.main(args, stdout=sys.stderr) - except SystemExit as e: - if e.code: - raise WorkflowException("keepdocker exited with code %s" % e.code) + with SourceLine(dockerRequirement, "dockerImageId", WorkflowException): + sp = dockerRequirement["dockerImageId"].split(":") + image_name = sp[0] + image_tag = sp[1] if len(sp) > 1 else None images = arvados.commands.keepdocker.list_images_in_arv(api_client, 3, image_name=image_name, image_tag=image_tag) - if not images: - raise WorkflowException("Could not find Docker image %s:%s" % (image_name, image_tag)) - - pdh = api_client.collections().get(uuid=images[0][0]).execute()["portable_data_hash"] - - with cached_lookups_lock: - cached_lookups[dockerRequirement["dockerImageId"]] = pdh - - return pdh + if not images: + # Fetch Docker image if necessary. + cwltool.docker.get_image(dockerRequirement, pull_image) + + # Upload image to Arvados + args = [] + if project_uuid: + args.append("--project-uuid="+project_uuid) + args.append(image_name) + if image_tag: + args.append(image_tag) + logger.info("Uploading Docker image %s", ":".join(args[1:])) + try: + arvados.commands.keepdocker.main(args, stdout=sys.stderr) + except SystemExit as e: + if e.code: + raise WorkflowException("keepdocker exited with code %s" % e.code) + + images = arvados.commands.keepdocker.list_images_in_arv(api_client, 3, + image_name=image_name, + image_tag=image_tag) + + if not images: + raise WorkflowException("Could not find Docker image %s:%s" % (image_name, image_tag)) + + pdh = api_client.collections().get(uuid=images[0][0]).execute()["portable_data_hash"] + + with cached_lookups_lock: + cached_lookups[dockerRequirement["dockerImageId"]] = pdh + + return pdh def arv_docker_clear_cache(): global cached_lookups diff --git a/sdk/cwl/arvados_cwl/arvjob.py b/sdk/cwl/arvados_cwl/arvjob.py index 04e94aecec..d6055d3710 100644 --- a/sdk/cwl/arvados_cwl/arvjob.py +++ b/sdk/cwl/arvados_cwl/arvjob.py @@ -88,7 +88,8 @@ class ArvadosJob(object): (docker_req, docker_is_req) = get_feature(self, "DockerRequirement") if docker_req and kwargs.get("use_container") is not False: if docker_req.get("dockerOutputDirectory"): - raise UnsupportedRequirement("Option 'dockerOutputDirectory' of DockerRequirement not supported.") + raise SourceLine(docker_req, "dockerOutputDirectory", UnsupportedRequirement).makeError( + "Option 'dockerOutputDirectory' of DockerRequirement not supported.") runtime_constraints["docker_image"] = arv_docker_get_image(self.arvrunner.api, docker_req, pull_image, self.arvrunner.project_uuid) else: runtime_constraints["docker_image"] = arvados_jobs_image(self.arvrunner) diff --git a/sdk/cwl/arvados_cwl/arvworkflow.py b/sdk/cwl/arvados_cwl/arvworkflow.py index 703bb47d8e..8c1db3a817 100644 --- a/sdk/cwl/arvados_cwl/arvworkflow.py +++ b/sdk/cwl/arvados_cwl/arvworkflow.py @@ -3,6 +3,8 @@ import json import copy import logging +from schema_salad.sourceline import SourceLine, cmap + from cwltool.pack import pack from cwltool.load_tool import fetch_document from cwltool.process import shortname @@ -46,7 +48,7 @@ def upload_workflow(arvRunner, tool, job_order, project_uuid, uuid=None, "workflow": { "name": name, "description": tool.tool.get("doc", ""), - "definition":yaml.safe_dump(packed) + "definition":yaml.round_trip_dump(packed) }} if project_uuid: body["workflow"]["owner_uuid"] = project_uuid @@ -94,23 +96,25 @@ class ArvadosWorkflow(Workflow): joborder_keepmount = copy.deepcopy(joborder) def keepmount(obj): - if "location" not in obj: - raise WorkflowException("%s object is missing required 'location' field: %s" % (obj["class"], obj)) - if obj["location"].startswith("keep:"): - obj["location"] = "/keep/" + obj["location"][5:] - if "listing" in obj: - del obj["listing"] - elif obj["location"].startswith("_:"): - del obj["location"] - else: - raise WorkflowException("Location is not a keep reference or a literal: '%s'" % obj["location"]) + with SourceLine(obj, None, WorkflowException): + if "location" not in obj: + raise WorkflowException("%s object is missing required 'location' field: %s" % (obj["class"], obj)) + with SourceLine(obj, "location", WorkflowException): + if obj["location"].startswith("keep:"): + obj["location"] = "/keep/" + obj["location"][5:] + if "listing" in obj: + del obj["listing"] + elif obj["location"].startswith("_:"): + del obj["location"] + else: + raise WorkflowException("Location is not a keep reference or a literal: '%s'" % obj["location"]) adjustFileObjs(joborder_keepmount, keepmount) adjustDirObjs(joborder_keepmount, keepmount) adjustFileObjs(packed, keepmount) adjustDirObjs(packed, keepmount) - wf_runner = { + wf_runner = cmap({ "class": "CommandLineTool", "baseCommand": "cwltool", "inputs": self.tool["inputs"], @@ -121,15 +125,15 @@ class ArvadosWorkflow(Workflow): "class": "InitialWorkDirRequirement", "listing": [{ "entryname": "workflow.cwl", - "entry": yaml.safe_dump(packed).replace("\\", "\\\\").replace('$(', '\$(').replace('${', '\${') + "entry": yaml.round_trip_dump(packed).replace("\\", "\\\\").replace('$(', '\$(').replace('${', '\${') }, { "entryname": "cwl.input.yml", - "entry": yaml.safe_dump(joborder_keepmount).replace("\\", "\\\\").replace('$(', '\$(').replace('${', '\${') + "entry": yaml.round_trip_dump(joborder_keepmount).replace("\\", "\\\\").replace('$(', '\$(').replace('${', '\${') }] }], "hints": workflowobj["hints"], "arguments": ["--no-container", "--move-outputs", "--preserve-entire-environment", "workflow.cwl#main", "cwl.input.yml"] - } + }) kwargs["loader"] = self.doc_loader kwargs["avsc_names"] = self.doc_schema return ArvadosCommandTool(self.arvrunner, wf_runner, **kwargs).job(joborder, output_callback, **kwargs) diff --git a/sdk/cwl/arvados_cwl/pathmapper.py b/sdk/cwl/arvados_cwl/pathmapper.py index 74d9481ff5..e63161dbb1 100644 --- a/sdk/cwl/arvados_cwl/pathmapper.py +++ b/sdk/cwl/arvados_cwl/pathmapper.py @@ -6,6 +6,8 @@ import os import arvados.commands.run import arvados.collection +from schema_salad.sourceline import SourceLine + from cwltool.pathmapper import PathMapper, MapperEnt, abspath, adjustFileObjs, adjustDirObjs from cwltool.workflow import WorkflowException @@ -46,11 +48,11 @@ class ArvPathMapper(PathMapper): if "contents" in srcobj: pass else: - raise WorkflowException("File literal '%s' is missing contents" % src) + raise SourceLine(srcobj, "location", WorkflowException).makeError("File literal '%s' is missing contents" % src) elif src.startswith("arvwf:"): self._pathmap[src] = MapperEnt(src, src, "File") else: - raise WorkflowException("Input file path '%s' is invalid" % st) + raise SourceLine(srcobj, "location", WorkflowException).makeError("Input file path '%s' is invalid" % st) if "secondaryFiles" in srcobj: for l in srcobj["secondaryFiles"]: self.visit(l, uploadfiles) @@ -76,7 +78,7 @@ class ArvPathMapper(PathMapper): with c.open(path + "/" + obj["basename"], "w") as f: f.write(obj["contents"].encode("utf-8")) else: - raise WorkflowException("Don't know what to do with '%s'" % obj["location"]) + raise SourceLine(obj, "location", WorkflowException).makeError("Don't know what to do with '%s'" % obj["location"]) def setup(self, referenced_files, basedir): # type: (List[Any], unicode) -> None diff --git a/sdk/cwl/arvados_cwl/runner.py b/sdk/cwl/arvados_cwl/runner.py index dea47567a7..e902140f19 100644 --- a/sdk/cwl/arvados_cwl/runner.py +++ b/sdk/cwl/arvados_cwl/runner.py @@ -6,6 +6,8 @@ import json import re from cStringIO import StringIO +from schema_salad.sourceline import SourceLine + import cwltool.draft2tool from cwltool.draft2tool import CommandLineTool import cwltool.workflow @@ -112,7 +114,8 @@ def upload_docker(arvrunner, tool): if docker_req: if docker_req.get("dockerOutputDirectory"): # TODO: can be supported by containers API, but not jobs API. - raise UnsupportedRequirement("Option 'dockerOutputDirectory' of DockerRequirement not supported.") + raise SourceLine(docker_req, "dockerOutputDirectory", UnsupportedRequirement).makeError( + "Option 'dockerOutputDirectory' of DockerRequirement not supported.") arv_docker_get_image(arvrunner.api, docker_req, True, arvrunner.project_uuid) elif isinstance(tool, cwltool.workflow.Workflow): for s in tool.steps: diff --git a/sdk/cwl/tests/test_container.py b/sdk/cwl/tests/test_container.py index 0394988ccd..45e2c7c45f 100644 --- a/sdk/cwl/tests/test_container.py +++ b/sdk/cwl/tests/test_container.py @@ -7,6 +7,7 @@ import os import functools import cwltool.process from schema_salad.ref_resolver import Loader +from schema_salad.sourceline import cmap from .matcher import JsonDiffMatcher @@ -34,12 +35,12 @@ class TestContainer(unittest.TestCase): document_loader, avsc_names, schema_metadata, metaschema_loader = cwltool.process.get_schema("v1.0") - tool = { + tool = cmap({ "inputs": [], "outputs": [], "baseCommand": "ls", "arguments": [{"valueFrom": "$(runtime.outdir)"}] - } + }) make_fs_access=functools.partial(arvados_cwl.CollectionFsAccess, api_client=runner.api) arvtool = arvados_cwl.ArvadosCommandTool(runner, tool, work_api="containers", avsc_names=avsc_names, basedir="", make_fs_access=make_fs_access, loader=Loader({})) @@ -87,7 +88,7 @@ class TestContainer(unittest.TestCase): runner.api.collections().get().execute.return_value = { "portable_data_hash": "99999999999999999999999999999993+99"} - tool = { + tool = cmap({ "inputs": [], "outputs": [], "hints": [{ @@ -105,7 +106,7 @@ class TestContainer(unittest.TestCase): "partition": "blurb" }], "baseCommand": "ls" - } + }) make_fs_access=functools.partial(arvados_cwl.CollectionFsAccess, api_client=runner.api) arvtool = arvados_cwl.ArvadosCommandTool(runner, tool, work_api="containers", avsc_names=avsc_names, make_fs_access=make_fs_access, diff --git a/sdk/cwl/tests/test_job.py b/sdk/cwl/tests/test_job.py index 15da596eae..7675e3d4bc 100644 --- a/sdk/cwl/tests/test_job.py +++ b/sdk/cwl/tests/test_job.py @@ -11,6 +11,7 @@ import arvados import arvados_cwl import cwltool.process from schema_salad.ref_resolver import Loader +from schema_salad.sourceline import cmap from .mock_discovery import get_rootDesc from .matcher import JsonDiffMatcher @@ -34,12 +35,12 @@ class TestJob(unittest.TestCase): list_images_in_arv.return_value = [["zzzzz-4zz18-zzzzzzzzzzzzzzz"]] runner.api.collections().get().execute.return_vaulue = {"portable_data_hash": "99999999999999999999999999999993+99"} - tool = { + tool = cmap({ "inputs": [], "outputs": [], "baseCommand": "ls", "arguments": [{"valueFrom": "$(runtime.outdir)"}] - } + }) make_fs_access=functools.partial(arvados_cwl.CollectionFsAccess, api_client=runner.api) arvtool = arvados_cwl.ArvadosCommandTool(runner, tool, work_api="jobs", avsc_names=avsc_names, basedir="", make_fs_access=make_fs_access, loader=Loader({})) @@ -305,7 +306,7 @@ class TestWorkflow(unittest.TestCase): find_or_create=True) mockcollection().open().__enter__().write.assert_has_calls([mock.call(subwf)]) - mockcollection().open().__enter__().write.assert_has_calls([mock.call('{sleeptime: 5}')]) + mockcollection().open().__enter__().write.assert_has_calls([mock.call('sleeptime: 5')]) def test_default_work_api(self): arvados_cwl.add_arv_hints() diff --git a/sdk/cwl/tests/wf/expect_packed.cwl b/sdk/cwl/tests/wf/expect_packed.cwl index 1622f4841b..f4d60dbfe8 100644 --- a/sdk/cwl/tests/wf/expect_packed.cwl +++ b/sdk/cwl/tests/wf/expect_packed.cwl @@ -1,31 +1,34 @@ +cwlVersion: v1.0 $graph: -- baseCommand: cat - class: CommandLineTool - id: '#submit_tool.cwl' +- class: CommandLineTool + requirements: + - class: DockerRequirement + dockerPull: debian:8 inputs: - - default: {class: File, location: 'keep:99999999999999999999999999999991+99/tool/blub.txt'} - id: '#submit_tool.cwl/x' - inputBinding: {position: 1} + - id: '#submit_tool.cwl/x' type: File + default: + class: File + location: keep:99999999999999999999999999999991+99/tool/blub.txt + inputBinding: + position: 1 outputs: [] - requirements: - - {class: DockerRequirement, dockerPull: 'debian:8'} + baseCommand: cat + id: '#submit_tool.cwl' - class: Workflow - id: '#main' inputs: - - default: {basename: blorp.txt, class: File, location: 'keep:99999999999999999999999999999991+99/input/blorp.txt'} - id: '#main/x' + - id: '#main/x' type: File - - default: {basename: 99999999999999999999999999999998+99, class: Directory, location: 'keep:99999999999999999999999999999998+99'} - id: '#main/y' + default: {class: File, location: 'keep:99999999999999999999999999999991+99/input/blorp.txt', + basename: blorp.txt} + - id: '#main/y' type: Directory - - default: - basename: anonymous - class: Directory - listing: - - {basename: renamed.txt, class: File, location: 'keep:99999999999999999999999999999998+99/file1.txt'} - id: '#main/z' + default: {class: Directory, location: 'keep:99999999999999999999999999999998+99', + basename: 99999999999999999999999999999998+99} + - id: '#main/z' type: Directory + default: {class: Directory, basename: anonymous, listing: [{basename: renamed.txt, + class: File, location: 'keep:99999999999999999999999999999998+99/file1.txt'}]} outputs: [] steps: - id: '#main/step1' @@ -33,4 +36,4 @@ $graph: - {id: '#main/step1/x', source: '#main/x'} out: [] run: '#submit_tool.cwl' -cwlVersion: v1.0 + id: '#main' diff --git a/sdk/cwl/tests/wf/scatter2_subwf.cwl b/sdk/cwl/tests/wf/scatter2_subwf.cwl index 0ae1cf04f0..daf18b11ca 100644 --- a/sdk/cwl/tests/wf/scatter2_subwf.cwl +++ b/sdk/cwl/tests/wf/scatter2_subwf.cwl @@ -1,33 +1,41 @@ +cwlVersion: v1.0 $graph: - class: Workflow - hints: - - {class: 'http://arvados.org/cwl#RunInSingleContainer'} id: '#main' inputs: - - {id: '#main/sleeptime', type: int} + - type: int + id: '#main/sleeptime' outputs: - - {id: '#main/out', outputSource: '#main/sleep1/out', type: string} - requirements: - - {class: InlineJavascriptRequirement} - - {class: ScatterFeatureRequirement} - - {class: StepInputExpressionRequirement} - - {class: SubworkflowFeatureRequirement} + - type: string + outputSource: '#main/sleep1/out' + id: '#main/out' steps: - - id: '#main/sleep1' - in: - - {id: '#main/sleep1/blurb', valueFrom: "${\n return String(inputs.sleeptime)\ - \ + \"b\";\n}\n"} - - {id: '#main/sleep1/sleeptime', source: '#main/sleeptime'} + - in: + - valueFrom: | + ${ + return String(inputs.sleeptime) + "b"; + } + id: '#main/sleep1/blurb' + - source: '#main/sleeptime' + id: '#main/sleep1/sleeptime' out: ['#main/sleep1/out'] run: - baseCommand: sleep class: CommandLineTool inputs: - - id: '#main/sleep1/sleeptime' + - type: int inputBinding: {position: 1} - type: int + id: '#main/sleep1/sleeptime' outputs: - - id: '#main/sleep1/out' - outputBinding: {outputEval: out} - type: string -cwlVersion: v1.0 \ No newline at end of file + - type: string + outputBinding: + outputEval: out + id: '#main/sleep1/out' + baseCommand: sleep + id: '#main/sleep1' + requirements: + - {class: InlineJavascriptRequirement} + - {class: ScatterFeatureRequirement} + - {class: StepInputExpressionRequirement} + - {class: SubworkflowFeatureRequirement} + hints: + - class: http://arvados.org/cwl#RunInSingleContainer \ No newline at end of file -- 2.30.2