FIX: Provide templated fields to cmdline only when requirements are met

ghisvail commented 1 year ago

Types of changes

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

Summary

Issue #619 showed an example where a templated field can be provided in the generated command-line string, despite being guarded by another boolean option via requires. This PR ensures that the helper function responsible for computing interpolated fields only consider templated fields for which all their required fields are set.

Checklist

[x] I have added tests to cover my changes (if necessary)
[ ] I have updated documentation (if necessary)

Closes #619

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: +5.12 :tada:

Comparison is base (32432c3) 76.58% compared to head (3ef89b2) 81.70%.

:exclamation: Current head 3ef89b2 differs from pull request most recent head 2ef6d4f. Consider uploading reports for the commit 2ef6d4f to get more accurate results

Additional details and impacted files

```diff @@ Coverage Diff @@ ## master #629 +/- ## ========================================== + Coverage 76.58% 81.70% +5.12% ========================================== Files 20 20 Lines 4390 4390 Branches 1263 0 -1263 ========================================== + Hits 3362 3587 +225 + Misses 830 803 -27 + Partials 198 0 -198 ``` | Flag | Coverage Δ | | |---|---|---| | unittests | `81.70% <ø> (+5.17%)` | :arrow_up: | 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/629?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [pydra/engine/helpers\_file.py](https://codecov.io/gh/nipype/pydra/pull/629?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cHlkcmEvZW5naW5lL2hlbHBlcnNfZmlsZS5weQ==) | `86.10% <ø> (+7.25%)` | :arrow_up: | | [pydra/utils/profiler.py](https://codecov.io/gh/nipype/pydra/pull/629?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-cHlkcmEvdXRpbHMvcHJvZmlsZXIucHk=) | `45.80% <ø> (+7.63%)` | :arrow_up: | ... and [13 files with indirect coverage changes](https://codecov.io/gh/nipype/pydra/pull/629/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.

ghisvail commented 1 year ago

We should get a different failure if requires are missing and the field is mandatory.

I think there is a wider discussion to be had on requires (and maybe xor). For instance, FSL's Eddy features requirements for some options based on a minimum value on a dependent field. Kind of like a feature flag followed by feature specific flags (here it's an optional processing step enabled when the model order is higher than 1).

I have a feeling a more general form of requires could be:

requires: mapping[str, Callable[T, bool]] | T | None] | iterable[str] | None

where the current form iterable[str] stands for all fields are defined, and the more general form mapping[...] means all fields are defined and satisfy a runtime check against a constant or a callable, if defined.

Is there any chance of documenting this? It would be nice to have a cookbook of specs showing how to achieve goals like conditional command line templates.

I agree that a cookbook of specs would be welcome. We have a few ones in the test suite but they are pretty abstract. I think once the model for requires (and maybe xor) is well specified, I would feel more confident in working on a cookbook-like extension to the docs.

nipype / pydra