Support multiple files for a single Girder-resource input

README.rst

@@ -81,6 +81,8 @@ The XML must conform to the `Slicer Execution Schema <https://www.slicer.org/w/i
   ``multi`` allow multiple shapes, indicated by separating coordinates of each shape by -1,-1.  Note that neither -1,-1 nor -2,-2 are allowed as coordinates within a shape -- to use those, specify them with decimals (e.g., -1.0,-1.0).
   The submit options will add suggestions on how the UI should handle changes.  If present, the option to auto-run a job as soon as a valid shape is set should be present.  ``autosubmit`` means this should always happen.  ``submit`` or ``submitoff`` offers this as a setting but is default to not submit the job.  ``submiton`` offers this as a setting and defaults to submitting the job.
 
+- Girder-resource input types (``image``, ``file``, ``item``, ``directory``) can have a ``multiple`` property.  If ``true``, the parameter accepts a comma-separated list of Girder IDs.  Each resource is bound into the container separately (using direct read-only mounts when possible) and the CLI receives a single comma-separated list of container paths, in the submitted order.  Inputs with ``multiple`` set are not available for batch processing.
+
 - Some input types (``image``, ``file``, ``item``, ``directory``) can have ``defaultNameMatch``, ``defaultPathMatch``, and ``defaultRelativePath`` properties.  The first two are regular expressions designed to give a UI a value to match to prepopulate default values from files or paths that match the regex.  ``defaultNameMatch`` is intended to match the final path element, whereas ``defaultPathMatch`` is used on the entire path as a combined string.  ``defaultRelativePath`` is used to find a value that has a path relative to some base.  In the Girder UI, this might be from an item.
 
 - Input types can have a ``datalist`` property.  If this is present, when the CLI is first loaded or, possibly periodically after parameters have been changed, the CLI may be called with optional parameters.  The CLI is expected to return a new-line separated list of values that can be used as recommended inputs.  As an example, a ``string`` input might have a ``datalist`` of ``--enumerate-options``; the cli would be called with the existing parameters PLUS the extra parameter specified by ``datalist``.  If the result is sensible, the input control would expose this list to the user.  The ``datalist`` property is a json-encoded dictionary that overrides other parameters.  This should override parameters that aren't needed to be resolved to produce the datalist (e.g., input and output files) as that will speed up the call.  The CLI should respond to the modified call with a response that contains multiple ``<element>some text</element>`` values that will be the suggested data for the control.

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)

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,18 +92,29 @@ 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')
 
     param_id = param.identifier()
     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'])

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)

tests/data/MultipleSpec.xml

@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<executable>
+  <category>Test</category>
+  <title>Assembles A Series</title>
+  <description>Consumes a multi-file series and produces a single output image.</description>
+  <version>0.1.0</version>
+  <parameters>
+    <label>IO</label>
+    <description>Input/output parameters.</description>
+    <image multiple="true">
+      <name>inputSeries</name>
+      <label>Input Series</label>
+      <channel>input</channel>
+      <index>0</index>
+      <description>Files of the input series</description>
+    </image>
+    <image>
+      <name>outputImage</name>
+      <label>Output Image</label>
+      <channel>output</channel>
+      <index>1</index>
+      <description>Assembled output image</description>
+    </image>
+  </parameters>
+</executable>

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

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