diff --git a/README.rst b/README.rst index da1ea1c..f2bd2c3 100644 --- a/README.rst +++ b/README.rst @@ -81,6 +81,8 @@ The XML must conform to the `Slicer Execution Schema some text`` values that will be the suggested data for the control. diff --git a/slicer_cli_web/girder_worker_plugin/direct_docker_run.py b/slicer_cli_web/girder_worker_plugin/direct_docker_run.py index a9a4f76..6d8e502 100644 --- a/slicer_cli_web/girder_worker_plugin/direct_docker_run.py +++ b/slicer_cli_web/girder_worker_plugin/direct_docker_run.py @@ -8,6 +8,7 @@ from girder_worker.docker.transforms import BindMountVolume, ContainerStdOut from girder_worker.docker.transforms.girder import GirderFileIdToVolume from girder_worker_utils import _walk_obj +from girder_worker_utils.transform import Transform from girder_worker_utils.transforms.girder_io import GirderClientTransform from .cli_progress import CLIProgressCLIWriter @@ -67,6 +68,26 @@ def transform(self, **kwargs): return super().transform(**kwargs) +class CommaJoinedVolumes(Transform): + """One container argument spanning several volume transforms: their + resolved container paths, comma-joined in order.""" + + def __init__(self, volumes): + self._volumes = volumes + + def transform(self, **kwargs): + return ','.join(str(volume.transform(**kwargs)) for volume in self._volumes) + + def cleanup(self, **kwargs): + for volume in self._volumes: + volume.cleanup(**kwargs) + + def _repr_model_(self): + return '<%s.%s: [%s]>' % ( + self.__module__, self.__class__.__name__, + ', '.join(volume._repr_model_() for volume in self._volumes)) + + class GirderApiUrl(GirderClientTransform): def transform(self, **kwargs): return self.gc.urlBase @@ -85,6 +106,9 @@ def resolve(arg, **kwargs): path = arg.resolve_direct_file_path() if path: extra_volumes.append(path) + elif isinstance(arg, CommaJoinedVolumes): + for volume in arg._volumes: + resolve(volume) return arg _walk_obj(args, resolve) _walk_obj(kwargs, resolve) diff --git a/slicer_cli_web/prepare_task.py b/slicer_cli_web/prepare_task.py index e893d19..475d9f7 100644 --- a/slicer_cli_web/prepare_task.py +++ b/slicer_cli_web/prepare_task.py @@ -18,7 +18,7 @@ FOLDER_SUFFIX = '_folder' -def _to_file_volume(param, model): +def _to_file_volume(param, model, filename=None, gc=None, direct_path=None): from girder_worker.docker.transforms.girder import (GirderFileIdToVolume, GirderFolderIdToVolume, GirderItemIdToVolume) @@ -29,19 +29,55 @@ def _to_file_volume(param, model): girder_type = SLICER_TYPE_TO_GIRDER_MODEL_MAP[param.typ] if girder_type == 'folder': - return GirderFolderIdToVolume(model['_id'], folder_name=model['name']) + return GirderFolderIdToVolume(model['_id'], folder_name=filename or model['name'], + gc=gc) elif girder_type == 'item': - return GirderItemIdToVolume(model['_id']) + return GirderItemIdToVolume(model['_id'], gc=gc) - if not Setting().get(PluginSettings.DIRECT_PATH): - return GirderFileIdToVolume(model['_id'], filename=model['name']) + filename = filename or model['name'] + if direct_path is None: + direct_path = Setting().get(PluginSettings.DIRECT_PATH) + if not direct_path: + return GirderFileIdToVolume(model['_id'], filename=filename, gc=gc) try: path = File().getLocalFilePath(model) return DirectGirderFileIdToVolume(model['_id'], direct_file_path=path, - filename=model['name']) + filename=filename, gc=gc) except FilePathException: - return GirderFileIdToVolume(model['_id'], filename=model['name']) + return GirderFileIdToVolume(model['_id'], filename=filename, gc=gc) + + +def _list_item_name(model): + # Id-prefix so same-named files in a list can't collide when mounted, and + # drop commas so a name can't corrupt the comma-joined path argument. + return ('%s_%s' % (model['_id'], model['name'])).replace(',', '_') + + +def _to_file_volume_arg(param, value): + from girder_worker.girder_plugin.constants import PluginSettings + + from .girder_worker_plugin.direct_docker_run import CommaJoinedVolumes + + if not isinstance(value, list): + return _to_file_volume(param, value) + # The direct-path setting is constant across the list; read it once. + direct_path = Setting().get(PluginSettings.DIRECT_PATH) + volumes = [] + gc = None + for model in value: + volume = _to_file_volume(param, model, filename=_list_item_name(model), + gc=gc, direct_path=direct_path) + # Share the first volume's Girder client with the rest of the list so + # a large list does not mint one access token per file. + gc = volume.gc or gc + volumes.append(volume) + return CommaJoinedVolumes(volumes) + + +def _first_model(value): + """A possibly-multiple Girder input uses its first model as the primary.""" + return value[0] if isinstance(value, list) else value def _to_girder_api(param, value): @@ -56,7 +92,7 @@ def _to_girder_api(param, value): return value -def _parseParamValue(param, value, user, token): +def _parseParamValue(param, value, user, token, primary_only=False): if isinstance(value, bytes): value = value.decode('utf8') @@ -64,10 +100,21 @@ def _parseParamValue(param, value, user, token): if is_on_girder(param): girder_type = SLICER_TYPE_TO_GIRDER_MODEL_MAP[param.typ] curModel = ModelImporter.model(girder_type) - loaded = curModel.load(value, level=AccessType.READ, user=user) - if not loaded: - raise RestException('Invalid %s id (%s).' % (curModel.name, str(value))) - return loaded + + def load(value): + loaded = curModel.load(value, level=AccessType.READ, user=user) + if not loaded: + raise RestException('Invalid %s id (%s).' % (curModel.name, str(value))) + return loaded + + if getattr(param, 'multiple', None): + ids = [subvalue.strip() for subvalue in str(value).split(',')] + # Callers that only use the primary (first) model can skip loading + # the rest of the list. + if primary_only: + ids = ids[:1] + return [load(subvalue) for subvalue in ids] + return load(value) try: if param.isVector(): @@ -150,7 +197,7 @@ def _add_optional_input_param(param, args, user, token, templateParams): if is_on_girder(param): # Bindings - container_args.append(_to_file_volume(param, value)) + container_args.append(_to_file_volume_arg(param, value)) elif is_girder_api(param): # Bindings container_args.append(_to_girder_api(param, value)) @@ -200,7 +247,7 @@ def _add_indexed_input_param(param, args, user, token, templateParams=None): if is_on_girder(param): # Bindings - return _to_file_volume(param, value), value['name'] + return _to_file_volume_arg(param, value), _first_model(value)['name'] if is_girder_api(param): return _to_girder_api(param, value), value['name'] value = _processTemplates(value, param, templateParams) @@ -249,9 +296,11 @@ def _populateTemplateParams(params, user, token, index_params, opt_params, templ for param in index_params + opt_params: if param.identifier() in params: try: - value = _parseParamValue(param, params[param.identifier()], user, token) + value = _parseParamValue(param, params[param.identifier()], user, token, + primary_only=True) except Exception: continue + value = _first_model(value) value = value.get('name') if isinstance(value, dict) else value if value: templateParams[f'parameter_{param.name}'] = value @@ -301,7 +350,8 @@ def prepare_task(params, user, token, index_params, opt_params, SLICER_TYPE_TO_GIRDER_MODEL_MAP[param.typ] != 'folder'): primary_input_name = name reference['userId'] = str(user['_id']) - value = _parseParamValue(param, params[param.identifier()], user, token) + value = _first_model(_parseParamValue( + param, params[param.identifier()], user, token, primary_only=True)) itemId = value['_id'] if SLICER_TYPE_TO_GIRDER_MODEL_MAP[param.typ] == 'file': reference['fileId'] = str(value['_id']) diff --git a/slicer_cli_web/rest_slicer_cli.py b/slicer_cli_web/rest_slicer_cli.py index 6bc1a0c..79828fa 100644 --- a/slicer_cli_web/rest_slicer_cli.py +++ b/slicer_cli_web/rest_slicer_cli.py @@ -78,6 +78,9 @@ def _addInputParamToHandler(param, handlerDesc, required=True): 'Girder ID of input %s (if batch input, this is a regex ' 'for item names) - %s: %s' % (param.typ, param.identifier(), param.description)) + elif getattr(param, 'multiple', None): + desc = 'Comma separated Girder IDs of input %s - %s: %s' % ( + param.typ, param.identifier(), param.description) else: desc = 'Girder ID of input %s - %s: %s' % ( param.typ, param.identifier(), param.description) diff --git a/tests/data/MultipleSpec.xml b/tests/data/MultipleSpec.xml new file mode 100644 index 0000000..4f73846 --- /dev/null +++ b/tests/data/MultipleSpec.xml @@ -0,0 +1,25 @@ + + + Test + Assembles A Series + Consumes a multi-file series and produces a single output image. + 0.1.0 + + + Input/output parameters. + + inputSeries + + input + 0 + Files of the input series + + + outputImage + + output + 1 + Assembled output image + + + diff --git a/tests/girder_worker_plugin/test_direct_docker_run.py b/tests/girder_worker_plugin/test_direct_docker_run.py index 9d72a94..0668be6 100644 --- a/tests/girder_worker_plugin/test_direct_docker_run.py +++ b/tests/girder_worker_plugin/test_direct_docker_run.py @@ -3,6 +3,7 @@ import pytest from slicer_cli_web.girder_worker_plugin.direct_docker_run import (TEMP_VOLUME_DIRECT_MOUNT_PREFIX, + CommaJoinedVolumes, DirectGirderFileIdToVolume, run) @@ -54,3 +55,33 @@ def test_direct_docker_run(mocker, server, adminToken, file): assert kwargs['container_args'] == [target_path] # volumes assert len(kwargs['volumes']) == 2 + + +@pytest.mark.plugin('slicer_cli_web') +def test_direct_docker_run_multiple_files(mocker, server, file): + from girder.models.file import File + + docker_run_mock = mocker.patch( + 'slicer_cli_web.girder_worker_plugin.direct_docker_run._docker_run') + docker_run_mock.return_value = [] + + gc_mock = MockedGirderClient() + + path = File().getLocalFilePath(file) + + run(image='test', container_args=[CommaJoinedVolumes([ + DirectGirderFileIdToVolume(file['_id'], filename='direct.dat', + direct_file_path=path, gc=gc_mock), + DirectGirderFileIdToVolume(file['_id'], filename='downloaded.dat', + direct_file_path=None, gc=gc_mock), + ])]) + + docker_run_mock.assert_called_once() + kwargs = docker_run_mock.call_args[1] + first, second = kwargs['container_args'][0].split(',') + assert first == '%s/direct.dat' % TEMP_VOLUME_DIRECT_MOUNT_PREFIX + # the second file has no direct path, so it falls back to a download + assert second.endswith('/downloaded.dat') + assert not second.startswith(TEMP_VOLUME_DIRECT_MOUNT_PREFIX) + # one bind mount and the temporary volume + assert len(kwargs['volumes']) == 2 diff --git a/tests/test_multiple_input.py b/tests/test_multiple_input.py new file mode 100644 index 0000000..f9101ca --- /dev/null +++ b/tests/test_multiple_input.py @@ -0,0 +1,88 @@ +import functools +import io +import json +import os + +import cherrypy +import pytest +from girder.api import rest +from girder.models.item import Item +from girder.models.token import Token +from girder.models.upload import Upload + +from slicer_cli_web import docker_resource, rest_slicer_cli +from slicer_cli_web.models import CLIItem + + +@pytest.fixture +def seriesFiles(folder, admin, fsAssetstore): + files = [] + for idx in range(2): + sampleData = b'Sample %d' % idx + files.append(Upload().uploadFromFile( + obj=io.BytesIO(sampleData), size=len(sampleData), + name='Series%d.dcm' % idx, parentType='folder', parent=folder, + user=admin)) + yield files + + +@pytest.fixture +def multipleHandlerFunc(server, admin, folder): + # Make a request to allow there to be some lingering context to handle the + # testing outside of a request for the actual handler. + server.request('/system/version') + rest.setCurrentUser(admin) + cherrypy.request.params['token'] = Token().createToken(admin)['_id'] + + xmlpath = os.path.join(os.path.dirname(__file__), 'data', 'MultipleSpec.xml') + + girderCLIItem = Item().createItem('multi', admin, folder) + Item().setMetadata(girderCLIItem, dict( + slicerCLIType='task', type='python', image='dockerImage', + digest='dockerImage@sha256:abc', xml=open(xmlpath, 'rb').read())) + + resource = docker_resource.DockerResource('test') + item = CLIItem(girderCLIItem) + handlerFunc = rest_slicer_cli.genHandlerToRunDockerCLI(item) + yield functools.partial(handlerFunc, resource) + + +@pytest.mark.plugin('slicer_cli_web') +def test_multiple_input_fans_out(folder, seriesFiles, multipleHandlerFunc): + job = multipleHandlerFunc(params={ + 'inputSeries': '%s,%s' % (seriesFiles[0]['_id'], seriesFiles[1]['_id']), + 'outputImage_folder': str(folder['_id']), + 'outputImage': 'out.png', + }) + + # The job title uses the first file of the list as the primary input + assert job['title'] == 'Assembles A Series on Series0.dcm' + + kwargs = json.loads(job['kwargs']) + container_args = kwargs['container_args'] + # cli name, one argument covering the whole series, and the output path + assert len(container_args) == 3 + serialized = repr(container_args[1]) + assert str(seriesFiles[0]['_id']) in serialized + assert str(seriesFiles[1]['_id']) in serialized + + +@pytest.mark.plugin('slicer_cli_web') +def test_multiple_input_comma_in_name(folder, admin, fsAssetstore, multipleHandlerFunc): + # A comma is legal in a Girder file name but is also the delimiter of the + # joined container-path argument, so it must not survive into the mounted + # filename. + sampleData = b'Sample' + commaFile = Upload().uploadFromFile( + obj=io.BytesIO(sampleData), size=len(sampleData), name='Series,part.dcm', + parentType='folder', parent=folder, user=admin) + + job = multipleHandlerFunc(params={ + 'inputSeries': str(commaFile['_id']), + 'outputImage_folder': str(folder['_id']), + 'outputImage': 'out.png', + }) + + serialized = repr(json.loads(job['kwargs'])['container_args'][1]) + assert 'Series_part.dcm' in serialized + assert 'Series,part.dcm' not in serialized