From cba7d6d58baeac67d5860234cf296b60f2d23ef6 Mon Sep 17 00:00:00 2001 From: Marnik Bercx Date: Thu, 11 Feb 2021 21:50:37 +0100 Subject: [PATCH 1/5] Work on `verdi group remove-nodes` command (#4728). * `verdi group remove-nodes`: Add warning when nodes are not in the group Currently, the `verdi group remove-nodes` command does not raise any warning when the nodes that the user wants to remove are not in the group. It also says it removed the number of requested nodes from the group, even when none of them is in the group specified. Here we: * Have the command fail with a `Critical` message when none of the requested nodes are in the group. * Raise a warning when any of the nodes requested are not in the specified group, and list the PK's of the nodes that are missing. Note that the Group.remove_nodes() command still does not raise any warning when the requested nodes are not in the group. * Fix bug and improve API Fixes a bug when the user actually doesn't provide any nodes. In case the `--clear` flag is also not provided, the command will fail since there is nothing to remove. In case it is provided, the command will ask for confirmation to remove all nodes unless the force flag is also set. * Fail if both the `--clear` flag and nodes are provided In the current API, it doesn't make sense to provide *both* the `--clear` flag and a list of node identifiers. Here we check if both are provided and abort the command in this case. * Add tests. --- aiida/cmdline/commands/cmd_group.py | 37 +++++++++++++++++++++++++--- tests/cmdline/commands/test_group.py | 33 +++++++++++++++++++++++-- 2 files changed, 64 insertions(+), 6 deletions(-) diff --git a/aiida/cmdline/commands/cmd_group.py b/aiida/cmdline/commands/cmd_group.py index f623fe74c0..20b8303f0e 100644 --- a/aiida/cmdline/commands/cmd_group.py +++ b/aiida/cmdline/commands/cmd_group.py @@ -47,12 +47,41 @@ def group_add_nodes(group, force, nodes): @with_dbenv() def group_remove_nodes(group, nodes, clear, force): """Remove nodes from a group.""" - if clear: - message = f'Do you really want to remove ALL the nodes from Group<{group.label}>?' - else: - message = f'Do you really want to remove {len(nodes)} nodes from Group<{group.label}>?' + from aiida.orm import QueryBuilder, Group, Node + + label = group.label + klass = group.__class__.__name__ + + if nodes and clear: + echo.echo_critical( + 'Specify either the `--clear` flag to remove all nodes or the identifiers of the nodes you want to remove.' + ) if not force: + + if nodes: + node_pks = [node.pk for node in nodes] + + query = QueryBuilder() + query.append(Group, filters={'id': group.pk}, tag='group') + query.append(Node, with_group='group', filters={'id': {'in': node_pks}}, project='id') + + group_node_pks = query.all(flat=True) + + if not group_node_pks: + echo.echo_critical(f'None of the specified nodes are in {klass}<{label}>.') + + if len(node_pks) > len(group_node_pks): + node_pks = set(node_pks).difference(set(group_node_pks)) + echo.echo_warning(f'{len(node_pks)} nodes with PK {node_pks} are not in {klass}<{label}>.') + + message = f'Are you sure you want to remove {len(group_node_pks)} nodes from {klass}<{label}>?' + + elif clear: + message = f'Are you sure you want to remove ALL the nodes from {klass}<{label}>?' + else: + echo.echo_critical(f'No nodes were provided for removal from {klass}<{label}>.') + click.confirm(message, abort=True) if clear: diff --git a/tests/cmdline/commands/test_group.py b/tests/cmdline/commands/test_group.py index 0a4f3c0933..db0cf51949 100644 --- a/tests/cmdline/commands/test_group.py +++ b/tests/cmdline/commands/test_group.py @@ -12,6 +12,7 @@ from aiida.backends.testbase import AiidaTestCase from aiida.common import exceptions from aiida.cmdline.commands import cmd_group +from aiida.cmdline.utils.echo import ExitCode class TestVerdiGroup(AiidaTestCase): @@ -156,7 +157,6 @@ def test_delete(self): self.assertEqual(group.count(), 2) result = self.cli_runner.invoke(cmd_group.group_delete, ['--force', 'group_test_delete_02']) - self.assertClickResultNoException(result) with self.assertRaises(exceptions.NotExistent): orm.load_group(label='group_test_delete_02') @@ -265,7 +265,7 @@ def test_add_remove_nodes(self): result = self.cli_runner.invoke(cmd_group.group_remove_nodes, ['--force', '--group=dummygroup1', node_01.uuid]) self.assertIsNone(result.exception, result.output) - # Check if node is added in group using group show command + # Check that the node is no longer in the group result = self.cli_runner.invoke(cmd_group.group_show, ['-r', 'dummygroup1']) self.assertClickResultNoException(result) self.assertNotIn('CalculationNode', result.output) @@ -280,6 +280,35 @@ def test_add_remove_nodes(self): self.assertClickResultNoException(result) self.assertEqual(group.count(), 0) + # Try to remove node that isn't in the group + result = self.cli_runner.invoke(cmd_group.group_remove_nodes, ['--group=dummygroup1', node_01.uuid]) + self.assertEqual(result.exit_code, ExitCode.CRITICAL) + + # Try to remove no nodes nor clear the group + result = self.cli_runner.invoke(cmd_group.group_remove_nodes, ['--group=dummygroup1']) + self.assertEqual(result.exit_code, ExitCode.CRITICAL) + + # Try to remove both nodes and clear the group + result = self.cli_runner.invoke(cmd_group.group_remove_nodes, ['--group=dummygroup1', '--clear', node_01.uuid]) + self.assertEqual(result.exit_code, ExitCode.CRITICAL) + + # Add a node with confirmation + result = self.cli_runner.invoke(cmd_group.group_add_nodes, ['--group=dummygroup1', node_01.uuid], input='y') + self.assertEqual(group.count(), 1) + + # Try to remove two nodes, one that isn't in the group, but abort + result = self.cli_runner.invoke( + cmd_group.group_remove_nodes, ['--group=dummygroup1', node_01.uuid, node_02.uuid], input='N' + ) + self.assertIn('Warning', result.output) + self.assertEqual(group.count(), 1) + + # Try to clear all nodes from the group, but abort + result = self.cli_runner.invoke(cmd_group.group_remove_nodes, ['--group=dummygroup1', '--clear'], input='N') + self.assertIn('Are you sure you want to remove ALL', result.output) + self.assertIn('Aborted', result.output) + self.assertEqual(group.count(), 1) + def test_copy_existing_group(self): """Test user is prompted to continue if destination group exists and is not empty""" source_label = 'source_copy_existing_group' From dca50f47a340ffa14302d7bd0714ba86227b4b57 Mon Sep 17 00:00:00 2001 From: Carl Simon Adorf Date: Fri, 12 Feb 2021 10:12:20 +0100 Subject: [PATCH 2/5] CI: Increase output verbosity of tests suite. (#4740) --- .github/workflows/tests.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/.github/workflows/tests.sh b/.github/workflows/tests.sh index 7ce9855d86..db69cfa29c 100755 --- a/.github/workflows/tests.sh +++ b/.github/workflows/tests.sh @@ -18,6 +18,7 @@ export PYTEST_ADDOPTS="${PYTEST_ADDOPTS} --cov-config=${GITHUB_WORKSPACE}/.cover export PYTEST_ADDOPTS="${PYTEST_ADDOPTS} --cov-report xml" export PYTEST_ADDOPTS="${PYTEST_ADDOPTS} --cov-append" export PYTEST_ADDOPTS="${PYTEST_ADDOPTS} --cov=aiida" +export PYTEST_ADDOPTS="${PYTEST_ADDOPTS} --verbose" # daemon tests verdi daemon start 4 From 28447f0cf5dd626d3eb350a93d821501df77346a Mon Sep 17 00:00:00 2001 From: Carl Simon Adorf Date: Tue, 16 Feb 2021 15:54:13 +0100 Subject: [PATCH 3/5] Skip test 'TestVerdiProcessDaemon::test_pause_play_kill'. (#4747) The test randomly fails to complete within a reasonable amount of time leading to a significant disruption of our CI pipeline. Investigated in issue #4731. --- tests/cmdline/commands/test_process.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/cmdline/commands/test_process.py b/tests/cmdline/commands/test_process.py index eb829df86f..39d8a7aabb 100644 --- a/tests/cmdline/commands/test_process.py +++ b/tests/cmdline/commands/test_process.py @@ -64,6 +64,7 @@ def tearDown(self): os.kill(self.daemon.pid, signal.SIGTERM) super().tearDown() + @pytest.mark.skip(reason='fails to complete randomly (see issue #4731)') @pytest.mark.requires_rmq def test_pause_play_kill(self): """ From 4a7d3bb0294ea24bb55d7c793ccf4ad87e3949a1 Mon Sep 17 00:00:00 2001 From: Chris Sewell Date: Wed, 17 Feb 2021 12:32:26 +0100 Subject: [PATCH 4/5] =?UTF-8?q?=F0=9F=A7=AA=20TESTS:=20Fix=20pre-commit=20?= =?UTF-8?q?(pin=20astroid)=20(#4757)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Temporary fix for https://github.com/PyCQA/astroid/issues/895 --- setup.json | 1 + 1 file changed, 1 insertion(+) diff --git a/setup.json b/setup.json index 03873c2074..b249a2528a 100644 --- a/setup.json +++ b/setup.json @@ -92,6 +92,7 @@ "notebook~=6.1,>=6.1.5" ], "pre-commit": [ + "astroid<2.5", "mypy==0.790", "packaging==20.3", "pre-commit~=2.2", From 3c5abcf9b07742514f0a4e167ef4d2fb235e9a43 Mon Sep 17 00:00:00 2001 From: Chris Sewell Date: Wed, 17 Feb 2021 12:49:24 +0100 Subject: [PATCH 5/5] =?UTF-8?q?=F0=9F=A7=AA=20TESTS:=20Fix=20plumpy=20inco?= =?UTF-8?q?mpatibility=20(#4751)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit As of https://github.com/aiidateam/plumpy/commit/7004bd96bbaa678b5486a62677e139216877deef, a paused workchain will hang if it is closed then played. This test violated that rule and also was faulty, in that it should test that the reloaded workchain can be played, not the original workchain. --- tests/engine/test_work_chain.py | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/tests/engine/test_work_chain.py b/tests/engine/test_work_chain.py index 0ebf6048af..49166c9872 100644 --- a/tests/engine/test_work_chain.py +++ b/tests/engine/test_work_chain.py @@ -680,7 +680,8 @@ def do_run(self): run_and_check_success(MainWorkChain) def test_if_block_persistence(self): - """ + """Test a reloaded `If` conditional can be resumed. + This test was created to capture issue #902 """ runner = get_manager().get_runner() @@ -688,11 +689,13 @@ def test_if_block_persistence(self): runner.schedule(wc) async def run_async(workchain): + + # run the original workchain until paused await run_until_paused(workchain) self.assertTrue(workchain.ctx.s1) self.assertFalse(workchain.ctx.s2) - # Now bundle the thing + # Now bundle the workchain bundle = plumpy.Bundle(workchain) # Need to close the process before recreating a new instance workchain.close() @@ -702,13 +705,20 @@ async def run_async(workchain): self.assertTrue(workchain2.ctx.s1) self.assertFalse(workchain2.ctx.s2) + # check bundling again creates the same saved state bundle2 = plumpy.Bundle(workchain2) self.assertDictEqual(bundle, bundle2) - workchain.play() - await workchain.future() - self.assertTrue(workchain.ctx.s1) - self.assertTrue(workchain.ctx.s2) + # run the loaded workchain to completion + runner.schedule(workchain2) + workchain2.play() + await workchain2.future() + self.assertTrue(workchain2.ctx.s1) + self.assertTrue(workchain2.ctx.s2) + + # ensure the original paused workchain future is finalised + # to avoid warnings + workchain.future().set_result(None) runner.loop.run_until_complete(run_async(wc))