Make --prepend-text and --append-text options properly interactive

[sphuber]

Fixes #4300

The --prepend-text and --append-text options, used for both the
verdi computer setup and verdi code setup commands were normal
options as opposed to all other options that are interactive options.
The InteractiveOption is a custom option class type we developed that
will present the user with a prompt if it was not explicitly provided on
the command line. The --non-interactive flag can be used by the user
to prevent prompting, as long as a default value is defined.

The two options in question were implemented differently, presumably
because they take a potentially multiline string, which is not easily
defined on a prompt and therefore instead these options would be defined
through a text editor. The prompting of this text editor, if necessary,
was however not performed in the option itself, but in the command body.
At this point, the prompt cycle of the parameter cycle controlled by
click is already over.

Additionally, since the function checking whether the option had been
defined also considered an empty string as undefined, despite it being
the default, a specified empty string would still lead to the user to
be prompted, even when specifying --non-interactive.

The fix is to make both options proper interactive options just like the
rest. To this end, we create the TemplateInteractiveOption that works
just like the InteractiveOption with the only difference being that it
uses a file editor instead of an inline prompt. This change does force
us to have both options pop up their own file editor, whereas before
they were joined in a single file and both files were specified in the
same file, separated by a header that we defined.

[sphuber]

Even though this fixes the problem described in the issue, I still think there potentially is some counter-intuitive behavior. With this change the --prepend-text and --append-text options no longer have to be specified explicitly (even in interactive mode). The command will simply use the defaults. However, these two options are the exception. All other options, such as the --shebang option, have to be specified (unless --non-interactive is specified) despite them having a default. I am not sure if this was the original intention of the --non-interactive flag but I feel it could be a bit counter-intuitive, even more so given that the rule does not apply unilaterally to all options.

[codecov]

Codecov Report

Merging #4318 into develop will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop    #4318      +/-   ##
===========================================
+ Coverage    79.08%   79.09%   +0.02%     
===========================================
  Files          468      468              
  Lines        34642    34608      -34     
===========================================
- Hits         27394    27371      -23     
+ Misses        7248     7237      -11     

Flag	Coverage Δ
#django	72.71% <100.00%> (+0.01%)	⬆️
#sqlalchemy	71.90% <100.00%> (+0.01%)	⬆️

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

Impacted Files	Coverage Δ
aiida/cmdline/commands/cmd_code.py	89.64% <100.00%> (+1.04%)	⬆️
aiida/cmdline/commands/cmd_computer.py	81.20% <100.00%> (+0.33%)	⬆️
aiida/cmdline/params/options/commands/code.py	100.00% <100.00%> (ø)
aiida/cmdline/params/options/commands/computer.py	86.21% <100.00%> (+1.03%)	⬆️
aiida/cmdline/params/options/interactive.py	84.85% <100.00%> (+1.38%)	⬆️
aiida/cmdline/utils/multi_line_input.py	95.46% <100.00%> (+14.97%)	⬆️
aiida/cmdline/params/options/contextualdefault.py	50.00% <0.00%> (-40.00%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cb273e7...ab639e6. Read the comment docs.

[ltalirz]

Thanks @sphuber

I just checked the behavior on develop using this yml file:

label: "daint-mc23"
hostname: "daint.cscs.ch"
description: Piz Daint supercomputer at CSCS Lugano, Switzerland, multicore partition.
transport: ssh
scheduler: "slurm"
work_dir: "/scratch/snx3000/{username}/aiida_run/"
shebang: "#!/bin/bash"
mpirun_command: "srun -n {tot_num_mpiprocs}"
mpiprocs_per_machine: "36"
prepend_text: |
    ### computer prepend_text start ###
    #SBATCH --partition=normal
    #SBATCH --account=mr0
    #SBATCH --constraint=mc
    #SBATCH --cpus-per-task=1
    export OMP_NUM_THREADS=$SLURM_CPUS_PER_TASK
    source $MODULESHOME/init/bash
    module load daint-mc
    ulimit -s unlimited
    ### computer prepend_text end ###
append_text: ""

I find that

1. verdi computer setup --config comp.yml -n actually works without prompting/complaining, which is the intended behavior (note that it is not clear from your commit message that the non-interactive mode works fine, please clarify)
2. verdi computer setup --config comp.yml will prompt for the scripts. It's true that this is not ideal since the value for both scripts is specified by the user (but I don't consider it a big deal)

After checking out your changes, 2. works without prompting as well, but as you mention, it disables prompting for the pre/postpend scripts altogether (which is not what we want).

I see two ways to resolve this:
A. Find a way to detect whether a value is specified by the user or comes from the click default (better, but not sure whether it is possible)
B. Set a non-empty string as the default value for the pre-pend and post-pend scripts (could be a one-liner comment placeholder)

P.S.

The problem was with in the utility function multi_lin_input.ensure_scripts which would compare the
prepend_text and append_text against None

I think you meant None or '' here

[sphuber]

After checking out your changes, 2. works without prompting as well, but as you mention, it disables prompting for the pre/postpend scripts altogether (which is not what we want).

I forgot about this and agree, this is definitely not what we want. Before trying to continue to add patches, I think we first need to make absolutely sure how we want the commands with the --config and --non-interactive options to behave. Because I think, aside from the prepend_text and append_text, other options are working as expected. To sketch out the scenarios we have, I created the following example command:

#!/usr/bin/env python
import click

from aiida.cmdline.params import options
from aiida.cmdline.params.options.interactive import InteractiveOption


@click.command()
@click.option('--label', prompt='Label', cls=InteractiveOption, required=True)
@click.option('--hostname', prompt='Hostname', cls=InteractiveOption, default='localhost')
@options.NON_INTERACTIVE()
@options.CONFIG_FILE()
def command(label, hostname, non_interactive):
    print(label, hostname)


if __name__ == '__main__':
    command()

As you can see, it has two options, both required, but one with a default and one without. In addition, there are the --config and --non-interactive options to allow specifying from config file and to prevent all prompting. Then I create two config files, config-complete.yml:

label: 'daint'
hostname: daint.cscs.ch

and config-partial.yml:

label: 'daint'

We now have the following (meaningful) permutations of possible commands to use:

Command	Result
./test_cli.py --label daint --hostname daint.cscs.ch	Succeeds: {daint, daint.cscs.ch}
./test_cli.py --label daint	Prompts: for hostname
./test_cli.py --label daint -n	Succeeds: {daint, localhost}
./test_cli.py -n	Errors: Missing option '--label'.
./test_cli.py --config config-complete.yml	Succeeds: {daint, daint.cscs.ch}
./test_cli.py --config config-partial.yml	Prompts: for hostname
./test_cli.py --config config-partial.yml -n	Succeeds: {daint, localhost}
./test_cli.py --config config-partial.yml --hostname daint.cscs.ch	Succeeds: {daint, daint.cscs.ch}

To me, the results are actually what I would expect, as long as the --non-interactive options is defined as follows:

Use the --non-interactive flag if the command should never prompt and simply use the default if defined. Note that the command will fail if any default-less option is not specified explicitly.

If we agree on this definition and the current behavior, then the current problem is simply with the implementation of the prepend_text and append_text. They, unlike all other options, are not of type InteractiveOption. Instead, their value is checked inside the command and a separate control loop is used to prompt for values. I think the fix would be to adapt these options to be individual prompters, just like the others. The default can then still remain the empty string and it would simply be accepted as an actual value.

Let me know if you agree with this assessment @ltalirz and I will update this PR.

[ltalirz]

Thanks @sphuber for the detailed writeup - I agree with the description of the desired behavior entirely (perhaps we could put it somewhere into the documentation, e.g. starting a "command line interface" section under "internal architecture).

If that behavior can be attained, then great!

[yakutovicha]

After checking out your changes, 2. works without prompting as well, but as you mention, it disables prompting for the pre/postpend scripts altogether (which is not what we want).

@ltalirz, maybe I misunderstood something, but I believe this is exactly what we want when in the yml file one provides both prepend and append text. Or do you mean that user won't be asked to provide prepend/append text even if they are absent in the yml file?

[ltalirz]

Or do you mean that user won't be asked to provide prepend/append text even if they are absent in the yml file?

Correct, with the current code, the user would never be prompted for append/prepend texts.

[yakutovicha]

Alright, thanks indeed @sphuber for the detailed explanation. I completely agree with your proposed solution.

[sphuber]

@ltalirz and @yakutovicha I have implemented the discussed changes and this should be good to go for a new review

[ltalirz]

thanks @sphuber - I think this is exactly the behavior we want, and the code has become cleaner as well.

I noticed a minor issue, then it should be good to merge.

[ltalirz]

P.S. As mentioned, you may want to consider adding what you wrote here somewhere into the documentation, e.g. starting a "command line interface" section under "internal architecture".

[sphuber]

Thanks for the review, I will address the changes.

P.S. As mentioned, you may want to consider adding what you wrote here somewhere into the documentation, e.g. starting a "command line interface" section under "internal architecture".

I know that is what I suggested, but really that was just to hash out where exactly the error in the code was. I don't see what information is there that is still worth having in terms of describing of how things should work. The difference between defining parameters inline or via config file ultimately had nothing to do with this issue. The only thing that we could describe is the intended working of the --non-interactive option:

Use the --non-interactive flag if the command should never prompt and simply use the default if defined. Note that the command will fail if any default-less option is not specified explicitly.

but that is more for the user manual than the internal workings. What exactly do you think would have to go in the internal architecture?

[ltalirz]

If you think it's self-explanatory enough, perhaps it's indeed not needed to add to the documentation.
It would just have been an illustration of how the prompting mechanism is supposed to work and what the --non-interactive option does.

Use the --non-interactive flag if the command should never prompt and simply use the default if defined. Note that the command will fail if any default-less option is not specified explicitly.

Yes, perhaps we can just expand the help string of the option a bit

aiida-core/aiida/cmdline/params/options/__init__.py

Line 208 in 5e5d5e0

'-n', '--non-interactive', is_flag=True, is_eager=True, help='Non-interactive mode: never prompt for input.'

My suggestion would be to focus a bit more on what the user can do

In non-interactive mode the cli never prompts for input and uses default values, if defined. Provide your values via the respective command line options or via a configuration file.

[sphuber]

My suggestion would be to focus a bit more on what the user can do

In non-interactive mode the cli never prompts for input and uses default values, if defined. Provide your values via the respective command line options or via a configuration file.

I took your suggestion but adapted it a bit and removed the mention of the configuration file. Although usable together, the --non-interactive and --config are not mutably dependent and so I think it does not belong in the description of the non-interactive flag. I have made it:

In non-interactive mode, the CLI never prompts but simply uses default values for options that define one.

[ltalirz]

thanks @sphuber , latest version works fine