Dependencies: update to `click==8.0` and remove `click-completion` #5111

sphuber · 2021-09-01T05:17:03Z

In this PR, I update the dependency requirement for click to the new v8.0 version. Quite a bit was changed and so our cmdline code had to be adapted significantly. Especially the interactive logic was changed allowing our custom InteractiveOption logic to be simplified significantly. After that, in the second commit we can now drop the click-completion dependency since all the tab-complete functionality we need are now integrated into click itself. See the two commits for more details on the changes.

sphuber · 2021-09-02T20:49:24Z

@chrisjsewell already assigned you before, but @ramirezfranciscof feel free to review this as well if you'd like

csadorf

Thanks for requesting my review. I am a bit confused by the size of this PR. Is it because the branch is not up-to-date?

setup.json

sphuber · 2021-10-06T09:33:42Z

Thanks for requesting my review. I am a bit confused by the size of this PR. Is it because the branch is not up-to-date?

Not really. This is just because their were quite a bit of chances in the new click version that required significant changes of our code. I would refer you to the commit messages for more information. I have rebased the branch so it is now up to date.

csadorf

All good from my side. I had only a cursory look over the code, so I'm not going to explicitly approve it, but thanks a lot for making sure that we can move forward with click 8.x. That was quite an effort.

tests/cmdline/commands/test_archive_import.py

unkcpz

Thanks! @sphuber
I am all good with the changes, only with a bunch of small questions.

I take a detail look at the new interactive.py implementation. I got a feeling when this PR get merged it is safe to use ! to set the value to None interactively. Anyway it is not emergent figure out that and will not be a reason to block this PR.

aiida/cmdline/params/options/contextualdefault.py

aiida/cmdline/commands/cmd_code.py

aiida/cmdline/params/types/plugin.py

aiida/cmdline/params/types/profile.py

tests/cmdline/commands/test_archive_import.py

tests/cmdline/commands/test_computer.py

sphuber · 2021-10-26T17:51:29Z

Thanks for your review @unkcpz . I have addressed your comments and the tests pass. Sole exception is Jenkins, but that seems to be unrelated. Not sure what it is having trouble with

unkcpz

@sphuber Thanks, looks good to me. Let's get it merged and I'll try it soon.

chrisjsewell

Don't merge just yet, I want to give it a quick once over today (won't delay it by long)

tests/conftest.py

aiida/cmdline/params/options/conditional.py

sphuber · 2021-10-28T06:48:32Z

@chrisjsewell please let me know if I can merge this now

chrisjsewell · 2021-10-28T07:39:57Z

tests/cmdline/commands/test_code.py

-@pytest.mark.parametrize('non_interactive_editor', ('sleep 1; vim -cwq',), indirect=True)
-def test_interactive_remote(clear_database_before_test, aiida_localhost, non_interactive_editor):
+@pytest.fixture
+def code(aiida_localhost):


I'd move this centrally to pytest fixtures, as I'm sure it could be of more general use. Not a biggie though

chrisjsewell

All good cheers, I hope 🤞

chrisjsewell

Actually,

https://theossrv6.epfl.ch/jenkins/blue/organizations/jenkins/aiida_core_aiidateam/detail/PR-5111/21/pipeline/56 appears to be hanging at:

aiida-core/.molecule/default/setup_aiida.yml

Lines 59 to 71 in 5530589

    
             - name: verdi computer setup localhost 
        
               when: aiida_check_computer.rc != 0 
        
               command: > 
        
                 {{ venv_bin }}/verdi -p {{ aiida_backend }} computer setup 
        
                 --non-interactive 
        
                 --label "localhost" 
        
                 --description "this computer" 
        
                 --hostname "localhost" 
        
                 --transport core.local 
        
                 --scheduler core.direct 
        
                 --work-dir {{ aiida_path }}/local_work_dir/ 
        
                 --mpirun-command "mpirun -np {tot_num_mpiprocs}" 
        
                 --mpiprocs-per-machine 1

Can you have a look into this, as I have a feeling it is something to do with the new CLI interactive setup (perhaps waiting for user input, despite the command setting --non-interactive)

chrisjsewell · 2021-10-28T08:27:18Z

Can you have a look into this, as I have a feeling it is something to do with the new CLI interactive setup

Yeh this just run through fine in #5201, so I definitely feel it is a "new issue" from this PR

chrisjsewell · 2021-10-28T08:58:19Z

Getting further, but now hanging on:

aiida-core/.molecule/default/test_polish_workchains.yml

Lines 22 to 28 in f5053d6

    
             - name: verdi add code setup 
        
               when: aiida_check_code.rc != 0 
        
               command: > 
        
                 {{ venv_bin }}/verdi -p {{ aiida_backend }} code setup 
        
                 -D "simple script that adds two numbers" 
        
                 -n -L add -P core.arithmetic.add 
        
                 -Y localhost --remote-abs-path=/bin/bash

Should verdi computer setup --non-interactive have hung in the first place though? Was it because it was asking for user input? If so was this because it is no longer grabbing defaults for one of shebang/append-text/prepend-text, and should it just fail anyway if --non-interactive is given without sufficient parameters set?

sphuber · 2021-10-28T09:04:49Z

Yeah, I think it is a problem with defaults not being taken. I was wondering why this then also didn't fail for the GHA, but there the config includes explicit values for those options. Will have a look if it really is the defaults not being taken with --non-interactive and if so why

chrisjsewell · 2021-10-28T09:06:30Z

Oh well, nice to see the Jenkins tests actually being useful and catching a regression lol

codecov · 2021-10-28T09:12:19Z

Codecov Report

Merging #5111 (969e36e) into develop (f5053d6) will decrease coverage by 0.01%.
The diff coverage is 86.87%.

❗ Current head 969e36e differs from pull request most recent head 0e33cf5. Consider uploading reports for the commit 0e33cf5 to get more accurate results

@@             Coverage Diff             @@
##           develop    #5111      +/-   ##
===========================================
- Coverage    81.00%   81.00%   -0.00%     
===========================================
  Files          535      532       -3     
  Lines        37410    37344      -66     
===========================================
- Hits         30301    30247      -54     
+ Misses        7109     7097      -12

Flag	Coverage Δ
django	`75.81% <86.87%> (-0.01%)`	⬇️
sqlalchemy	`74.92% <86.87%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
aiida/cmdline/__init__.py	`100.00% <ø> (ø)`
aiida/cmdline/params/__init__.py	`100.00% <ø> (ø)`
aiida/cmdline/params/options/__init__.py	`100.00% <ø> (ø)`
aiida/cmdline/params/options/main.py	`96.04% <ø> (ø)`
aiida/cmdline/params/types/__init__.py	`100.00% <ø> (ø)`
aiida/cmdline/commands/cmd_run.py	`89.66% <33.34%> (-0.17%)`	⬇️
aiida/cmdline/params/types/profile.py	`63.64% <50.00%> (-0.64%)`	⬇️
aiida/cmdline/params/types/plugin.py	`85.19% <61.54%> (-3.27%)`	⬇️
aiida/transports/cli.py	`92.40% <75.00%> (-0.94%)`	⬇️
aiida/cmdline/params/types/path.py	`91.05% <80.00%> (ø)`
... and 13 more

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 f5053d6...0e33cf5. Read the comment docs.

The entire logic around parameter parsing, including the consuming of the value, optionally prompting and then processing the value has changed. Especially the prompting logic has changed, which now makes the custom logic in our `InteractiveOption` largely unnecessary. It had to be refactored significantly and it now no longer changes the prompt behavior but just overrides certain methods to include the concept of non-interactivity which is not native to `click`. In addition to this major change, there were also various smaller changes. The following adaptations had to be made for compatibility: * Parameter validators now have to return the value, whereas before this was not required. * Custom parameter types need to start with checking for the value to already have the expected return type and then return it. This is necessary because the convert method can be called multiple times: https://click.palletsprojects.com/en/8.0.x/parameters/#implementing-custom-types * The `aiida.cmdline.params.options.contextualdefault.ContextualDefaultOption` has been removed as it was not used in `aiida-core` nor in any plugin that is hosted on Github. * `Parameter.get_default` now takes the `call` argument * Remove explicit parameter name from `version_option`, this is now by default set to just `--version`. * Add explicit `type` for `MultipleValueOption` options in `verdi run`. This is necessary because without it the entire string would be parsed as a single string and not a tuple of strings. * The `ConditionalOption` test `test_prompt_callback` had to be changed. With the old `click`, as soon as wrong input was provided at the prompt and the callback raised, the command would fail. With the new behavior of `click`, the user will be prompted until the callback validates, printing the error message each time. This required the test to be changed to actually pass a valid input at the end through the `user_input` or the test would hang as it was infinitely prompting. * The `Path` parameter removed the `path_type` attribute. It has been more or less been replaced by `name`. * The `click._compat.filename_ui` utility was removed. Note that the lower requirement for `click` is set to `v8.0.3` since in lower patch versions the behavior of prompts for boolean type parameters is different.

As of `click==8.0` the full set of functionality that `click-completion` provided for our tab-completion is now shipped with `click` itself, so we can remove this dependency. The main difference is that the string that is required to activate it, is slightly different and it is shell dependent. The documentation has been updated to correctly reflect this. Since it is now shell dependent, the old `verdi completioncommand`, which was already (unofficially) deprecated is no longer correct. To keep it correct, one would have to try and detect the shell but this is clearly outside of the scope of `aiida-core` so we simply remote it.

sphuber · 2021-10-28T11:30:36Z

@chrisjsewell all fixed now

chrisjsewell · 2021-10-28T11:39:39Z

Just to check, is this now still the behaviour?

$ verdi computer setup --non-interactive
Usage: verdi computer setup [OPTIONS]

Error: Missing option '-L' / '--label'.

sphuber · 2021-10-28T11:43:50Z

Just to check, is this now still the behaviour?

$ verdi computer setup --non-interactive
Usage: verdi computer setup [OPTIONS]

Error: Missing option '-L' / '--label'.

yes

chrisjsewell

take two

sphuber · 2021-10-28T11:44:42Z

The fix was that the TemplateInteractiveOption, which subclasses the InteractiveOption, did not properly handle defaults in its prompt_for_value, which is properly handled in InteractiveOption but the subclass complete overrode it.

sphuber requested a review from chrisjsewell September 1, 2021 05:17

sphuber force-pushed the fix/5098/remove-click-completion branch 3 times, most recently from c20983e to 370020c Compare September 2, 2021 20:10

sphuber force-pushed the fix/5098/remove-click-completion branch 3 times, most recently from 2e1c561 to 37f10be Compare September 9, 2021 07:14

sphuber force-pushed the fix/5098/remove-click-completion branch from 37f10be to 441a300 Compare September 17, 2021 13:52

sphuber mentioned this pull request Sep 17, 2021

Command line: allow to not set input plugin for code #5140

Closed

sphuber force-pushed the fix/5098/remove-click-completion branch from 441a300 to 32f074d Compare September 17, 2021 14:20

sphuber requested a review from csadorf October 6, 2021 08:07

csadorf reviewed Oct 6, 2021

View reviewed changes

setup.json Outdated Show resolved Hide resolved

csadorf added the dependencies Pull requests that update a dependency file label Oct 6, 2021

sphuber force-pushed the fix/5098/remove-click-completion branch from 32f074d to a58b344 Compare October 6, 2021 09:31

sphuber force-pushed the fix/5098/remove-click-completion branch from d053847 to d55f6a1 Compare October 6, 2021 09:41

csadorf reviewed Oct 6, 2021

View reviewed changes

tests/cmdline/commands/test_archive_import.py Show resolved Hide resolved

unkcpz self-requested a review October 21, 2021 09:02

unkcpz previously approved these changes Oct 21, 2021

View reviewed changes

sphuber dismissed unkcpz’s stale review via d55f6a1 October 22, 2021 09:24

sphuber force-pushed the fix/5098/remove-click-completion branch 3 times, most recently from daf3c9d to 7da17d6 Compare October 26, 2021 15:57

sphuber requested a review from unkcpz October 26, 2021 17:50

unkcpz previously approved these changes Oct 27, 2021

View reviewed changes

chrisjsewell suggested changes Oct 27, 2021

View reviewed changes

chrisjsewell reviewed Oct 27, 2021

View reviewed changes

tests/conftest.py Show resolved Hide resolved

chrisjsewell reviewed Oct 27, 2021

View reviewed changes

aiida/cmdline/params/options/conditional.py Outdated Show resolved Hide resolved

sphuber dismissed unkcpz’s stale review via a66b00b October 27, 2021 22:10

sphuber force-pushed the fix/5098/remove-click-completion branch from 7da17d6 to a66b00b Compare October 27, 2021 22:10

sphuber requested a review from chrisjsewell October 27, 2021 22:11

chrisjsewell reviewed Oct 28, 2021

View reviewed changes

chrisjsewell previously approved these changes Oct 28, 2021

View reviewed changes

sphuber dismissed chrisjsewell’s stale review via 904aeac October 28, 2021 07:42

sphuber force-pushed the fix/5098/remove-click-completion branch from ed5f324 to 904aeac Compare October 28, 2021 07:42

chrisjsewell suggested changes Oct 28, 2021

View reviewed changes

ramirezfranciscof mentioned this pull request Oct 28, 2021

Verdi: Warn for already existing code@computer label #5194

Closed

sphuber force-pushed the fix/5098/remove-click-completion branch from fc87f3a to 969e36e Compare October 28, 2021 09:30

sphuber added 2 commits October 28, 2021 13:00

sphuber force-pushed the fix/5098/remove-click-completion branch from 969e36e to 0e33cf5 Compare October 28, 2021 11:01

sphuber requested a review from chrisjsewell October 28, 2021 11:30

chrisjsewell approved these changes Oct 28, 2021

View reviewed changes

sphuber merged commit 62ff5c4 into aiidateam:develop Oct 28, 2021

sphuber deleted the fix/5098/remove-click-completion branch October 28, 2021 11:44

sphuber mentioned this pull request Oct 28, 2021

conftest: add input arg for run_cli_command, fix typings #5175

Closed

chrisjsewell mentioned this pull request Mar 12, 2022

🔀 MERGE: Release v2.0.0b1 #5426

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dependencies: update to `click==8.0` and remove `click-completion` #5111

Dependencies: update to `click==8.0` and remove `click-completion` #5111

sphuber commented Sep 1, 2021 •

edited

Loading

sphuber commented Sep 2, 2021

csadorf left a comment

sphuber commented Oct 6, 2021

csadorf left a comment

unkcpz left a comment

sphuber commented Oct 26, 2021

unkcpz left a comment

chrisjsewell left a comment

sphuber commented Oct 28, 2021

chrisjsewell Oct 28, 2021

chrisjsewell left a comment

chrisjsewell left a comment •

edited

Loading

chrisjsewell commented Oct 28, 2021

chrisjsewell commented Oct 28, 2021

sphuber commented Oct 28, 2021

chrisjsewell commented Oct 28, 2021

codecov bot commented Oct 28, 2021 •

edited

Loading

sphuber commented Oct 28, 2021

chrisjsewell commented Oct 28, 2021

sphuber commented Oct 28, 2021

chrisjsewell left a comment

sphuber commented Oct 28, 2021

	- name: verdi computer setup localhost
	when: aiida_check_computer.rc != 0
	command: >
	{{ venv_bin }}/verdi -p {{ aiida_backend }} computer setup
	--non-interactive
	--label "localhost"
	--description "this computer"
	--hostname "localhost"
	--transport core.local
	--scheduler core.direct
	--work-dir {{ aiida_path }}/local_work_dir/
	--mpirun-command "mpirun -np {tot_num_mpiprocs}"
	--mpiprocs-per-machine 1

Dependencies: update to click==8.0 and remove click-completion #5111

Dependencies: update to click==8.0 and remove click-completion #5111

Conversation

sphuber commented Sep 1, 2021 • edited Loading

sphuber commented Sep 2, 2021

csadorf left a comment

Choose a reason for hiding this comment

sphuber commented Oct 6, 2021

csadorf left a comment

Choose a reason for hiding this comment

unkcpz left a comment

Choose a reason for hiding this comment

sphuber commented Oct 26, 2021

unkcpz left a comment

Choose a reason for hiding this comment

chrisjsewell left a comment

Choose a reason for hiding this comment

sphuber commented Oct 28, 2021

chrisjsewell Oct 28, 2021

Choose a reason for hiding this comment

chrisjsewell left a comment

Choose a reason for hiding this comment

chrisjsewell left a comment • edited Loading

Choose a reason for hiding this comment

chrisjsewell commented Oct 28, 2021

chrisjsewell commented Oct 28, 2021

sphuber commented Oct 28, 2021

chrisjsewell commented Oct 28, 2021

codecov bot commented Oct 28, 2021 • edited Loading

Codecov Report

sphuber commented Oct 28, 2021

chrisjsewell commented Oct 28, 2021

sphuber commented Oct 28, 2021

chrisjsewell left a comment

Choose a reason for hiding this comment

sphuber commented Oct 28, 2021

Dependencies: update to `click==8.0` and remove `click-completion` #5111

Dependencies: update to `click==8.0` and remove `click-completion` #5111

sphuber commented Sep 1, 2021 •

edited

Loading

chrisjsewell left a comment •

edited

Loading

codecov bot commented Oct 28, 2021 •

edited

Loading