Runtime test of command-line parsing

[Lestropie]

Branching from question in #2665; giving its own thread as there's a few points to discuss that will diverge some distance from the code changes currently proposed there.

If I read correctly, the fundamental request is the capability to flag to a command that it should terminate at completion of command-line parsing, presumably for the purpose of verification of validity of a pipeline before commencing execution of the pipeline as a whole.

1. In MRtrix3-land, that would essentially equate to aborting after app.Parser.parse_args() in Python or MR::App::parse() in C++.

2. Important to recognize that this would not guarantee that the command is capable of executing; only that there was not an error that was detectable at the point of command-line parsing. For instance we do not check for the existence of input / output images, since the user could be doing something like this and just checking the filesystem for a file corresponding to that string won't work. There would be no check that things like text files expected to contain numerical matrix data actually contain such.

3. The proposal in #2665 of "--dry-run" is potentially an issue in several ways:

   1. MRtrix3 uses single-dash rather than double-dash for options.
   2. For many commands there may be a reasonable interpretation of what an option called "dry run" would do that would be quite different to the proposed functionality.
   3. See point 4.ii. below.

4. Regarding how one might engage such a functionality, there are multiple possibilities:

   1. We already have "hidden" command-line options for exporting command usages that are not included in the command help pages in any format. For functionality such as that proposed, which would only be of utility to workflow management engine developers, I think it would be reasonable that it not be shown to users.
   2. Conversely, it seems fragile to me to use a command-line option to test parsing of command-line options. Theoretically such a command-line option could be inserted at any point; but what if its insertion were to be what broke the command-line parsing? This would also prevent differential handling of command-line parsing errors in the case where this test is being performed, as there is no guarantee that the fact that such a test is being performed will be detected.
   3. Flagging in the "configuration" as suggested (which could perhaps be interpreted in a couple of ways) might not be the best way, given that I'd generally consider this a per-invocation toggle, whereas the point of a "configuration" is to set parameters that are common to most if not all invocations.
   4. On first contemplation the most appealing solution to me is an environment variable.

Awaiting any further elaboration / clarification from @tclose & will be curious to hear opinions from other @MRtrix3/mrtrix3-devs.

[tclose]

Branching from question in #2665; giving its own thread as there's a few points to discuss that will diverge some distance from the code changes currently proposed there.

If I read correctly, the fundamental request is the capability to flag to a command that it should terminate at completion of command-line parsing, presumably for the purpose of verification of validity of a pipeline before commencing execution of the pipeline as a whole.

Yes, that is what I had in mind.

1. In MRtrix3-land, that would essentially equate to aborting after app.Parser.parse_args() in Python or MR::App::parse() in C++.

Yes, that would be sufficient

2. Important to recognize that this would not guarantee that the command is capable of executing; only that there was not an error that was detectable at the point of command-line parsing. For instance we do not check for the existence of input / output images, since the user could be doing something like this and just checking the filesystem for a file corresponding to that string won't work. There would be no check that things like text files expected to contain numerical matrix data actually contain such.

That would be fine for my use case. I could see it being useful for other use cases, but file format checking is to be handled in Pydra using my fileformats package in any case.

3. The proposal in WIP: Implement Pydra code-generators #2665 of "--dry-run" is potentially an issue in several ways:

   1. MRtrix3 uses single-dash rather than double-dash for options.
   2. For many commands there may be a reasonable interpretation of what an option called "dry run" would do that would be quite different to the proposed functionality.
   3. See point 4.ii. below.

4. Regarding how one might engage such a functionality, there are multiple possibilities:

   1. We already have "hidden" command-line options for exporting command usages that are not included in the command help pages in any format. For functionality such as that proposed, which would only be of utility to workflow management engine developers, I think it would be reasonable that it not be shown to users.
   2. Conversely, it seems fragile to me to use a command-line option to test parsing of command-line options. Theoretically such a command-line option could be inserted at any point; but what if its insertion were to be what broke the command-line parsing? This would also prevent differential handling of command-line parsing errors in the case where this test is being performed, as there is no guarantee that the fact that such a test is being performed will be detected.
   3. Flagging in the "configuration" as suggested (which could perhaps be interpreted in a couple of ways) might not be the best way, given that I'd generally consider this a per-invocation toggle, whereas the point of a "configuration" is to set parameters that are common to most if not all invocations.
   4. On first contemplation the most appealing solution to me is an environment variable.

I'm not fussed on the mechanism to trigger the behaviour. It just needs to be used during CI, so an environment variable would probably be more convenient.

[jdtournier]

Couple of clarifications:

Important to recognize that this would not guarantee that the command is capable of executing; only that there was not an error that was detectable at the point of command-line parsing. For instance we do not check for the existence of input / output images, since the user could be doing something like this and just checking the filesystem for a file corresponding to that string won't work. There would be no check that things like text files expected to contain numerical matrix data actually contain such.

It's actually slightly worse than that. The MR::App::parse() function only populates the relevant structures that hold the arguments & options ready for use by the rest of the code, and performs the minimum amount of checks required to do that. It does not guarantee that these are actually used in the code, and doesn't check for conflicts that may arise in actual use -- these are all delegated to the code performing the processing. For example, there are commands that will accept a number of options, but some of these may be mutually exclusive, depending on usage and what other options may have been provided; this won't be checked in MR::App::parse(). There are commands that accept a variable number of arguments, but in practice some of these should come in pairs (e.g. dwi2fod msmt_csd); again, this won't be checked in MR::App::parse(). There are all sorts of other situations where MR::App::parse() won't flag any issues, but the command will abort later for reasons that could very reasonably be expected to flag an error with a --dry-run flag...

MRtrix3 uses single-dash rather than double-dash for options.

MRtrix3 uses any number of dashes. We don't distinguish between -dry-run or --dry-run.

---

Nonetheless, assuming we do want to have some means to running through MR:App::parse() and reporting any errors encountered at that point, the simplest would indeed be to use an environment variable, though I would personally prefer to use a hidden option - an environment variable is most useful when it's intended to apply to a session, rather than a single command invocation. The issue at this point is that the hidden 'options' that @Lestropie was referring to aren't really options, but regular arguments, which are furthermore expected to be provided as the first and only argument (see relevant code here in app.cpp). But we could add the following ahead of line 1037:

if (strcmp (argv[1], "__check_usage__") == 0) {
        __check_usage_and_exit = true;
        ++argv;
        --argc;
        return;
      }

which would detect when __check_usage__ is provided as the first argument, skip it from the subsequent MR::App::parse(), and set some variable to skip run() after parsing. So you could run your checks by inserting __check_usage__ as the first argument, would that work for you?

We could then insert a quick check in core/command.h at line 101:

    ...
    ::MR::App::parse ();
    if (!__check_usage_and_exit)
      run ();
    ...

We'd need to declare extern bool __check_usage_and_exit in core/app.h, etc., but broadly, that should do it?

[tclose]

Thanks @jdtournier, that all sounds good to me

[tclose]

I can have a go at including it in my PR if that would be appropriate

[Lestropie]

It does not guarantee that these are actually used in the code

Correct; feature request is #1975.

some of these may be mutually exclusive

The Python CLI has some limited flagging of option mutual exclusivity that operates at the point of command-line parsing. Would be possible to do similar in C++, but doesn't currently exist.

an environment variable is most useful when it's intended to apply to a session

But if it's going to be used exclusively during CI, that seems to suit? It feels more tailored / less hacky to me than doing increments / decrements on argc / argv.

the hidden 'options' that @Lestropie was referring to aren't really options, but regular arguments, which are furthermore expected to be provided as the first and only argument

OK, you got me. "Hidden usages"?

I can have a go at including it in my PR if that would be appropriate

I basically always advocate for independent PRs rather than concatenating commits, even if I don't always obey it myself. Can use a feature branch as the destination branch of the PR merge if it makes more sense. It just protects against conversations crossing over one another.

[jdtournier]

an environment variable is most useful when it's intended to apply to a session

But if it's going to be used exclusively during CI, that seems to suit? It feels more tailored / less hacky to me than doing increments / decrements on argc / argv.

Good point. It'll depend on how that integrates into the CI. If it's relying on the ./run_tests approach, there's no functionality to set environment variables. It would make sense when running an independent GitHub Action though.

But what I meant here is that an additional argument is much more visible than a environment variable. If someone accidentally left it set earlier in the session, it could cause a fair bit of confusion down the track when trying to actually run something. Most of our environment variables affect some aspect of how the commands run, but this one would prevent them from running altogether, which is why I'd prefer an argument / option. Not a massive deal either way, to be fair...

the hidden 'options' that @Lestropie was referring to aren't really options, but regular arguments, which are furthermore expected to be provided as the first and only argument

OK, you got me. "Hidden usages"?

:rofl: OK, that might have come across as a touch pedantic... Reason I mention is because I initially started thinking of solutions based on the idea that these were options, and it took me a while to twig that they weren't. :man_shrugging:

[tclose]

an environment variable is most useful when it's intended to apply to a session

But if it's going to be used exclusively during CI, that seems to suit? It feels more tailored / less hacky to me than doing increments / decrements on argc / argv.

Good point. It'll depend on how that integrates into the CI. If it's relying on the ./run_tests approach, there's no functionality to set environment variables. It would make sense when running an independent GitHub Action though.

The plan is to put it in a GH action so env var would work, and would actually be easier.

It could also be nice to have the test there to run "live" (non dry run) tests. Although how to handle the test data in that case might be tricky.

[tclose]

The other option is to just use the "live" commands but stop them after a shortish timeout (i.e. no "dry-run" option/env-var required). The only issue with this is that we then need to provide real test data, which I have some ideas around how to automate, but would require detailed specification of the input file-types along the lines of what I was requesting in #2605

[Lestropie]

OK, that might have come across as a touch pedantic...

You know me, always hyper-focusing on other people being pedantic about words... :roll_eyes:

The other option is to just use the "live" commands but stop them after a shortish timeout (i.e. no "dry-run" option/env-var required).

I'd thought about this a little when doing the python API. This is really a limitation with the usage() / run() (usage() / execute() in Python) two-function designation. What you ultimately need is three functions: one to define the command-line interface, one to do basic data import and sanity / compatibility checking, and one to do the heavy lifting, finally with a way to terminate after two out of three. But that's a heftier refactor, and also beginning to stray from issue scope if you want to explore it.