verdi computer setup migrated to click #1735

giovannipizzi · 2018-07-09T18:57:29Z

verdi computer setup migration to click.
The other two interactive options (configure and update) will come in a later PR.

It is still incomplete, some options are missing, and the test infrastructure has not been adapted yet.

Many tests already added for the non-interactive part. Still a few to add for the interactive part, and to adapt the tests.

Also, changed back the default question from 'disabled' to 'enabled'

codecov-io · 2018-07-09T19:14:34Z

Codecov Report

Merging #1735 into verdi will increase coverage by 0.53%.
The diff coverage is 84.48%.

@@            Coverage Diff            @@
##            verdi   #1735      +/-   ##
=========================================
+ Coverage   61.66%   62.2%   +0.53%     
=========================================
  Files         300     300              
  Lines       33471   33430      -41     
=========================================
+ Hits        20641   20794     +153     
+ Misses      12830   12636     -194

Impacted Files	Coverage Δ
aiida/cmdline/commands/code.py	`90.96% <100%> (+0.32%)`	⬆️
aiida/orm/implementation/general/computer.py	`64.47% <50%> (+13.6%)`	⬆️
aiida/cmdline/params/types/computer.py	`80.48% <76.47%> (-19.52%)`	⬇️
aiida/control/computer.py	`85.71% <85.71%> (ø)`
aiida/cmdline/commands/computer.py	`45.05% <88.33%> (+12.87%)`	⬆️
aiida/backends/djsite/db/models.py	`75.34% <0%> (+0.12%)`	⬆️
aiida/scheduler/datastructures.py	`63.97% <0%> (+0.62%)`	⬆️
aiida/orm/implementation/django/computer.py	`65.95% <0%> (+2.65%)`	⬆️
aiida/backends/sqlalchemy/models/computer.py	`87.14% <0%> (+2.85%)`	⬆️
... and 3 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 7943be3...3443372. Read the comment docs.

DropD

Great work, just a few minor things, and also please add the relevant files to .pre-commit-config.yaml

DropD · 2018-07-10T09:43:01Z

aiida/backends/tests/cmdline/commands/test_computer.py

+        self.runner = CliRunner()
+
+    def test_help(self):
+        self.runner.invoke(setup_computer, ['--help'])


Note: this does not fail if the command throws an error, use

self.runner.invoke(setup_computer, ['--help'], catch_exceptions=False)

ok. I copied this from test_code so I will fix this there too.

DropD · 2018-07-10T09:44:35Z

aiida/backends/tests/cmdline/commands/test_computer.py

+
+    def test_reachable(self):
+        import subprocess as sp
+        output = sp.check_output(['verdi', 'computer', 'setup', '--help'])


Switch off exception catching here too, because 'Usage' will also be in the output if computer is considered an invalid command.

This is check_output, that raises an exception in case of non-zero return code: https://docs.python.org/2/library/subprocess.html#subprocess.check_output
So I don't think I need to change here

DropD · 2018-07-10T09:44:54Z

aiida/backends/tests/cmdline/commands/test_computer.py

+        output = sp.check_output(['verdi', 'computer', 'setup', '--help'])
+        self.assertIn('Usage:', output)
+
+    # TODO: test that mpiprocs-per-machine is not requested e.g. for SGE


Make this into an issue instead.

Actually it looks like you have a test for this already!

Yes, I've a couple of leftover comments, I'm going to clean these up

DropD · 2018-07-10T09:49:34Z

aiida/backends/tests/cmdline/commands/test_computer.py

+
+    def test_noninteractive_optional_default_mpiprocs_2(self):
+        """
+        Test 'verdi setup' and check that if is the specified value is zero, it means unspecified


"... check that if the value specified for mpiprocs-per-machine is zero ..."

DropD · 2018-07-10T09:53:47Z

aiida/cmdline/commands/computer.py

-                except EOFError:
-                    # Ctrl+D pressed: end of input.
-                    pass
+## Todo: reenable interactive and mixed tests


They look enabled to me, please double-check and remove this line

DropD · 2018-07-10T09:55:58Z

aiida/cmdline/params/options/__init__.py

@@ -151,7 +151,7 @@ def active_process_states():
 RAW = OverridableOption('-r', '--raw', 'raw', is_flag=True, default=False,
                        help='display only raw query results, without any headers or footers')

-HOSTNAME = OverridableOption('-H', '--hostname', help='hostname')
+HOSTNAME = OverridableOption('-H', '--hostname', help='the computer hostname')


Keep in mind this is also used for the postgres host name

ok, I revert this back, anyway I put a longer one for verdi computer setup

DropD · 2018-07-10T10:01:25Z

aiida/control/computer.py

+    """Build a computer with validation of attribute combinations"""
+
+    def __init__(self, **kwargs):
+        self._computer_spec = kwargs


@ltalirz mentioned in the CodeBuilder that this would be safer instead:

for key, value in kwargs.items(): self.__setattr__(key, value)

As it validates each keyword argument while setting it.

Also, removing two stale files that are not needed after non-interactive verdi code setup

giovannipizzi · 2018-07-10T13:56:32Z

Fixed. Files are not added to the pre-commit because this is only a partial fix to verdi code and there is still much to improve especially for prospector. We will add them with the final commit (some of the files are already on precommit).
For me it's good to merge.

giovannipizzi added 4 commits July 9, 2018 12:54

Starting to prepare the 'verdi setup' command.

d267346

It is still incomplete, some options are missing, and the test infrastructure has not been adapted yet.

Almost finished the implementation of verdi computer setup

67c46c1

Many tests already added for the non-interactive part. Still a few to add for the interactive part, and to adapt the tests.

Added interactive and mixed tests for 'verdi computer setup'

4da1e3d

Also, changed back the default question from 'disabled' to 'enabled'

Fixing the travis scripts to setup the computer

9403754

giovannipizzi requested review from DropD and ConradJohnston July 9, 2018 18:57

Merge branch 'verdi' into fix_1582_verdi_computer_click_only_interactive

8f2b5e9

DropD suggested changes Jul 10, 2018

View reviewed changes

giovannipizzi and others added 2 commits July 10, 2018 15:53

Minor fixes required in review of aiidateam#1735

42d6705

Also, removing two stale files that are not needed after non-interactive verdi code setup

Merge branch 'verdi' into fix_1582_verdi_computer_click_only_interactive

3443372

DropD approved these changes Jul 10, 2018

View reviewed changes

DropD merged commit 2463538 into aiidateam:verdi Jul 10, 2018

giovannipizzi deleted the fix_1582_verdi_computer_click_only_interactive branch July 10, 2018 14:15

ltalirz mentioned this pull request Sep 26, 2018

Computer configuration #1974

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

verdi computer setup migrated to click #1735

verdi computer setup migrated to click #1735

giovannipizzi commented Jul 9, 2018

codecov-io commented Jul 9, 2018 •

edited

Loading

DropD left a comment

DropD Jul 10, 2018

giovannipizzi Jul 10, 2018

DropD Jul 10, 2018

giovannipizzi Jul 10, 2018

DropD Jul 10, 2018

DropD Jul 10, 2018

DropD Jul 10, 2018

giovannipizzi Jul 10, 2018

DropD Jul 10, 2018

giovannipizzi Jul 10, 2018

DropD Jul 10, 2018

giovannipizzi Jul 10, 2018

DropD Jul 10, 2018

giovannipizzi Jul 10, 2018

DropD Jul 10, 2018

giovannipizzi Jul 10, 2018

giovannipizzi commented Jul 10, 2018

verdi computer setup migrated to click #1735

verdi computer setup migrated to click #1735

Conversation

giovannipizzi commented Jul 9, 2018

codecov-io commented Jul 9, 2018 • edited Loading

Codecov Report

DropD left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

giovannipizzi commented Jul 10, 2018

codecov-io commented Jul 9, 2018 •

edited

Loading