Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Proper way to format a tuple parameter #613

Closed
ghisvail opened this issue Dec 29, 2022 · 8 comments · Fixed by #614
Closed

Proper way to format a tuple parameter #613

ghisvail opened this issue Dec 29, 2022 · 8 comments · Fixed by #614
Labels
question Further information is requested

Comments

@ghisvail
Copy link
Collaborator

ghisvail commented Dec 29, 2022

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 🙏

@ghisvail ghisvail added the question Further information is requested label Dec 29, 2022
@effigies
Copy link
Contributor

Looks like we have something for list, but not tuple:

if argstr.endswith("...") and isinstance(value, list):

@ghisvail
Copy link
Collaborator Author

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

@satra
Copy link
Contributor

satra commented Dec 30, 2022

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
Copy link
Collaborator Author

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
Copy link
Collaborator Author

ghisvail commented Dec 30, 2022

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
Copy link
Collaborator

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

@ghisvail
Copy link
Collaborator Author

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
Copy link
Collaborator Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants