FIX: Allow any iterable for ShellOutSpec requires

[ghisvail]

Types of changes

• Bug fix (non-breaking change which fixes an issue)

Summary

Specifying the requires metadata for a field in ShellOutSpec as a set instead of a list bypasses part of the metadata checker resulting in an Exception further down the implementation.

This fix allow to specify the requires metadata for ShellOutSpec as any iterable (including a set), instead of being restricted to a list. It's then explicitly transformed to a list of list by the metadata checker, if necessary.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.05 ⚠️

Comparison is base (11a6d68) 81.75% compared to head (97faad1) 81.71%.

❗ Current head 97faad1 differs from pull request most recent head d061b35. Consider uploading reports for the commit d061b35 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #631      +/-   ##
==========================================
- Coverage   81.75%   81.71%   -0.05%     
==========================================
  Files          20       20              
  Lines        4390     4391       +1     
  Branches     1263        0    -1263     
==========================================
- Hits         3589     3588       -1     
- Misses        797      803       +6     
+ Partials        4        0       -4     

Flag	Coverage Δ
unittests	81.71% <100.00%> (-0.05%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pydra/engine/specs.py	94.76% <100.00%> (+0.01%)	⬆️

... and 4 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[effigies]

I'm a little confused by this one. The type checking is divorced from the semantics. Why should this be a list of lists of tuples or strings? It's hard to reason about the effects of converting an arbitrary iterable to a list when the meaning of the tuples and strings (both iterables) are opaque.

Instead of having this here in this function, I'd rather have a separate function:

def canonicalize_requires(requires):
    """Canonicalize a requirement to a nested list of lists of requirements

    Fully specified requirements are a list of lists of strings or tuples that have the form
    ``[["abc", "def"], ...]`` or ``[[("abc", "def"), ("ghi", "jkl")], ...]``. For brevity, we permit the following
    forms.

    >>> # some example code

    Non-list iterables are also accepted:

    >>> canonicalize_requirements({"abc", "def"})
    [["abc", "def"]]
    """
    ...

[ghisvail]

I did not want to change the semantics too much in this PR so I did not attempt to refactor the code. The code flow could be improved.

The idea remains that I should be able to do:

metadata={
    ...,
    "requires": {"foo", "bar"},
}

or

metadata={
    ...,
    "requires": ["foo", "bar"],
}

and the most straightfoward way was to normalize Iterable to list internally first.

[effigies]

If you want to keep refactoring/modifying semantics for a separate PR, that's fine by me.

[djarecka]

I just want add that I slightly distinguish between lists and tuples, at least for the inner element of the main list. My idea was:

• you could have OR condition when you have a list of lists, see here
• you can specify a value for an input if you have a tuple in the list, see here)

I'm not saying that we can't change the main container to a set, but it would be good to keep this logic

[ghisvail]

Thanks for clarifying @djarecka

The goal is to keep the semantics you listed, whilst allowing "requires": "foo" for the most simple case, i.e. optional output guarded by a flag.

May I ask why the choice went for OR rather than AND? I would have thought, perhaps naively, that requirements more often compose by the latter than the former?

[djarecka]

AND is covered by default when you have multiple elements, e.g. here.

So if you have:

"requires": [
     ["file1", "additional_inp_A"],
     ["file1", "additional_inp_B"],
  ],

That means ("file1" AND "additional_inp_A") OR ("file1" AND "additional_inp_B")