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

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

Merged
merged 3 commits into from
Mar 14, 2023

Conversation

ghisvail
Copy link
Collaborator

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

  • I have added tests to cover my changes (if necessary)
  • I have updated documentation (if necessary)

Closes #619

@codecov
Copy link

codecov bot commented Mar 13, 2023

Codecov Report

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

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

❗ 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
@@            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%) ⬆️

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

Impacted Files Coverage Δ
pydra/engine/helpers_file.py 86.10% <ø> (+7.25%) ⬆️
pydra/utils/profiler.py 45.80% <ø> (+7.63%) ⬆️

... and 13 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.

Copy link
Contributor

@effigies effigies left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense to me. We should get a different failure if requires are missing and the field is mandatory.

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.

pydra/engine/helpers_file.py Outdated Show resolved Hide resolved
@ghisvail
Copy link
Collaborator Author

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.

@ghisvail ghisvail merged commit 11a6d68 into nipype:master Mar 14, 2023
@ghisvail ghisvail deleted the fix/issue-619 branch March 14, 2023 09:24
@ghisvail ghisvail restored the fix/issue-619 branch March 14, 2023 12:46
@ghisvail ghisvail deleted the fix/issue-619 branch March 14, 2023 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Combining requires and output_file_template for a field
2 participants