FIX: Allow any iterable for ShellOutSpec requires

ghisvail commented 1 year ago

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[bot] commented 1 year ago

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.05 :warning:

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

:exclamation: 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

```diff @@ 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%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#carryforward-flags-in-the-pull-request-comment) to find out more. | [Impacted Files](https://codecov.io/gh/nipype/pydra/pull/631?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [pydra/engine/specs.py](https://codecov.io/gh/nipype/pydra/pull/631?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cHlkcmEvZW5naW5lL3NwZWNzLnB5) | `94.76% <100.00%> (+0.01%)` | :arrow_up: | ... and [4 files with indirect coverage changes](https://codecov.io/gh/nipype/pydra/pull/631/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

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

djarecka commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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")

nipype / pydra