Python Command Line Changes

[ankitasanil]

These changes are relevant to the issue #1392 (MRtrix3#1392) and include below modifications:

• Creation of custom callable classes for handling different datatypes for positional and optional arguments on the command line. Used ArgumentTypeError for exception management instead of ValueError since the former allows allows adding custom error messages.
• Addition of newly defined callables (in app.py) for each positional and optional command line argument of mrtrix_cleanup and dwicat

@Lestropie - Please review these changes

[Lestropie]

Is the use syntax definitely "type=app.Parser().TypeInputImage()" as opposed to "type=app.Parser.TypeInputImage"? I'd have expected the latter, in that it would be defining the type of the callable rather than an instance thereof.

[Lestropie]

For a number of output types, check:
https://github.com/MRtrix3/mrtrix3/blob/3.0.4/core/app.cpp#L1181-L1235
From memory, when the -force option is used, there's a global "output files will be overwritten", but I think that in some instances there can be individual error messages of "this specific file / directory already exists, and will be overwritten at command completion"

[tclose]

You mean a warning message needs to be added? The value of --force isn't going to be accessible to the output directory type is it?

[Lestropie]

1. Sorry yes, individual warning messages for individual items, not errors.
2. In some cases we do issue an error; specifically attempting to overwrite a pre-existing fixel directory using -force. Can lead to unexpected behaviour, so we forbid it.
3. Regarding knowledge of the force-overwrite status, this I don't know. It depends on the internals of argparse. But normally I'd read from app.FORCE_OVERWRITE in that case, which only gets set after argparse.parse_args() is finished, which means it probably will never be set when those callables are invoked.
   Better, which bears some relation to ENH Warn on unused options MRtrix3/mrtrix3#1975, would be for the callable to yield some class for which a pre-existing file at that path could be checked at the first instance at which the executing script reads from that path.
4. Pretty sure that the situation where individual warning messages for overwriting individual files is specific to the Python scripts and doesn't happen in the C++ binaries. So if that no longer happens as a natural consequence of these changes, it's not the end of the world, especially given that up to this point it had to be hard-coded on a per-case basis.

[tclose]

Yeah, you won't be able to get it to work from inside the type callable. You would need to add some code to the app I think to loop through all types and find the file/directory types

[Lestropie]

Preferable for class names to echo those used in C++ code:
https://github.com/MRtrix3/mrtrix3/blob/3.0.4/core/cmdline_option.h#L44-L62

[Lestropie]

While in cd33930 for population_template I've made modifications to the loading of data from command-line options, these are not tested as part of CI. We would need to augment the script test data with multi-contrast data from multiple subjects, and add tests that use all possible command-line options.

[Lestropie]

From a few different sources, it looks like custom Actions should derive from argparse.Action.

[tclose]

Is the use syntax definitely "type=app.Parser().TypeInputImage()" as opposed to "type=app.Parser.TypeInputImage"? I'd have expected the latter, in that it would be defining the type of the callable rather than an instance thereof.

Looks ok to me. app.Parser.Type* need to be callable objects so they can return an object of different type, i.e. a bool instead of app.Parser.Boolean

[tclose]

@Lestropie , just trying to work out what is necessary to finish off this PR. Is it just a few stylistic changes you want first?

[tclose]

Preferable for class names to echo those used in C++ code: https://github.com/MRtrix3/mrtrix3/blob/3.0.4/core/cmdline_option.h#L44-L62

Could they even mirror them more closely by using snake_case, e.g. type_image_in() (would still be a class under the hood but no one needs to know that), or perhaps just image_in() since it is going to be set to type, i.e. type=image_in()

[tclose]

From a few different sources, it looks like custom Actions should derive from argparse.Action.

which are the custom Actions?

[Lestropie]

what is necessary to finish off this PR

• Hoped for confirmation from @ankitasanil regarding whether she wanted to contribute to finishing the feature set.
  (If not, will pull this branch into the main repo and make a PR there)
• Anything outstanding in run.command(): Support for command-line piping MRtrix3/mrtrix3#1392, particularly piping of image data given that was the primary goal
• Contemplation of class naming / placement. Been a while since I looked at / contemplated it, but I would probably opt for something that complies with convention / pylint enforcement for class names but has word ordering consistent with the corresponding C++ modifier functions.

which are the custom Actions?

This might be a red herring on my part. I may have temporarily mixed up types and actions. Though it also depends on the most appropriate way to handle image piping.

[tclose]

what is necessary to finish off this PR

• Hoped for confirmation from @ankitasanil regarding whether she wanted to contribute to finishing the feature set.
  (If not, will pull this branch into the main repo and make a PR there)
• Anything outstanding in run.command(): Support for command-line piping MRtrix3/mrtrix3#1392, particularly piping of image data given that was the primary goal

Would it be possible to split them up if there are other issues in MRtrix3#1392 that prove tricky?

• Contemplation of class naming / placement. Been a while since I looked at / contemplated it, but I would probably opt for something that complies with convention / pylint enforcement for class names but has word ordering consistent with the corresponding C++ modifier functions.

Personally I find pylint pretty annoying (flake8 is more reasonable IMO), and in such cases where the user doesn't need to know whether it is a class or function there isn't much benefit trying to stick to the convention.

which are the custom Actions?

This might be a red herring on my part. I may have temporarily mixed up types and actions. Though it also depends on the most appropriate way to handle image piping.

Yeah, I think actions are for different use cases

[Lestropie]

Would it be possible to split them up if there are other issues in MRtrix3#1392 that prove tricky?

Definitely don't want to skip over the piping; appreciate that in your scenario you're invested in typed arguments, but from our perspective that was actually just a means to an end, so stopping short of that mark would be kind of unusual. And wouldn't want to split for the sake of having typed arguments for Pydra if it then turned out that we need to make further backward-incompatible back-end changes to facilitate the piping.

The other stuff should not be terribly difficult. Using argparse.FileType I'm happy to skip, but we do need basic sanity check tests in place, and that should be easy as we shouldn't need to modify the test data repo.

[tclose]

Would it be possible to split them up if there are other issues in MRtrix3#1392 that prove tricky?

Definitely don't want to skip over the piping; appreciate that in your scenario you're invested in typed arguments, but from our perspective that was actually just a means to an end, so stopping short of that mark would be kind of unusual. And wouldn't want to split for the sake of having typed arguments for Pydra if it then turned out that we need to make further backward-incompatible back-end changes to facilitate the piping.

The other stuff should not be terribly difficult. Using argparse.FileType I'm happy to skip, but we do need basic sanity check tests in place, and that should be easy as we shouldn't need to modify the test data repo.

ok, fair enough. Let me know if there is anything I can do to push it along

[Lestropie]

Given you're active and eager, I'll pull these commits over to a branch on the main repo, try to make the primary up-front changes I'd want to make, and then we can see what's left from there.