From 2b06f6e1b4cc19156ef7020fe6fcc0c9837bd01a Mon Sep 17 00:00:00 2001 From: Hilary James Oliver Date: Mon, 23 Dec 2019 09:09:48 +1300 Subject: [PATCH 1/5] Stop spurious warnings on adding to internal queues. --- cylc/flow/config.py | 30 +++++++++++++++++------------- 1 file changed, 17 insertions(+), 13 deletions(-) diff --git a/cylc/flow/config.py b/cylc/flow/config.py index f3a19ba38c9..904bb1bbc41 100644 --- a/cylc/flow/config.py +++ b/cylc/flow/config.py @@ -1092,9 +1092,11 @@ def configure_queues(self): # Then reassign to other queues as requested. warnings = [] requeued = [] + # Record non-default queues by task name, to avoid spurious warnings + # about tasks "already added to a queue", when the queue is the same. + myq = {} for key, queue in list(queues.copy().items()): - # queues.copy() is essential here to allow items to be removed from - # the queues dict. + myq[key] = [] if key == self.Q_DEFAULT: continue # Assign tasks to queue and remove them from default. @@ -1109,7 +1111,7 @@ def configure_queues(self): try: queues[self.Q_DEFAULT]['members'].remove(fmem) except ValueError: - if fmem in requeued: + if fmem in requeued and fmem not in myq[key]: msg = "%s: ignoring %s from %s (%s)" % ( key, fmem, qmember, 'already assigned to a queue') @@ -1118,6 +1120,7 @@ def configure_queues(self): # Ignore: task not used in the graph. pass else: + myq[key] = fmem qmembers.append(fmem) requeued.append(fmem) else: @@ -1127,31 +1130,32 @@ def configure_queues(self): queues[self.Q_DEFAULT]['members'].remove(qmember) except ValueError: if qmember in requeued: - msg = "%s: ignoring '%s' (%s)" % ( - key, qmember, 'task already assigned') + msg = "%s: ignoring %s (%s)" % ( + key, qmember, + 'already assigned to a queue') warnings.append(msg) elif qmember not in all_task_names: - msg = "%s: ignoring '%s' (%s)" % ( + msg = "%s: ignoring %s (%s)" % ( key, qmember, 'task not defined') warnings.append(msg) else: # Ignore: task not used in the graph. pass else: + myq[key] = qmember qmembers.append(qmember) requeued.append(qmember) - - if warnings: - err_msg = "Queue configuration warnings:" - for msg in warnings: - err_msg += "\n+ %s" % msg - LOG.warning(err_msg) - if qmembers: queue['members'] = qmembers else: del queues[key] + if warnings: + err_msg = "Queue configuration warnings:" + for msg in warnings: + err_msg += "\n+ %s" % msg + LOG.warning(err_msg) + if cylc.flow.flags.verbose and len(queues) > 1: log_msg = "Internal queues created:" for key, queue in queues.items(): From 0d888d1ea99996be527523c853f837c1964e0e4c Mon Sep 17 00:00:00 2001 From: Hilary James Oliver Date: Mon, 30 Mar 2020 13:08:29 +1300 Subject: [PATCH 2/5] Add test and change log. --- CHANGES.md | 5 ++++ tests/queues/03-warnings.t | 51 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 tests/queues/03-warnings.t diff --git a/CHANGES.md b/CHANGES.md index 0c50080b928..5c83c775a4d 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -20,6 +20,8 @@ the cylc/cylc-ui repository (and see also cylc/cylc-uiserver). The User Guide an other documention has been removed from the Python package to the cylc/cylc-doc repository. +### Fixes + Cylc 8.0aX (alpha) releases are not compatible with Cylc 7 or with previous 8.0aX releases, as the API is still under heavy development. @@ -33,6 +35,9 @@ The xtrigger examples were moved to a separate `cylc/cylc-xtriggers` project Jinja filters were moved from its `Jinja2Filters` folder to within the `cylc` namespace, under `cylc.jinja.filters`. +[#3541](https://github.com/cylc/cylc-flow/pull/3541) - Don't warn that a task +was already added to an internal queue, if the queue is the same. + ------------------------------------------------------------------------------- ## __cylc-8.0a2 (2019-Q4?)__ diff --git a/tests/queues/03-warnings.t b/tests/queues/03-warnings.t new file mode 100644 index 00000000000..5eafc524117 --- /dev/null +++ b/tests/queues/03-warnings.t @@ -0,0 +1,51 @@ +#!/bin/bash +# THIS FILE IS PART OF THE CYLC SUITE ENGINE. +# Copyright (C) 2008-2019 NIWA & British Crown (Met Office) & Contributors. +# +# This program is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# This program is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with this program. If not, see . +#------------------------------------------------------------------------------- +# Test that reassigning a task to the same queue does not result in a warning. +# GitHub #3539 +. $(dirname $0)/test_header +#------------------------------------------------------------------------------- +set_test_number 2 +#------------------------------------------------------------------------------- +init_suite $TEST_NAME_BASE <<'__SUITE_RC__' +[scheduling] + [[queues]] + [[[q1]]] + members = A, B + [[[q2]]] + members = x + [[dependencies]] + graph = "x => y" +[runtime] + [[A]] + [[B]] + [[x]] + inherit = A, B + [[y]] +__SUITE_RC__ +#------------------------------------------------------------------------------- +TEST_NAME=$TEST_NAME_BASE-validate +run_ok $TEST_NAME cylc validate $SUITE_NAME +#------------------------------------------------------------------------------- +# Validation should not warn about x being added to q1 from family B +# but it should warn about x being added to q2 (already added to q1). +cmp_ok ${TEST_NAME}.stderr - <<'__STDOUT__' +WARNING - Queue configuration warnings: + + q2: ignoring x (already assigned to a queue) +__STDOUT__ +#------------------------------------------------------------------------------- +purge_suite $SUITE_NAME From f027c6d7172157eb8ae27c670b27ee694c1c25f2 Mon Sep 17 00:00:00 2001 From: Hilary James Oliver Date: Mon, 30 Mar 2020 16:51:21 +1300 Subject: [PATCH 3/5] Replace functional test with unit test. --- cylc/flow/tests/test_config.py | 32 ++++++++++++++++++++- tests/queues/03-warnings.t | 51 ---------------------------------- 2 files changed, 31 insertions(+), 52 deletions(-) delete mode 100644 tests/queues/03-warnings.t diff --git a/cylc/flow/tests/test_config.py b/cylc/flow/tests/test_config.py index 50d716362ff..251c4fe5a16 100644 --- a/cylc/flow/tests/test_config.py +++ b/cylc/flow/tests/test_config.py @@ -16,9 +16,10 @@ import os import pytest -from tempfile import TemporaryDirectory, NamedTemporaryFile +from tempfile import TemporaryDirectory, NamedTemporaryFile, mkdtemp from pathlib import Path from cylc.flow.config import SuiteConfig +from cylc.flow.tests.util import CylcWorkflowTestCase def get_test_inheritance_quotes(): @@ -211,3 +212,32 @@ def test_family_inheritance_and_quotes(self): ['MAINFAM_major1_minor10']) assert 'goodbye_0_major1_minor10' in \ config.runtime['descendants']['SOMEFAM'] + + +def test_queue_config(caplog): + suiterc_content = """ +[scheduling] + [[queues]] + [[[q1]]] + members = A, B + [[[q2]]] + members = x + [[dependencies]] + graph = "x => y" +[runtime] + [[A]] + [[B]] + [[x]] + inherit = A, B + [[y]] + """ + workflow_directory = Path(mkdtemp()) + suite_rc = Path(workflow_directory, "suite.rc") + with suite_rc.open(mode="w") as f: + f.write(suiterc_content) + f.flush() + config = SuiteConfig(suite="qtest", fpath=f.name) + config.configure_queues() + log = caplog.messages[0].split('\n') + assert log[0] == "Queue configuration warnings:" + assert log[1] == "+ q2: ignoring x (already assigned to a queue)" diff --git a/tests/queues/03-warnings.t b/tests/queues/03-warnings.t deleted file mode 100644 index 5eafc524117..00000000000 --- a/tests/queues/03-warnings.t +++ /dev/null @@ -1,51 +0,0 @@ -#!/bin/bash -# THIS FILE IS PART OF THE CYLC SUITE ENGINE. -# Copyright (C) 2008-2019 NIWA & British Crown (Met Office) & Contributors. -# -# This program is free software: you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program. If not, see . -#------------------------------------------------------------------------------- -# Test that reassigning a task to the same queue does not result in a warning. -# GitHub #3539 -. $(dirname $0)/test_header -#------------------------------------------------------------------------------- -set_test_number 2 -#------------------------------------------------------------------------------- -init_suite $TEST_NAME_BASE <<'__SUITE_RC__' -[scheduling] - [[queues]] - [[[q1]]] - members = A, B - [[[q2]]] - members = x - [[dependencies]] - graph = "x => y" -[runtime] - [[A]] - [[B]] - [[x]] - inherit = A, B - [[y]] -__SUITE_RC__ -#------------------------------------------------------------------------------- -TEST_NAME=$TEST_NAME_BASE-validate -run_ok $TEST_NAME cylc validate $SUITE_NAME -#------------------------------------------------------------------------------- -# Validation should not warn about x being added to q1 from family B -# but it should warn about x being added to q2 (already added to q1). -cmp_ok ${TEST_NAME}.stderr - <<'__STDOUT__' -WARNING - Queue configuration warnings: - + q2: ignoring x (already assigned to a queue) -__STDOUT__ -#------------------------------------------------------------------------------- -purge_suite $SUITE_NAME From acab90a1cab21cd35a122a8a8f7c54cedaa122a6 Mon Sep 17 00:00:00 2001 From: Hilary James Oliver Date: Mon, 30 Mar 2020 22:52:01 +1300 Subject: [PATCH 4/5] Address review feedback. --- cylc/flow/tests/test_config.py | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/cylc/flow/tests/test_config.py b/cylc/flow/tests/test_config.py index 251c4fe5a16..857daad5ebf 100644 --- a/cylc/flow/tests/test_config.py +++ b/cylc/flow/tests/test_config.py @@ -16,10 +16,9 @@ import os import pytest -from tempfile import TemporaryDirectory, NamedTemporaryFile, mkdtemp +from tempfile import TemporaryDirectory, NamedTemporaryFile from pathlib import Path from cylc.flow.config import SuiteConfig -from cylc.flow.tests.util import CylcWorkflowTestCase def get_test_inheritance_quotes(): @@ -214,7 +213,7 @@ def test_family_inheritance_and_quotes(self): config.runtime['descendants']['SOMEFAM'] -def test_queue_config(caplog): +def test_queue_config(caplog, tmp_path): suiterc_content = """ [scheduling] [[queues]] @@ -231,12 +230,9 @@ def test_queue_config(caplog): inherit = A, B [[y]] """ - workflow_directory = Path(mkdtemp()) - suite_rc = Path(workflow_directory, "suite.rc") - with suite_rc.open(mode="w") as f: - f.write(suiterc_content) - f.flush() - config = SuiteConfig(suite="qtest", fpath=f.name) + suite_rc = tmp_path / "suite.rc" + suite_rc.write_text(suiterc_content) + config = SuiteConfig(suite="qtest", fpath=suite_rc.absolute()) config.configure_queues() log = caplog.messages[0].split('\n') assert log[0] == "Queue configuration warnings:" From 00dedb40ad4bc1a35026cb0f12f2cbf45861f1a1 Mon Sep 17 00:00:00 2001 From: Hilary James Oliver Date: Mon, 30 Mar 2020 22:53:52 +1300 Subject: [PATCH 5/5] Removed unnecessary method call in test. --- cylc/flow/tests/test_config.py | 1 - 1 file changed, 1 deletion(-) diff --git a/cylc/flow/tests/test_config.py b/cylc/flow/tests/test_config.py index 857daad5ebf..61dad6ff12e 100644 --- a/cylc/flow/tests/test_config.py +++ b/cylc/flow/tests/test_config.py @@ -233,7 +233,6 @@ def test_queue_config(caplog, tmp_path): suite_rc = tmp_path / "suite.rc" suite_rc.write_text(suiterc_content) config = SuiteConfig(suite="qtest", fpath=suite_rc.absolute()) - config.configure_queues() log = caplog.messages[0].split('\n') assert log[0] == "Queue configuration warnings:" assert log[1] == "+ q2: ignoring x (already assigned to a queue)"