nipype / pydra

Pydra Dataflow Engine
https://nipype.github.io/pydra/
Other
120 stars 59 forks source link

Proper way to format a tuple parameter #613

Closed ghisvail closed 1 year ago

ghisvail commented 1 year ago

I have got an interface which expects a parameter foo as a triple with the following format --foo 1 2 3

I reached for the following definition as a first attempt:

input_spec = pydra.specs.SpecInfo(
    name="Input",
    fields=[
        (
            "foo",
            ty.Tuple[int, int, int],
            {
                "help_string": "foo triple",
                "argstr": "--foo {foo}",
            },
        ),
    ],
    bases=(pydra.specs.ShellSpec,),

Which does not work since it produces --foo (1, 2, 3).

If I use --foo... and instantiate the interface with a list instead of tuple then I get --foo 1 --foo 2 --foo 3.

If I use formatter instead of argstr with "formatter": lambda field: f"--foo {' '.join(field)}", I get an error because formatter seem to be called with each element of the list instead of the entire list (TypeError: sequence item 0: expected str instance, int found).

I am probably missing another option, hence this support request 🙏

effigies commented 1 year ago

Looks like we have something for list, but not tuple: https://github.com/nipype/pydra/blob/66919ab54a985733d99b891a24ba605f12aa3b7d/pydra/engine/task.py#L461

ghisvail commented 1 year ago

Is there a way to turn foo=[1, 2, 3] to --foo 1 2 3 with list today?

satra commented 1 year ago
In [8]: input_spec = pydra.specs.SpecInfo(
   ...:     name="Input",
   ...:     fields=[
   ...:         (
   ...:             "foo",
   ...:             ty.List[int],
   ...:             {
   ...:                 "help_string": "boolean option",
   ...:                 "mandatory": True,
   ...:                 "argstr": "--foo",
   ...:             },
   ...:         ),
   ...:     ],
   ...:     bases=(pydra.specs.ShellSpec,),
   ...: )

In [9]: class MyInterface(pydra.ShellCommandTask):
   ...:     input_spec = input_spec
   ...:     executable = "ls"
   ...: 

In [10]: MyInterface(foo=[1,2,3]).cmdline
Out[10]: 'ls --foo 1 2 3'

you can also add sep to metadata to create a custom separator. for example adding sep=':' would generate

In [16]: MyInterface(foo=[1,2,3]).cmdline
Out[16]: 'ls --foo 1:2:3'

and adding ellipsis argstr='--foo...' would generate:

In [22]: MyInterface(foo=[1,2,3]).cmdline
Out[22]: 'ls --foo 1 --foo 2 --foo 3'
ghisvail commented 1 year ago

Thanks @satra for providing a solution.

I would have never guessed that using the same "agstr" as for a boolean option would produce the pattern I was looking for with a list.

Now could we widen this to other iterable types like tuples or sets? In my case, I'd want to make it explicit that foo is a 3-element iterable, not any potentially larger or smaller sized list.

ghisvail commented 1 year ago

Now could we widen this to other iterable types like tuples or sets?

Actually, regardless of this particular issue, I don't find very intuitive that:

>>> MyInterface(foo=[1, 2, 3]).cmdline
'--foo 1 2 3'
>>> MyInterface(foo=(1, 2, 3)).cmdline
'--foo (1, 2, 3)'
>>> MyInterface(foo={1, 2, 3}).cmdline
'--foo {1, 2, 3}'
djarecka commented 1 year ago

and what you would like to see for tuples and dictionaries? The same command as for a list?

ghisvail commented 1 year ago

and what you would like to see for tuples and dictionaries?

I'd expect the same behaviour between lists, tuples and sets since they are all collections.

ghisvail commented 1 year ago

I tried to provide an enhancement proposal with a test showcasing that formatting now works with other popular iterable containers such as tuples in #614.

I did not try to refactor this part of the code which looks a bit scary to the novice contributor I consider myself. One thing that surprises me is that we are not leveraging the type information that much (apart for the bool case) and rely on the value of the field for handling each case.