From 7199f3beb8ee886dd2d19b18290fc8c9e490c3b3 Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Mon, 6 Feb 2023 12:05:07 +0000 Subject: [PATCH 01/57] update schema descriptions --- cylc/flow/network/schema.py | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/cylc/flow/network/schema.py b/cylc/flow/network/schema.py index b27576573ff..3c72640c5b7 100644 --- a/cylc/flow/network/schema.py +++ b/cylc/flow/network/schema.py @@ -1460,7 +1460,8 @@ class Flow(String): class Broadcast(Mutation): class Meta: description = sstrip(''' - Override `[runtime]` configurations in a running workflow. + Override `[runtime]` configurations in a workflow. + Valid for: ['running'] workflows. Uses for broadcast include making temporary changes to task behaviour, and task-to-downstream-task communication via @@ -1489,6 +1490,7 @@ class Meta: Note: a "clear" broadcast for a specific cycle or namespace does *not* clear all-cycle or all-namespace broadcasts. + ''') resolver = mutator @@ -1548,6 +1550,8 @@ class Meta: description = sstrip(''' Set workflow hold after cycle point. All tasks after this point will be held. + Valid for: ['running'] workflows. + ''') resolver = mutator @@ -1565,6 +1569,7 @@ class Pause(Mutation): class Meta: description = sstrip(''' Pause a workflow. + Valid for: ['running'] workflows. This suspends submission of tasks. ''') @@ -1580,6 +1585,7 @@ class Message(Mutation): class Meta: description = sstrip(''' Record task messages. + Valid for: ['running'] workflows. Send messages to: - The job stdout/stderr. @@ -1610,8 +1616,10 @@ class ReleaseHoldPoint(Mutation): class Meta: description = sstrip(''' Release all tasks and unset the workflow hold point, if set. + Valid for: ['running'] workflows. Held tasks do not submit their jobs even if ready to run. + ''') resolver = mutator @@ -1625,8 +1633,10 @@ class Resume(Mutation): class Meta: description = sstrip(''' Resume a paused workflow. + Valid for: ['paused', 'stopped'] workflows. See also the opposite command `pause`. + ''') resolver = mutator @@ -1639,7 +1649,8 @@ class Arguments: class Reload(Mutation): class Meta: description = sstrip(''' - Reload the configuration of a running workflow. + Reload the configuration of a workflow. + Valid for: ['running'] workflows. All settings including task definitions, with the exception of workflow log config, can be changed on reload. Changes to task @@ -1666,6 +1677,7 @@ class SetVerbosity(Mutation): class Meta: description = sstrip(''' Change the logging severity level of a running workflow. + Valid for: ['running'] workflows. Only messages at or above the chosen severity level will be logged; for example, if you choose `WARNING`, only warnings and critical @@ -1685,7 +1697,7 @@ class Meta: description = sstrip(''' Set the maximum graph distance (n) from an active node of the data-store graph window. - + Valid for: ['running'] workflows. ''') resolver = mutator @@ -1701,6 +1713,7 @@ class Meta: description = sstrip(f''' Tell a workflow to shut down or stop a specified flow from spawning any further. + Valid for: ['paused', 'running'] workflows. By default stopping workflows wait for submitted and running tasks to complete before shutting down. You can change this behaviour @@ -1711,6 +1724,7 @@ class Meta: Remaining task event handlers, job poll and kill commands, will be executed prior to shutdown, unless the stop mode is `{WorkflowStopMode.Now.name}`. + ''') resolver = mutator @@ -1739,6 +1753,7 @@ class ExtTrigger(Mutation): class Meta: description = sstrip(''' Report an external event message to a scheduler. + Valid for: ['running'] workflows. External triggers allow any program to send messages to the Cylc scheduler. Cylc can use such @@ -1759,6 +1774,7 @@ class Meta: Note: To manually trigger a task use "Trigger" not "ExtTrigger". + ''') resolver = partial(mutator, command='put_ext_trigger') @@ -1835,6 +1851,7 @@ class Hold(Mutation, TaskMutation): class Meta: description = sstrip(''' Hold tasks within a workflow. + Valid for: ['running'] workflows. Held tasks do not submit their jobs even if ready to run. ''') @@ -1845,6 +1862,7 @@ class Release(Mutation, TaskMutation): class Meta: description = sstrip(''' Release held tasks within a workflow. + Valid for: ['running'] workflows. See also the opposite command `hold`. ''') @@ -1856,6 +1874,8 @@ class Kill(Mutation, TaskMutation): class Meta: description = sstrip(''' Kill running or submitted jobs. + Valid for: ['running'] workflows. + ''') resolver = partial(mutator, command='kill_tasks') @@ -1864,6 +1884,7 @@ class Poll(Mutation, TaskMutation): class Meta: description = sstrip(''' Poll (query) jobs to verify and update their statuses. + Valid for: ['running'] workflows. This checks the job status file and queries the job runner on the job platform. @@ -1871,6 +1892,7 @@ class Meta: Pollable tasks are those in the n=0 window with an associated job ID, including incomplete finished tasks. + ''') resolver = partial(mutator, command='poll_tasks') @@ -1882,7 +1904,7 @@ class Remove(Mutation, TaskMutation): class Meta: description = sstrip(''' Remove one or more task instances from a running workflow. - + Valid for: ['running'] workflows. ''') resolver = partial(mutator, command='remove_tasks') @@ -1891,6 +1913,7 @@ class SetOutputs(Mutation, TaskMutation): class Meta: description = sstrip(''' Artificially mark task outputs as completed. + Valid for: ['running'] workflows. This allows you to manually intervene with Cylc's scheduling algorithm by artificially satisfying outputs of tasks. @@ -1912,6 +1935,7 @@ class Trigger(Mutation, TaskMutation): class Meta: description = sstrip(''' Manually trigger tasks. + Valid for: ['running'] workflows. Warning: waiting tasks that are queue-limited will be queued if triggered, to submit as normal when released by the queue; queued From daac5f75b062def456725603d0d9d7f7d2fd63ef Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Tue, 7 Feb 2023 13:17:45 +0000 Subject: [PATCH 02/57] Review feedback --- cylc/flow/network/schema.py | 49 +++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/cylc/flow/network/schema.py b/cylc/flow/network/schema.py index 3c72640c5b7..c49a775aa52 100644 --- a/cylc/flow/network/schema.py +++ b/cylc/flow/network/schema.py @@ -1461,7 +1461,6 @@ class Broadcast(Mutation): class Meta: description = sstrip(''' Override `[runtime]` configurations in a workflow. - Valid for: ['running'] workflows. Uses for broadcast include making temporary changes to task behaviour, and task-to-downstream-task communication via @@ -1490,7 +1489,7 @@ class Meta: Note: a "clear" broadcast for a specific cycle or namespace does *not* clear all-cycle or all-namespace broadcasts. - + Valid for: running workflows. ''') resolver = mutator @@ -1550,8 +1549,8 @@ class Meta: description = sstrip(''' Set workflow hold after cycle point. All tasks after this point will be held. - Valid for: ['running'] workflows. + Valid for: running workflows. ''') resolver = mutator @@ -1569,9 +1568,9 @@ class Pause(Mutation): class Meta: description = sstrip(''' Pause a workflow. - Valid for: ['running'] workflows. - This suspends submission of tasks. + + Valid for: running workflows. ''') resolver = mutator @@ -1585,7 +1584,6 @@ class Message(Mutation): class Meta: description = sstrip(''' Record task messages. - Valid for: ['running'] workflows. Send messages to: - The job stdout/stderr. @@ -1596,6 +1594,8 @@ class Meta: as success and failure. Applications run by jobs can use this command to report messages and to report registered task outputs. + + Valid for: running workflows. ''') resolver = partial(mutator, command='put_messages') @@ -1616,10 +1616,10 @@ class ReleaseHoldPoint(Mutation): class Meta: description = sstrip(''' Release all tasks and unset the workflow hold point, if set. - Valid for: ['running'] workflows. Held tasks do not submit their jobs even if ready to run. + Valid for: running workflows. ''') resolver = mutator @@ -1633,10 +1633,9 @@ class Resume(Mutation): class Meta: description = sstrip(''' Resume a paused workflow. - Valid for: ['paused', 'stopped'] workflows. - See also the opposite command `pause`. + Valid for: paused workflows. ''') resolver = mutator @@ -1650,7 +1649,6 @@ class Reload(Mutation): class Meta: description = sstrip(''' Reload the configuration of a workflow. - Valid for: ['running'] workflows. All settings including task definitions, with the exception of workflow log config, can be changed on reload. Changes to task @@ -1664,6 +1662,8 @@ class Meta: If the modified workflow config does not parse, failure to reload will be reported but no harm will be done to the running workflow. + + Valid for: running workflows. ''') resolver = partial(mutator, command='reload_workflow') @@ -1677,11 +1677,12 @@ class SetVerbosity(Mutation): class Meta: description = sstrip(''' Change the logging severity level of a running workflow. - Valid for: ['running'] workflows. Only messages at or above the chosen severity level will be logged; for example, if you choose `WARNING`, only warnings and critical messages will be logged. + + Valid for: running workflows. ''') resolver = mutator @@ -1697,7 +1698,8 @@ class Meta: description = sstrip(''' Set the maximum graph distance (n) from an active node of the data-store graph window. - Valid for: ['running'] workflows. + + Valid for: running workflows. ''') resolver = mutator @@ -1713,7 +1715,6 @@ class Meta: description = sstrip(f''' Tell a workflow to shut down or stop a specified flow from spawning any further. - Valid for: ['paused', 'running'] workflows. By default stopping workflows wait for submitted and running tasks to complete before shutting down. You can change this behaviour @@ -1725,6 +1726,7 @@ class Meta: be executed prior to shutdown, unless the stop mode is `{WorkflowStopMode.Now.name}`. + Valid for: paused, running workflows. ''') resolver = mutator @@ -1753,7 +1755,6 @@ class ExtTrigger(Mutation): class Meta: description = sstrip(''' Report an external event message to a scheduler. - Valid for: ['running'] workflows. External triggers allow any program to send messages to the Cylc scheduler. Cylc can use such @@ -1775,6 +1776,7 @@ class Meta: Note: To manually trigger a task use "Trigger" not "ExtTrigger". + Valid for: running workflows. ''') resolver = partial(mutator, command='put_ext_trigger') @@ -1851,9 +1853,10 @@ class Hold(Mutation, TaskMutation): class Meta: description = sstrip(''' Hold tasks within a workflow. - Valid for: ['running'] workflows. Held tasks do not submit their jobs even if ready to run. + + Valid for: running workflows. ''') resolver = mutator @@ -1862,9 +1865,10 @@ class Release(Mutation, TaskMutation): class Meta: description = sstrip(''' Release held tasks within a workflow. - Valid for: ['running'] workflows. See also the opposite command `hold`. + + Valid for: running workflows. ''') resolver = mutator @@ -1874,8 +1878,8 @@ class Kill(Mutation, TaskMutation): class Meta: description = sstrip(''' Kill running or submitted jobs. - Valid for: ['running'] workflows. + Valid for: running workflows. ''') resolver = partial(mutator, command='kill_tasks') @@ -1884,7 +1888,6 @@ class Poll(Mutation, TaskMutation): class Meta: description = sstrip(''' Poll (query) jobs to verify and update their statuses. - Valid for: ['running'] workflows. This checks the job status file and queries the job runner on the job platform. @@ -1893,6 +1896,7 @@ class Meta: an associated job ID, including incomplete finished tasks. + Valid for: running workflows. ''') resolver = partial(mutator, command='poll_tasks') @@ -1904,7 +1908,8 @@ class Remove(Mutation, TaskMutation): class Meta: description = sstrip(''' Remove one or more task instances from a running workflow. - Valid for: ['running'] workflows. + + Valid for: running workflows. ''') resolver = partial(mutator, command='remove_tasks') @@ -1913,12 +1918,13 @@ class SetOutputs(Mutation, TaskMutation): class Meta: description = sstrip(''' Artificially mark task outputs as completed. - Valid for: ['running'] workflows. This allows you to manually intervene with Cylc's scheduling algorithm by artificially satisfying outputs of tasks. By default this makes tasks appear as if they succeeded. + + Valid for: running workflows. ''') resolver = partial(mutator, command='force_spawn_children') @@ -1935,13 +1941,14 @@ class Trigger(Mutation, TaskMutation): class Meta: description = sstrip(''' Manually trigger tasks. - Valid for: ['running'] workflows. Warning: waiting tasks that are queue-limited will be queued if triggered, to submit as normal when released by the queue; queued tasks will submit immediately if triggered, even if that violates the queue limit (so you may need to trigger a queue-limited task twice to get it to submit immediately). + + Valid for: running workflows. ''') resolver = partial(mutator, command='force_trigger_tasks') From 31c51307bc54da5cae109b60d7dcb238031e8752 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Mon, 6 Feb 2023 12:05:11 +0000 Subject: [PATCH 03/57] data store: support unsatisfied ext_trigger --- cylc/flow/data_store_mgr.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cylc/flow/data_store_mgr.py b/cylc/flow/data_store_mgr.py index 819f9f1d41f..a7c4cd34afb 100644 --- a/cylc/flow/data_store_mgr.py +++ b/cylc/flow/data_store_mgr.py @@ -2094,7 +2094,7 @@ def delta_task_ext_trigger(self, itask, trig, message, satisfied): tp_delta.stamp = f'{tp_id}@{update_time}' ext_trigger = tp_delta.external_triggers[trig] ext_trigger.message = message - ext_trigger.satisfied = True + ext_trigger.satisfied = satisfied ext_trigger.time = update_time self.updates_pending = True From d50149980802b29e8535b67db6871326642f11d4 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 14 Feb 2023 11:04:15 +0000 Subject: [PATCH 04/57] Cylc lint fixes (#5363) Improve testing for family names being lower case - Modified original Regex to test for _any_ letter being lowercase - Added regex for FAMILY:trigger-all and trigger-any Check for Jinja2 correctly --- CHANGES.md | 8 +++++++ cylc/flow/scripts/lint.py | 41 +++++++++++++++++++++++++++++++-- tests/unit/scripts/test_lint.py | 16 +++++++++++-- 3 files changed, 61 insertions(+), 4 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index b863a4ac5e7..b18c85d3b04 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,6 +10,14 @@ creating a new release entry be sure to copy & paste the span tag with the updated. Only the first match gets replaced, so it's fine to leave the old ones in. --> +------------------------------------------------------------------------------- +## __cylc-8.1.2 (Upcoming)__ + +### Fixes + +[#5363](https://github.com/cylc/cylc-flow/pull/5363) Improvements and bugfixes +for `cylc lint`. + ------------------------------------------------------------------------------- ## __cylc-8.1.1 (Released 2023-01-31)__ diff --git a/cylc/flow/scripts/lint.py b/cylc/flow/scripts/lint.py index d3571297ded..fdcd8a67273 100755 --- a/cylc/flow/scripts/lint.py +++ b/cylc/flow/scripts/lint.py @@ -125,11 +125,35 @@ 'url': STYLE_GUIDE + 'trailing-whitespace', 'index': 6 }, - re.compile(r'inherit\s*=\s*[a-z].*$'): { + # Look for families both from inherit=FAMILY and FAMILY:trigger-all/any + re.compile(r'(inherit\s*=\s*.*[a-z].*)|(\w[a-z]\w:\w+?-a(ll|ny))'): { 'short': 'Family name contains lowercase characters.', 'url': STYLE_GUIDE + 'task-naming-conventions', 'index': 7 }, + re.compile(r'{[{%]'): { + 'short': JINJA2_FOUND_WITHOUT_SHEBANG, + 'url': '', + 'index': 8 + }, + re.compile(r'platform\s*=\s*\$\(.*?\)'): { + 'short': 'Host Selection Script may be redundant with platform', + 'url': ( + 'https://cylc.github.io/cylc-doc/stable/html/7-to-8/' + 'major-changes/platforms.html' + ), + 'index': 9 + }, + re.compile(r'platform\s*=\s*(`.*?`)'): { + 'short': 'Using backticks to invoke subshell is deprecated', + 'url': 'https://github.com/cylc/cylc-flow/issues/3825', + 'index': 10 + }, + re.compile(r'#.*?{[{%]'): { + 'short': 'Cylc will process commented Jinja2!', + 'url': '', + 'index': 11 + } # re.compile(r'^.{{maxlen},}'): { # 'short': 'line > {maxlen} characters.', # 'url': STYLE_GUIDE + 'line-length-and-continuation', @@ -170,6 +194,10 @@ '``global.cylc[platforms][]job runner``' ) }, + re.compile(r'host\s*=\s*(`.*?`)'): { + 'short': 'Using backticks to invoke subshell will fail at Cylc 8.', + 'url': 'https://github.com/cylc/cylc-flow/issues/3825' + } } RULESETS = ['728', 'style', 'all'] EXTRA_TOML_VALIDATION = { @@ -464,7 +492,16 @@ def check_cylc_file( ): continue - if check.findall(line) and not line.strip().startswith('#'): + if ( + check.findall(line) + and ( + not line.strip().startswith('#') + or ( + 'commented Jinja2!' in message['short'] + and check.findall(line) + ) + ) + ): count += 1 if modify: if message['url'].startswith('http'): diff --git a/tests/unit/scripts/test_lint.py b/tests/unit/scripts/test_lint.py index 2fb399b2098..2b993047f93 100644 --- a/tests/unit/scripts/test_lint.py +++ b/tests/unit/scripts/test_lint.py @@ -92,10 +92,10 @@ hold after point = 20220101T0000Z [[dependencies]] [[[R1]]] - graph = foo + graph = MyFaM:finish-all => remote [runtime] - [[MYFAM]] + [[MyFaM]] extra log files = True {% from 'cylc.flow' import LOG %} script = {{HELLOWORLD}} @@ -127,10 +127,14 @@ # Shouldn't object to a comment, unlike the terrible indents below: [[bad indent]] + inherit = MyFaM [[remote]] [meta] + [[and_another_thing]] + [[[remote]]] + host = `rose host-select thingy` """ @@ -141,11 +145,19 @@ [[dependencies]] +{% foo %} +{{foo}} +# {{quix}} + [runtime] [[foo]] inherit = hello [[[job]]] something\t + [[bar]] + platform = $(some-script foo) + [[baz]] + platform = `no backticks` """ LINT_TEST_FILE += ( From 331e066b9b62aac8b248bf20c8db3a36d4ebb332 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 16 Feb 2023 11:16:41 +0000 Subject: [PATCH 05/57] graphql: stabilise ID iteration order * A couple of ID lists were being converted to sets which caused the return order to become jumbled. * Switch to a function which removes duplicates whilst maintaining order * Closes #5366 --- cylc/flow/network/resolvers.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/cylc/flow/network/resolvers.py b/cylc/flow/network/resolvers.py index e551d773d63..b6effe143c6 100644 --- a/cylc/flow/network/resolvers.py +++ b/cylc/flow/network/resolvers.py @@ -385,7 +385,7 @@ async def get_nodes_all(self, node_type, args): async def get_nodes_by_ids(self, node_type, args): """Return protobuf node objects for given id.""" - nat_ids = set(args.get('native_ids', [])) + nat_ids = uniq(args.get('native_ids', [])) # Both cases just as common so 'if' not 'try' if 'sub_id' in args and args['delta_store']: flow_data = [ @@ -447,7 +447,7 @@ async def get_edges_all(self, args): async def get_edges_by_ids(self, args): """Return protobuf edge objects for given id.""" - nat_ids = set(args.get('native_ids', [])) + nat_ids = uniq(args.get('native_ids', [])) if 'sub_id' in args and args['delta_store']: flow_data = [ delta[args['delta_type']] @@ -470,6 +470,8 @@ async def get_nodes_edges(self, root_nodes, args): nodes = root_nodes node_ids = {n.id for n in root_nodes} edges = [] + # NOTE: we don't expect edges to be returned in definition order + # so it is ok to use `set` here edge_ids = set() # Setup for edgewise search. new_nodes = root_nodes @@ -492,6 +494,8 @@ async def get_nodes_edges(self, root_nodes, args): edges += new_edges # Gather nodes. # One of source or target will be in current set of nodes. + # NOTE: we don't expect node-edges to be returned in definition + # order so it is ok to use `set` here new_node_ids = set( [e.source for e in new_edges] + [e.target for e in new_edges]).difference(node_ids) @@ -518,6 +522,8 @@ async def subscribe_delta(self, root, info, args): yielded GraphQL subscription objects. """ + # NOTE: we don't expect workflows to be returned in definition order + # so it is ok to use `set` here workflow_ids = set(args.get('workflows', args.get('ids', ()))) sub_id = uuid4() info.variable_values['backend_sub_id'] = sub_id @@ -546,6 +552,8 @@ async def subscribe_delta(self, root, info, args): while True: if not workflow_ids: old_ids = w_ids + # NOTE: we don't expect workflows to be returned in + # definition order so it is ok to use `set` here w_ids = set(delta_queues.keys()) for remove_id in old_ids.difference(w_ids): if remove_id in self.delta_store[sub_id]: From 1584fd91426d50c4dc24d77f3cd264ae4933d019 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 16 Feb 2023 14:51:53 +0000 Subject: [PATCH 06/57] fix no rose vars in cylc view (#5367) * fix no rose vars in cylc view * Update CHANGES.md Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --------- Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- CHANGES.md | 5 ++++- cylc/flow/scripts/view.py | 2 ++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGES.md b/CHANGES.md index b18c85d3b04..67fab457844 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -11,10 +11,13 @@ updated. Only the first match gets replaced, so it's fine to leave the old ones in. --> ------------------------------------------------------------------------------- -## __cylc-8.1.2 (Upcoming)__ +## __cylc-8.1.2 (Coming Soon)__ ### Fixes +[#5367](https://github.com/cylc/cylc-flow/pull/5367) - Enable using +Rose options (`-O`, `-S` & `-D`) with `cylc view`. + [#5363](https://github.com/cylc/cylc-flow/pull/5363) Improvements and bugfixes for `cylc lint`. diff --git a/cylc/flow/scripts/view.py b/cylc/flow/scripts/view.py index 6c6cf905016..423480cc20f 100644 --- a/cylc/flow/scripts/view.py +++ b/cylc/flow/scripts/view.py @@ -98,6 +98,8 @@ def get_option_parser(): parser.add_option( *AGAINST_SOURCE_OPTION.args, **AGAINST_SOURCE_OPTION.kwargs) + parser.add_cylc_rose_options() + return parser From c8696bbe949dd515a05e8ee5969c62f2a27b5e42 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 17 Feb 2023 08:06:24 +0000 Subject: [PATCH 07/57] Fix a bug preventing `cylc vip --workflow-name=foo` from working. (#5349) * Fix a bug preventing `cylc vip --workflow-name=foo` from working. `--workflow-name foo` was fine. Caused by removal of unwanted sys.argv for Cylc VIP not removing items not exactly matching items in the Command line options arguments. * response to review --- CHANGES.md | 3 +++ cylc/flow/option_parsers.py | 4 ++++ tests/unit/test_option_parsers.py | 13 +++++++++++++ 3 files changed, 20 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 67fab457844..8b1801d16d2 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -15,6 +15,9 @@ ones in. --> ### Fixes +[#5349](https://github.com/cylc/cylc-flow/pull/5349) - Bugfix: `cylc vip --workflow-name` +only worked when used with a space, not an `=`. + [#5367](https://github.com/cylc/cylc-flow/pull/5367) - Enable using Rose options (`-O`, `-S` & `-D`) with `cylc view`. diff --git a/cylc/flow/option_parsers.py b/cylc/flow/option_parsers.py index e9145010b75..3a0ea9d6212 100644 --- a/cylc/flow/option_parsers.py +++ b/cylc/flow/option_parsers.py @@ -821,6 +821,7 @@ def cleanup_sysargv( for x in compound_script_opts } # Filter out non-cylc-play options. + args = [i.split('=')[0] for i in sys.argv] for unwanted_opt in (set(options.__dict__)) - set(script_opts_by_dest): for arg in compound_opts_by_dest[unwanted_opt].args: if arg in sys.argv: @@ -831,6 +832,9 @@ def cleanup_sysargv( not in ['store_true', 'store_false'] ): sys.argv.pop(index) + elif arg in args: + index = args.index(arg) + sys.argv.pop(index) # replace compound script name: sys.argv[1] = script_name diff --git a/tests/unit/test_option_parsers.py b/tests/unit/test_option_parsers.py index 332851d34c4..e3831a1daea 100644 --- a/tests/unit/test_option_parsers.py +++ b/tests/unit/test_option_parsers.py @@ -397,6 +397,19 @@ def test_combine_options(inputs, expect): 'play --foo something myworkflow'.split(), id='no path given' ), + param( + 'vip --bar=something'.split(), + { + 'script_name': 'play', + 'workflow_id': 'myworkflow', + 'compound_script_opts': [ + OptionSettings(['--bar', '-b'])], + 'script_opts': [], + 'source': './myworkflow', + }, + 'play myworkflow'.split(), + id='removes --key=value' + ), ] ) def test_cleanup_sysargv(monkeypatch, argv_before, kwargs, expect): From 19bcae02d5dea5252e289c2e8a38bea90890380a Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Tue, 8 Nov 2022 17:25:10 +0000 Subject: [PATCH 08/57] Replace actions/create-release with cylc/release-actions/create-release --- .github/workflows/2_auto_publish_release.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/2_auto_publish_release.yml b/.github/workflows/2_auto_publish_release.yml index 9ffa4015e06..ad9afbc56e0 100644 --- a/.github/workflows/2_auto_publish_release.yml +++ b/.github/workflows/2_auto_publish_release.yml @@ -48,7 +48,7 @@ jobs: - name: Publish GitHub release id: create-release - uses: actions/create-release@v1 + uses: cylc/release-actions/create-release@v1 env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} with: From 5f24b85100957338febc05f37d5ff171f4c60e49 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Mon, 20 Feb 2023 16:54:38 +0000 Subject: [PATCH 09/57] Prepare release 8.1.2 Workflow: Release stage 1 - create release PR (Cylc 8+ only), run: 22 --- CHANGES.md | 2 +- cylc/flow/__init__.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 8b1801d16d2..e9218295822 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -11,7 +11,7 @@ updated. Only the first match gets replaced, so it's fine to leave the old ones in. --> ------------------------------------------------------------------------------- -## __cylc-8.1.2 (Coming Soon)__ +## __cylc-8.1.2 (Released 2023-02-20)__ ### Fixes diff --git a/cylc/flow/__init__.py b/cylc/flow/__init__.py index 372c8b39560..5853885797c 100644 --- a/cylc/flow/__init__.py +++ b/cylc/flow/__init__.py @@ -46,7 +46,7 @@ def environ_init(): environ_init() -__version__ = '8.1.2.dev' +__version__ = '8.1.2' def iter_entry_points(entry_point_name): From 6218ab12f1ba942a0df9dce0bfcbeb4dc8de37c7 Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Mon, 6 Feb 2023 12:05:07 +0000 Subject: [PATCH 10/57] update schema descriptions --- cylc/flow/network/schema.py | 32 ++++++++++++++++++++++++++++---- 1 file changed, 28 insertions(+), 4 deletions(-) diff --git a/cylc/flow/network/schema.py b/cylc/flow/network/schema.py index b27576573ff..3c72640c5b7 100644 --- a/cylc/flow/network/schema.py +++ b/cylc/flow/network/schema.py @@ -1460,7 +1460,8 @@ class Flow(String): class Broadcast(Mutation): class Meta: description = sstrip(''' - Override `[runtime]` configurations in a running workflow. + Override `[runtime]` configurations in a workflow. + Valid for: ['running'] workflows. Uses for broadcast include making temporary changes to task behaviour, and task-to-downstream-task communication via @@ -1489,6 +1490,7 @@ class Meta: Note: a "clear" broadcast for a specific cycle or namespace does *not* clear all-cycle or all-namespace broadcasts. + ''') resolver = mutator @@ -1548,6 +1550,8 @@ class Meta: description = sstrip(''' Set workflow hold after cycle point. All tasks after this point will be held. + Valid for: ['running'] workflows. + ''') resolver = mutator @@ -1565,6 +1569,7 @@ class Pause(Mutation): class Meta: description = sstrip(''' Pause a workflow. + Valid for: ['running'] workflows. This suspends submission of tasks. ''') @@ -1580,6 +1585,7 @@ class Message(Mutation): class Meta: description = sstrip(''' Record task messages. + Valid for: ['running'] workflows. Send messages to: - The job stdout/stderr. @@ -1610,8 +1616,10 @@ class ReleaseHoldPoint(Mutation): class Meta: description = sstrip(''' Release all tasks and unset the workflow hold point, if set. + Valid for: ['running'] workflows. Held tasks do not submit their jobs even if ready to run. + ''') resolver = mutator @@ -1625,8 +1633,10 @@ class Resume(Mutation): class Meta: description = sstrip(''' Resume a paused workflow. + Valid for: ['paused', 'stopped'] workflows. See also the opposite command `pause`. + ''') resolver = mutator @@ -1639,7 +1649,8 @@ class Arguments: class Reload(Mutation): class Meta: description = sstrip(''' - Reload the configuration of a running workflow. + Reload the configuration of a workflow. + Valid for: ['running'] workflows. All settings including task definitions, with the exception of workflow log config, can be changed on reload. Changes to task @@ -1666,6 +1677,7 @@ class SetVerbosity(Mutation): class Meta: description = sstrip(''' Change the logging severity level of a running workflow. + Valid for: ['running'] workflows. Only messages at or above the chosen severity level will be logged; for example, if you choose `WARNING`, only warnings and critical @@ -1685,7 +1697,7 @@ class Meta: description = sstrip(''' Set the maximum graph distance (n) from an active node of the data-store graph window. - + Valid for: ['running'] workflows. ''') resolver = mutator @@ -1701,6 +1713,7 @@ class Meta: description = sstrip(f''' Tell a workflow to shut down or stop a specified flow from spawning any further. + Valid for: ['paused', 'running'] workflows. By default stopping workflows wait for submitted and running tasks to complete before shutting down. You can change this behaviour @@ -1711,6 +1724,7 @@ class Meta: Remaining task event handlers, job poll and kill commands, will be executed prior to shutdown, unless the stop mode is `{WorkflowStopMode.Now.name}`. + ''') resolver = mutator @@ -1739,6 +1753,7 @@ class ExtTrigger(Mutation): class Meta: description = sstrip(''' Report an external event message to a scheduler. + Valid for: ['running'] workflows. External triggers allow any program to send messages to the Cylc scheduler. Cylc can use such @@ -1759,6 +1774,7 @@ class Meta: Note: To manually trigger a task use "Trigger" not "ExtTrigger". + ''') resolver = partial(mutator, command='put_ext_trigger') @@ -1835,6 +1851,7 @@ class Hold(Mutation, TaskMutation): class Meta: description = sstrip(''' Hold tasks within a workflow. + Valid for: ['running'] workflows. Held tasks do not submit their jobs even if ready to run. ''') @@ -1845,6 +1862,7 @@ class Release(Mutation, TaskMutation): class Meta: description = sstrip(''' Release held tasks within a workflow. + Valid for: ['running'] workflows. See also the opposite command `hold`. ''') @@ -1856,6 +1874,8 @@ class Kill(Mutation, TaskMutation): class Meta: description = sstrip(''' Kill running or submitted jobs. + Valid for: ['running'] workflows. + ''') resolver = partial(mutator, command='kill_tasks') @@ -1864,6 +1884,7 @@ class Poll(Mutation, TaskMutation): class Meta: description = sstrip(''' Poll (query) jobs to verify and update their statuses. + Valid for: ['running'] workflows. This checks the job status file and queries the job runner on the job platform. @@ -1871,6 +1892,7 @@ class Meta: Pollable tasks are those in the n=0 window with an associated job ID, including incomplete finished tasks. + ''') resolver = partial(mutator, command='poll_tasks') @@ -1882,7 +1904,7 @@ class Remove(Mutation, TaskMutation): class Meta: description = sstrip(''' Remove one or more task instances from a running workflow. - + Valid for: ['running'] workflows. ''') resolver = partial(mutator, command='remove_tasks') @@ -1891,6 +1913,7 @@ class SetOutputs(Mutation, TaskMutation): class Meta: description = sstrip(''' Artificially mark task outputs as completed. + Valid for: ['running'] workflows. This allows you to manually intervene with Cylc's scheduling algorithm by artificially satisfying outputs of tasks. @@ -1912,6 +1935,7 @@ class Trigger(Mutation, TaskMutation): class Meta: description = sstrip(''' Manually trigger tasks. + Valid for: ['running'] workflows. Warning: waiting tasks that are queue-limited will be queued if triggered, to submit as normal when released by the queue; queued From fbd04ae8f9bad27579782a71c82482f0836ce4db Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Tue, 7 Feb 2023 13:17:45 +0000 Subject: [PATCH 11/57] Review feedback --- cylc/flow/network/schema.py | 49 +++++++++++++++++++++---------------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/cylc/flow/network/schema.py b/cylc/flow/network/schema.py index 3c72640c5b7..c49a775aa52 100644 --- a/cylc/flow/network/schema.py +++ b/cylc/flow/network/schema.py @@ -1461,7 +1461,6 @@ class Broadcast(Mutation): class Meta: description = sstrip(''' Override `[runtime]` configurations in a workflow. - Valid for: ['running'] workflows. Uses for broadcast include making temporary changes to task behaviour, and task-to-downstream-task communication via @@ -1490,7 +1489,7 @@ class Meta: Note: a "clear" broadcast for a specific cycle or namespace does *not* clear all-cycle or all-namespace broadcasts. - + Valid for: running workflows. ''') resolver = mutator @@ -1550,8 +1549,8 @@ class Meta: description = sstrip(''' Set workflow hold after cycle point. All tasks after this point will be held. - Valid for: ['running'] workflows. + Valid for: running workflows. ''') resolver = mutator @@ -1569,9 +1568,9 @@ class Pause(Mutation): class Meta: description = sstrip(''' Pause a workflow. - Valid for: ['running'] workflows. - This suspends submission of tasks. + + Valid for: running workflows. ''') resolver = mutator @@ -1585,7 +1584,6 @@ class Message(Mutation): class Meta: description = sstrip(''' Record task messages. - Valid for: ['running'] workflows. Send messages to: - The job stdout/stderr. @@ -1596,6 +1594,8 @@ class Meta: as success and failure. Applications run by jobs can use this command to report messages and to report registered task outputs. + + Valid for: running workflows. ''') resolver = partial(mutator, command='put_messages') @@ -1616,10 +1616,10 @@ class ReleaseHoldPoint(Mutation): class Meta: description = sstrip(''' Release all tasks and unset the workflow hold point, if set. - Valid for: ['running'] workflows. Held tasks do not submit their jobs even if ready to run. + Valid for: running workflows. ''') resolver = mutator @@ -1633,10 +1633,9 @@ class Resume(Mutation): class Meta: description = sstrip(''' Resume a paused workflow. - Valid for: ['paused', 'stopped'] workflows. - See also the opposite command `pause`. + Valid for: paused workflows. ''') resolver = mutator @@ -1650,7 +1649,6 @@ class Reload(Mutation): class Meta: description = sstrip(''' Reload the configuration of a workflow. - Valid for: ['running'] workflows. All settings including task definitions, with the exception of workflow log config, can be changed on reload. Changes to task @@ -1664,6 +1662,8 @@ class Meta: If the modified workflow config does not parse, failure to reload will be reported but no harm will be done to the running workflow. + + Valid for: running workflows. ''') resolver = partial(mutator, command='reload_workflow') @@ -1677,11 +1677,12 @@ class SetVerbosity(Mutation): class Meta: description = sstrip(''' Change the logging severity level of a running workflow. - Valid for: ['running'] workflows. Only messages at or above the chosen severity level will be logged; for example, if you choose `WARNING`, only warnings and critical messages will be logged. + + Valid for: running workflows. ''') resolver = mutator @@ -1697,7 +1698,8 @@ class Meta: description = sstrip(''' Set the maximum graph distance (n) from an active node of the data-store graph window. - Valid for: ['running'] workflows. + + Valid for: running workflows. ''') resolver = mutator @@ -1713,7 +1715,6 @@ class Meta: description = sstrip(f''' Tell a workflow to shut down or stop a specified flow from spawning any further. - Valid for: ['paused', 'running'] workflows. By default stopping workflows wait for submitted and running tasks to complete before shutting down. You can change this behaviour @@ -1725,6 +1726,7 @@ class Meta: be executed prior to shutdown, unless the stop mode is `{WorkflowStopMode.Now.name}`. + Valid for: paused, running workflows. ''') resolver = mutator @@ -1753,7 +1755,6 @@ class ExtTrigger(Mutation): class Meta: description = sstrip(''' Report an external event message to a scheduler. - Valid for: ['running'] workflows. External triggers allow any program to send messages to the Cylc scheduler. Cylc can use such @@ -1775,6 +1776,7 @@ class Meta: Note: To manually trigger a task use "Trigger" not "ExtTrigger". + Valid for: running workflows. ''') resolver = partial(mutator, command='put_ext_trigger') @@ -1851,9 +1853,10 @@ class Hold(Mutation, TaskMutation): class Meta: description = sstrip(''' Hold tasks within a workflow. - Valid for: ['running'] workflows. Held tasks do not submit their jobs even if ready to run. + + Valid for: running workflows. ''') resolver = mutator @@ -1862,9 +1865,10 @@ class Release(Mutation, TaskMutation): class Meta: description = sstrip(''' Release held tasks within a workflow. - Valid for: ['running'] workflows. See also the opposite command `hold`. + + Valid for: running workflows. ''') resolver = mutator @@ -1874,8 +1878,8 @@ class Kill(Mutation, TaskMutation): class Meta: description = sstrip(''' Kill running or submitted jobs. - Valid for: ['running'] workflows. + Valid for: running workflows. ''') resolver = partial(mutator, command='kill_tasks') @@ -1884,7 +1888,6 @@ class Poll(Mutation, TaskMutation): class Meta: description = sstrip(''' Poll (query) jobs to verify and update their statuses. - Valid for: ['running'] workflows. This checks the job status file and queries the job runner on the job platform. @@ -1893,6 +1896,7 @@ class Meta: an associated job ID, including incomplete finished tasks. + Valid for: running workflows. ''') resolver = partial(mutator, command='poll_tasks') @@ -1904,7 +1908,8 @@ class Remove(Mutation, TaskMutation): class Meta: description = sstrip(''' Remove one or more task instances from a running workflow. - Valid for: ['running'] workflows. + + Valid for: running workflows. ''') resolver = partial(mutator, command='remove_tasks') @@ -1913,12 +1918,13 @@ class SetOutputs(Mutation, TaskMutation): class Meta: description = sstrip(''' Artificially mark task outputs as completed. - Valid for: ['running'] workflows. This allows you to manually intervene with Cylc's scheduling algorithm by artificially satisfying outputs of tasks. By default this makes tasks appear as if they succeeded. + + Valid for: running workflows. ''') resolver = partial(mutator, command='force_spawn_children') @@ -1935,13 +1941,14 @@ class Trigger(Mutation, TaskMutation): class Meta: description = sstrip(''' Manually trigger tasks. - Valid for: ['running'] workflows. Warning: waiting tasks that are queue-limited will be queued if triggered, to submit as normal when released by the queue; queued tasks will submit immediately if triggered, even if that violates the queue limit (so you may need to trigger a queue-limited task twice to get it to submit immediately). + + Valid for: running workflows. ''') resolver = partial(mutator, command='force_trigger_tasks') From 58e7fc4cd6c2d810777c59065b8064df929dde03 Mon Sep 17 00:00:00 2001 From: Melanie Hall <37735232+datamel@users.noreply.github.com> Date: Fri, 24 Feb 2023 11:41:35 +0000 Subject: [PATCH 12/57] Apply suggestions from code review Co-authored-by: Oliver Sanders --- cylc/flow/network/schema.py | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/cylc/flow/network/schema.py b/cylc/flow/network/schema.py index c49a775aa52..e9fe55c0060 100644 --- a/cylc/flow/network/schema.py +++ b/cylc/flow/network/schema.py @@ -1460,7 +1460,7 @@ class Flow(String): class Broadcast(Mutation): class Meta: description = sstrip(''' - Override `[runtime]` configurations in a workflow. + Override `[runtime]` configurations in a running workflow. Uses for broadcast include making temporary changes to task behaviour, and task-to-downstream-task communication via @@ -1489,7 +1489,7 @@ class Meta: Note: a "clear" broadcast for a specific cycle or namespace does *not* clear all-cycle or all-namespace broadcasts. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = mutator @@ -1550,7 +1550,7 @@ class Meta: Set workflow hold after cycle point. All tasks after this point will be held. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = mutator @@ -1595,7 +1595,7 @@ class Meta: this command to report messages and to report registered task outputs. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = partial(mutator, command='put_messages') @@ -1619,7 +1619,7 @@ class Meta: Held tasks do not submit their jobs even if ready to run. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = mutator @@ -1663,7 +1663,7 @@ class Meta: If the modified workflow config does not parse, failure to reload will be reported but no harm will be done to the running workflow. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = partial(mutator, command='reload_workflow') @@ -1682,7 +1682,7 @@ class Meta: for example, if you choose `WARNING`, only warnings and critical messages will be logged. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = mutator @@ -1699,7 +1699,7 @@ class Meta: Set the maximum graph distance (n) from an active node of the data-store graph window. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = mutator @@ -1776,7 +1776,7 @@ class Meta: Note: To manually trigger a task use "Trigger" not "ExtTrigger". - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = partial(mutator, command='put_ext_trigger') @@ -1856,7 +1856,7 @@ class Meta: Held tasks do not submit their jobs even if ready to run. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = mutator @@ -1868,7 +1868,7 @@ class Meta: See also the opposite command `hold`. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = mutator @@ -1879,7 +1879,7 @@ class Meta: description = sstrip(''' Kill running or submitted jobs. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = partial(mutator, command='kill_tasks') @@ -1896,7 +1896,7 @@ class Meta: an associated job ID, including incomplete finished tasks. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = partial(mutator, command='poll_tasks') @@ -1909,7 +1909,7 @@ class Meta: description = sstrip(''' Remove one or more task instances from a running workflow. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = partial(mutator, command='remove_tasks') @@ -1924,7 +1924,7 @@ class Meta: By default this makes tasks appear as if they succeeded. - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = partial(mutator, command='force_spawn_children') @@ -1948,7 +1948,7 @@ class Meta: the queue limit (so you may need to trigger a queue-limited task twice to get it to submit immediately). - Valid for: running workflows. + Valid for: paused, running workflows. ''') resolver = partial(mutator, command='force_trigger_tasks') From d67a0582db79c0a3f2bd38261dcd42b06296b9c2 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Mon, 27 Feb 2023 12:00:50 +0000 Subject: [PATCH 13/57] Bump version 8.1.3.dev --- cylc/flow/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cylc/flow/__init__.py b/cylc/flow/__init__.py index 5853885797c..dd44be05591 100644 --- a/cylc/flow/__init__.py +++ b/cylc/flow/__init__.py @@ -46,7 +46,7 @@ def environ_init(): environ_init() -__version__ = '8.1.2' +__version__ = '8.1.3.dev' def iter_entry_points(entry_point_name): From 74056a987d471eefea408bd1ebc9f24793bb6c05 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Mon, 27 Feb 2023 16:40:19 +0000 Subject: [PATCH 14/57] install: sort workflows by ID when listing (#5378) --- cylc/flow/scripts/install.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/cylc/flow/scripts/install.py b/cylc/flow/scripts/install.py index b33856ad9ad..574c2c0e3a7 100755 --- a/cylc/flow/scripts/install.py +++ b/cylc/flow/scripts/install.py @@ -215,12 +215,16 @@ async def scan(wf_name: str, ping: bool = True) -> None: 'source': False, 'ping': ping, # get status of scanned workflows }) - active = [ - item async for item in get_pipe( - opts, None, - scan_dir=get_workflow_run_dir(wf_name) # restricted scan - ) - ] + active = sorted( + [ + item async for item in get_pipe( + opts, + None, + scan_dir=get_workflow_run_dir(wf_name) # restricted scan + ) + ], + key=lambda flow: flow['name'] + ) if active: n = len(active) grammar = ( From 5b8cadc25ff26b9c553c2a8c910cfd273667410e Mon Sep 17 00:00:00 2001 From: Melanie Hall <37735232+datamel@users.noreply.github.com> Date: Tue, 28 Feb 2023 12:16:02 +0000 Subject: [PATCH 15/57] Update cylc/flow/network/schema.py --- cylc/flow/network/schema.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cylc/flow/network/schema.py b/cylc/flow/network/schema.py index e9fe55c0060..59919f2c3cf 100644 --- a/cylc/flow/network/schema.py +++ b/cylc/flow/network/schema.py @@ -1648,7 +1648,7 @@ class Arguments: class Reload(Mutation): class Meta: description = sstrip(''' - Reload the configuration of a workflow. + Reload the configuration of a running workflow. All settings including task definitions, with the exception of workflow log config, can be changed on reload. Changes to task From 72b2fd28012e49334ad9c1bdab065704636c2667 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Tue, 28 Feb 2023 13:16:47 +0000 Subject: [PATCH 16/57] Fix set-verbosity GraphQL bug --- cylc/flow/__init__.py | 2 +- cylc/flow/network/schema.py | 16 +++++++------ cylc/flow/scripts/set_verbosity.py | 10 ++++---- tests/functional/cli/03-set-verbosity.t | 32 +++++++++++++++++++++---- 4 files changed, 41 insertions(+), 19 deletions(-) diff --git a/cylc/flow/__init__.py b/cylc/flow/__init__.py index dd44be05591..d5c6af6bcbf 100644 --- a/cylc/flow/__init__.py +++ b/cylc/flow/__init__.py @@ -26,12 +26,12 @@ LOG.addHandler(logging.NullHandler()) LOG_LEVELS = { + "DEBUG": logging.DEBUG, "INFO": logging.INFO, "NORMAL": logging.INFO, "WARNING": logging.WARNING, "ERROR": logging.ERROR, "CRITICAL": logging.CRITICAL, - "DEBUG": logging.DEBUG, } diff --git a/cylc/flow/network/schema.py b/cylc/flow/network/schema.py index b27576573ff..dea6fbe916d 100644 --- a/cylc/flow/network/schema.py +++ b/cylc/flow/network/schema.py @@ -19,7 +19,6 @@ from copy import deepcopy from functools import partial import json -import logging from operator import attrgetter from textwrap import dedent from typing import ( @@ -38,6 +37,7 @@ from graphene.types.generic import GenericScalar from graphene.utils.str_converters import to_snake_case +from cylc.flow import LOG_LEVELS from cylc.flow.broadcast_mgr import ALL_CYCLE_POINTS_STRS, addict from cylc.flow.flow_mgr import FLOW_ALL, FLOW_NEW, FLOW_NONE from cylc.flow.id import Tokens @@ -1426,9 +1426,11 @@ class TimePoint(String): LogLevels = graphene.Enum( 'LogLevels', - list(logging._nameToLevel.items()), - description=lambda x: f'Python logging level: {x.name} = {x.value}.' - if x else '' + list(LOG_LEVELS.items()), + description=lambda x: ( + f'Logging level: {x.name} = {x.value}.' + if x else '' + ) ) @@ -1667,9 +1669,9 @@ class Meta: description = sstrip(''' Change the logging severity level of a running workflow. - Only messages at or above the chosen severity level will be logged; - for example, if you choose `WARNING`, only warnings and critical - messages will be logged. + Only messages at or above the chosen severity level will be logged. + For example, if you choose `WARNING`, only warning, error and + critical level messages will be logged. ''') resolver = mutator diff --git a/cylc/flow/scripts/set_verbosity.py b/cylc/flow/scripts/set_verbosity.py index 5f941383425..a87655d2267 100755 --- a/cylc/flow/scripts/set_verbosity.py +++ b/cylc/flow/scripts/set_verbosity.py @@ -66,7 +66,7 @@ def get_option_parser() -> COP: return parser -async def run(options: 'Values', severity, workflow_id) -> None: +async def run(options: 'Values', severity: str, workflow_id: str) -> None: pclient = get_client(workflow_id, timeout=options.comms_timeout) mutation_kwargs = { @@ -81,11 +81,9 @@ async def run(options: 'Values', severity, workflow_id) -> None: @cli_function(get_option_parser) -def main(parser: COP, options: 'Values', severity_str: str, *ids) -> None: - try: - severity = LOG_LEVELS[severity_str] - except KeyError: - raise InputError("Illegal logging level, %s" % severity_str) +def main(parser: COP, options: 'Values', severity: str, *ids: str) -> None: + if severity not in LOG_LEVELS: + raise InputError(f"Illegal logging level, {severity}") call_multi( partial(run, options, severity), *ids, diff --git a/tests/functional/cli/03-set-verbosity.t b/tests/functional/cli/03-set-verbosity.t index df5441e95d2..a93b20e7ea5 100755 --- a/tests/functional/cli/03-set-verbosity.t +++ b/tests/functional/cli/03-set-verbosity.t @@ -15,9 +15,31 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . #------------------------------------------------------------------------------- -# Test "cylc set-verbosity" with a bad verbosity level. +# Test "cylc set-verbosity" . "$(dirname "$0")/test_header" -set_test_number 2 -run_fail "${TEST_NAME_BASE}" cylc set-verbosity duck quack -grep_ok 'InputError: Illegal logging level, duck' "${TEST_NAME_BASE}.stderr" -exit +set_test_number 6 + +# Test illegal log level +TEST_NAME="${TEST_NAME_BASE}-bad" +run_fail "$TEST_NAME" cylc set-verbosity duck quack +grep_ok 'InputError: Illegal logging level, duck' "${TEST_NAME}.stderr" + +# Test good log level +TEST_NAME="${TEST_NAME_BASE}-good" +init_workflow "${TEST_NAME_BASE}" << '__FLOW__' +[scheduler] + allow implicit tasks = True +[scheduling] + [[graph]] + R1 = andor +__FLOW__ + +run_ok "${TEST_NAME}-validate" cylc validate "$WORKFLOW_NAME" +workflow_run_ok "${TEST_NAME}-run" cylc play --pause "$WORKFLOW_NAME" + +run_ok "$TEST_NAME" cylc set-verbosity DEBUG "$WORKFLOW_NAME" +log_scan "${TEST_NAME}-grep" "${WORKFLOW_RUN_DIR}/log/scheduler/log" 5 1 \ + 'Command succeeded: set_verbosity' + +cylc stop "$WORKFLOW_NAME" +purge From 1f038a8b05b3fb0e64e0d908021d7a64ed3dc6ef Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Tue, 28 Feb 2023 16:05:27 +0000 Subject: [PATCH 17/57] Update changelog [skip ci] --- CHANGES.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index e9218295822..97637a6371b 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -9,6 +9,13 @@ creating a new release entry be sure to copy & paste the span tag with the `actions:bind` attribute, which is used by a regex to find the text to be updated. Only the first match gets replaced, so it's fine to leave the old ones in. --> +------------------------------------------------------------------------------- +## __cylc-8.1.3 (Upcoming)__ + +### Fixes + +[#5384](https://github.com/cylc/cylc-flow/pull/5384) - +Fixes `cylc set-verbosity`. ------------------------------------------------------------------------------- ## __cylc-8.1.2 (Released 2023-02-20)__ From 045469bedff8f37e79bd627dc9fc2e275904a6b7 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Thu, 2 Mar 2023 13:36:04 +0000 Subject: [PATCH 18/57] Fix possible IndexError during shutdown --- cylc/flow/exceptions.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/cylc/flow/exceptions.py b/cylc/flow/exceptions.py index 04e0029862c..94d90f25066 100644 --- a/cylc/flow/exceptions.py +++ b/cylc/flow/exceptions.py @@ -19,6 +19,7 @@ from textwrap import wrap from typing import ( Callable, + Dict, Iterable, NoReturn, Optional, @@ -366,11 +367,10 @@ class PlatformLookupError(CylcConfigError): class HostSelectException(CylcError): """No hosts could be selected from the provided configuration.""" - def __init__(self, data): + def __init__(self, data: Dict[str, dict]): self.data = data - CylcError.__init__(self) - def __str__(self): + def __str__(self) -> str: ret = 'Could not select host from:' for host, data in sorted(self.data.items()): if host != 'ranking': @@ -430,7 +430,6 @@ class NoHostsError(CylcError): """None of the hosts of a given platform were reachable.""" def __init__(self, platform): self.platform_name = platform['name'] - super().__init__() def __str__(self): return f'Unable to find valid host for {self.platform_name}' @@ -440,7 +439,6 @@ class NoPlatformsError(CylcError): """None of the platforms of a given group were reachable.""" def __init__(self, platform_group): self.platform_group = platform_group - super().__init__() def __str__(self): return f'Unable to find a platform from group {self.platform_group}.' From 7766238405f655506c93d567df9d38dcef44fbd1 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 2 Mar 2023 17:17:12 +0000 Subject: [PATCH 19/57] data_store: fix possible traceback with NoneType items * Closes #5388 * Even though `None` is a valid value for an `optional` field in Protobuf, and even though you can initiate these fields with `None` values, you cannot later set them to `None`. --- CHANGES.md | 3 +++ cylc/flow/data_store_mgr.py | 49 +++++++++++++++++++++++++++++++------ 2 files changed, 45 insertions(+), 7 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 97637a6371b..93ef3d154ba 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -17,6 +17,9 @@ ones in. --> [#5384](https://github.com/cylc/cylc-flow/pull/5384) - Fixes `cylc set-verbosity`. +[#5394](https://github.com/cylc/cylc-flow/pull/5394) - +Fixes a possible scheduler traceback observed with remote task polling. + ------------------------------------------------------------------------------- ## __cylc-8.1.2 (Released 2023-02-20)__ diff --git a/cylc/flow/data_store_mgr.py b/cylc/flow/data_store_mgr.py index a7c4cd34afb..0d5eb64bf37 100644 --- a/cylc/flow/data_store_mgr.py +++ b/cylc/flow/data_store_mgr.py @@ -180,6 +180,41 @@ } +def setbuff(obj, key, value): + """Set an attribute on a protobuf object. + + Although `None` is a valid value for an `optional` field in protobuf e.g: + + >>> job = PbJob(job_id=None) + + Attempting to set a field to none after initiation results in error: + + >>> job.job_id = None + Traceback (most recent call last): + TypeError: ... + + For safety this method only sets the attribute if the value is not None. + + Note: + If the above doctest fails, then the behaviour of protobuf has changed + and this wrapper might not be necessary any more. + + See: + https://github.com/cylc/cylc-flow/issues/5388 + + Example: + >>> from types import SimpleNamespace + >>> obj = SimpleNamespace() + >>> setbuff(obj, 'a', 1); obj + namespace(a=1) + >>> setbuff(obj, 'b', None); obj + namespace(a=1) + + """ + if value is not None: + setattr(obj, key, value) + + def generate_checksum(in_strings): """Generate cross platform & python checksum from strings.""" # can't use hash(), it's not the same across 32-64bit or python invocations @@ -546,7 +581,7 @@ def generate_definition_elements(self): user_defined_meta = {} for key, val in dict(tdef.describe()).items(): if key in ['title', 'description', 'URL']: - setattr(task.meta, key, val) + setbuff(task.meta, key, val) else: user_defined_meta[key] = val task.meta.user_defined = json.dumps(user_defined_meta) @@ -581,7 +616,7 @@ def generate_definition_elements(self): user_defined_meta = {} for key, val in famcfg.get('meta', {}).items(): if key in ['title', 'description', 'URL']: - setattr(family.meta, key, val) + setbuff(family.meta, key, val) else: user_defined_meta[key] = val family.meta.user_defined = json.dumps(user_defined_meta) @@ -619,7 +654,7 @@ def generate_definition_elements(self): user_defined_meta = {} for key, val in config.cfg['meta'].items(): if key in ['title', 'description', 'URL']: - setattr(workflow.meta, key, val) + setbuff(workflow.meta, key, val) else: user_defined_meta[key] = val workflow.meta.user_defined = json.dumps(user_defined_meta) @@ -633,7 +668,7 @@ def generate_definition_elements(self): else: time_zone_info = TIME_ZONE_LOCAL_INFO for key, val in time_zone_info.items(): - setattr(workflow.time_zone_info, key, val) + setbuff(workflow.time_zone_info, key, val) workflow.run_mode = config.run_mode() workflow.cycling_mode = config.cfg['scheduling']['cycling mode'] @@ -1033,7 +1068,7 @@ def generate_orphan_task(self, itask): user_defined_meta = {} for key, val in dict(tdef.describe()).items(): if key in ['title', 'description', 'URL']: - setattr(task.meta, key, val) + setbuff(task.meta, key, val) else: user_defined_meta[key] = val task.meta.user_defined = json.dumps(user_defined_meta) @@ -2157,7 +2192,7 @@ def delta_job_attr(self, job_d, attr_key, attr_val): if not job: return j_delta = PbJob(stamp=f'{j_id}@{time()}') - setattr(j_delta, attr_key, attr_val) + setbuff(j_delta, attr_key, attr_val) self.updated[JOBS].setdefault( j_id, PbJob(id=j_id) @@ -2199,7 +2234,7 @@ def delta_job_time(self, job_d, event_key, time_str=None): return j_delta = PbJob(stamp=f'{j_id}@{time()}') time_attr = f'{event_key}_time' - setattr(j_delta, time_attr, time_str) + setbuff(j_delta, time_attr, time_str) self.updated[JOBS].setdefault( j_id, PbJob(id=j_id) From 0ce5b0317268e04d9b566cae99f9468e8256577a Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Tue, 7 Mar 2023 10:11:38 +0000 Subject: [PATCH 20/57] Fix mypy error --- cylc/flow/loggingutil.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cylc/flow/loggingutil.py b/cylc/flow/loggingutil.py index ca338f13acb..35beba3f583 100644 --- a/cylc/flow/loggingutil.py +++ b/cylc/flow/loggingutil.py @@ -353,7 +353,7 @@ def setup_segregated_log_streams( stdout_handler = logging.StreamHandler(sys.stdout) stdout_handler.setLevel(logging.DEBUG) # Filter out >= warnings from stdout - stdout_handler.addFilter(lambda rec: int(rec.levelno < logging.WARNING)) + stdout_handler.addFilter(lambda rec: rec.levelno < logging.WARNING) if stderr_handler.formatter: stdout_handler.setFormatter(stderr_handler.formatter) logger.addHandler(stdout_handler) From a92efaee57bfaef2bf95bbd5099088075ea013e8 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Wed, 8 Mar 2023 10:19:09 +0000 Subject: [PATCH 21/57] Restore default for PBS max job name len. (#5386) Restore default for PBS max job name len. Co-authored-by: Oliver Sanders Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- CHANGES.md | 4 +++ cylc/flow/job_runner_handlers/pbs.py | 5 ++- tests/unit/job_runner_handlers/test_pbs.py | 36 +++++++++++++++++++--- 3 files changed, 40 insertions(+), 5 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 97637a6371b..f47d0fc1cc7 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -17,6 +17,10 @@ ones in. --> [#5384](https://github.com/cylc/cylc-flow/pull/5384) - Fixes `cylc set-verbosity`. +[#5386](https://github.com/cylc/cylc-flow/pull/5386) Fix bug where +absence of `job name length maximum` in PBS platform settings would cause +Cylc to crash when preparing the job script. + ------------------------------------------------------------------------------- ## __cylc-8.1.2 (Released 2023-02-20)__ diff --git a/cylc/flow/job_runner_handlers/pbs.py b/cylc/flow/job_runner_handlers/pbs.py index 37545c5b967..c28ce3899ff 100644 --- a/cylc/flow/job_runner_handlers/pbs.py +++ b/cylc/flow/job_runner_handlers/pbs.py @@ -96,7 +96,10 @@ def format_directives(self, job_conf): f'{tokens["task"]}.{tokens["cycle"]}' f".{job_conf['workflow_name'].replace('/', '-')}" ) - job_name_len_max = job_conf['platform']["job name length maximum"] + job_name_len_max = job_conf['platform'].get( + "job name length maximum", + self.JOB_NAME_LEN_MAX + ) if job_name_len_max: directives["-N"] = directives["-N"][0:job_name_len_max] diff --git a/tests/unit/job_runner_handlers/test_pbs.py b/tests/unit/job_runner_handlers/test_pbs.py index 0c65e6192f2..ac7c884046b 100644 --- a/tests/unit/job_runner_handlers/test_pbs.py +++ b/tests/unit/job_runner_handlers/test_pbs.py @@ -16,13 +16,19 @@ import pytest -from cylc.flow.job_runner_handlers.pbs import JOB_RUNNER_HANDLER +from cylc.flow.job_runner_handlers.pbs import ( + JOB_RUNNER_HANDLER, + PBSHandler +) + + +VERY_LONG_STR = 'x' * 240 @pytest.mark.parametrize( 'job_conf,lines', [ - ( # basic + pytest.param( { 'directives': {}, 'execution_time_limit': 180, @@ -40,8 +46,28 @@ '#PBS -e cylc-run/chop/log/job/1/axe/01/job.err', '#PBS -l walltime=180', ], + id='basic' + ), + pytest.param( + { + 'directives': {}, + 'execution_time_limit': 180, + 'job_file_path': 'cylc-run/chop/log/job/1/axe/01/job', + 'workflow_name': 'chop', + 'task_id': VERY_LONG_STR, + 'platform': { + 'job runner': 'pbs', + } + }, + [ + f'#PBS -N None.{VERY_LONG_STR[:PBSHandler.JOB_NAME_LEN_MAX - 5]}', + '#PBS -o cylc-run/chop/log/job/1/axe/01/job.out', + '#PBS -e cylc-run/chop/log/job/1/axe/01/job.err', + '#PBS -l walltime=180', + ], + id='long-job-name' ), - ( # super short job name length maximum + pytest.param( { 'directives': {}, 'execution_time_limit': 180, @@ -59,8 +85,9 @@ '#PBS -e cylc-run/chop/log/job/1/axe/01/job.err', '#PBS -l walltime=180', ], + id='truncate-job-name' ), - ( # some useful directives + pytest.param( { 'directives': { '-q': 'forever', @@ -85,6 +112,7 @@ '#PBS -V', '#PBS -l mem=256gb', ], + id='custom-directives' ), ], ) From 5de4c2607bc961bdb0661cfd08010855d5aa70a0 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Wed, 1 Mar 2023 17:05:31 +0000 Subject: [PATCH 22/57] job runners: document the handler interface * Move existing documentation into a class for easier auto-documentation. * Pull in a little content from cylc-doc and attempt to pad things out a little. * Pull in lists of fields for `job_conf` and `submit_opts` obtained from the codebase. --- .../flow/job_runner_handlers/documentation.py | 416 ++++++++++++++++++ cylc/flow/job_runner_mgr.py | 98 +---- cylc/flow/task_job_mgr.py | 3 +- 3 files changed, 422 insertions(+), 95 deletions(-) create mode 100644 cylc/flow/job_runner_handlers/documentation.py diff --git a/cylc/flow/job_runner_handlers/documentation.py b/cylc/flow/job_runner_handlers/documentation.py new file mode 100644 index 00000000000..ff931ff25ea --- /dev/null +++ b/cylc/flow/job_runner_handlers/documentation.py @@ -0,0 +1,416 @@ +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) 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 . + +""" +This file is used to auto-generate some reference documentation for the +job runner plugin interface. + +Note the class contained here is just for documentation purposes and is +not intended to be subclassed. +""" + +import re +from typing import ( + Iterable, + List, + Tuple, +) + + +class ExampleHandler(): + """Documentation for writing job runner handlers. + + Cylc can submit jobs to a number of different job runners + (aka batch systems) e.g. Slurm and PBS. For a list of built-in integrations + see :ref:`AvailableMethods`. + + If the job runner you require is not on this list, Cylc provides a generic + interface for writing your own integration. + + Defining a new job runner handler requires a little Python programming. Use + the built-in handlers + (e.g. :py:mod:`cylc.flow.job_runner_handlers.background`) as examples. + + .. _where to put job runner handler modules: + + .. rubric:: Installation + + Custom job runner handlers must be installed on workflow and job + hosts in one of these locations: + + - under ``WORKFLOW-RUN-DIR/lib/python/`` + - under ``CYLC-PATH/cylc/flow/job_runner_handlers/`` + - or anywhere in ``$PYTHONPATH`` + + Each module should export the symbol ``JOB_RUNNER_HANDLER`` for the + singleton instance that implements the job system handler logic e.g: + + .. code-block:: python + :caption: my_handler.py + + class MyHandler(): + pass + + JOB_RUNNER_HANDLER = MyHandler() + + Each job runner handler class should instantiate with no argument. + + + .. rubric:: Usage + + You can then define a Cylc platform using the handler: + + .. code-block:: cylc + + [platforms] + [[my_platform]] + job runner = my_handler # note matches Python module name + hosts = localhost + + And configure tasks to submit to it: + + .. code-block:: cylc + + [runtime] + [[my_task]] + script = echo "Hello World!" + platform = my_platform + + + .. rubric:: Common Arguments + + ``job_conf: dict`` + The Cylc job configuration as a dictionary with the following fields: + + * ``dependencies`` + * ``directives`` + * ``env-script`` + * ``environment`` + * ``err-script`` + * ``execution_time_limit`` + * ``exit-script`` + * ``flow_nums`` + * ``init-script`` + * ``job_d`` + * ``job_file_path`` + * ``job_runner_command_template`` + * ``job_runner_name`` + * ``logfiles`` + * ``namespace_hierarchy`` + * ``param_var`` + * ``platform`` + * ``post-script`` + * ``pre-script`` + * ``script`` + * ``submit_num`` + * ``task_id`` + * ``try_num`` + * ``uuid_str`` + * ``work_d`` + * ``workflow_name`` + + ``submit_opts: dict`` + The Cylc job submission options as a dictionary which may contain + the following fields: + + * ``env`` + * ``execution_time_limit`` + * ``execution_time_limit`` + * ``job_runner_cmd_tmpl`` + * ``job_runner_cmd_tmpl`` + + + .. rubric:: An Example + + The following ``qsub.py`` module overrides the built-in *pbs* + job runner handler to change the directive prefix from ``#PBS`` to + ``#QSUB``: + + .. TODO - double check that this still works, it's been a while + + .. code-block:: python + + #!/usr/bin/env python3 + + from cylc.flow.job_runner_handlers.pbs import PBSHandler + + class QSUBHandler(PBSHandler): + DIRECTIVE_PREFIX = "#QSUB " + + JOB_RUNNER_HANDLER = QSUBHandler() + + If this is in the Python search path (see + :ref:`Where To Put Job Runner Handler Modules` above) you can use it by + name in your global configuration: + + .. code-block:: cylc + + [platforms] + [[my_platform]] + hosts = myhostA, myhostB + job runner = qsub # <---! + + Then in your ``flow.cylc`` file you can use this platform: + + .. code-block:: cylc + + # Note, this workflow will fail at run time because we only changed the + # directive format, and PBS does not accept ``#QSUB`` directives in + # reality. + + [scheduling] + [[graph]] + R1 = "a" + [runtime] + [[root]] + execution time limit = PT1M + platform = my_platform + [[[directives]]] + -l nodes = 1 + -q = long + -V = + + .. note:: + + Don't subclass this module as it provides optional interfaces which + you may not want to inherit. + + """ + + FAIL_SIGNALS: Tuple[str] + """A tuple containing the names of signals to trap for reporting errors. + + The default is ``("EXIT", "ERR", "TERM", "XCPU")``. + + ``ERR`` and ``EXIT`` are always recommended. + ``EXIT`` is used to report premature stopping of the job + script, and its trap is unset at the end of the script. + """ + + KILL_CMD_TMPL: str + """Command template for killing a job submission. + + A Python string template for getting the job runner command to remove + and terminate a job ID. The command is formed using the logic: + ``job_runner.KILL_CMD_TMPL % {"job_id": job_id}``. + + For info on Python string template format see: + https://docs.python.org/3/library/stdtypes.html#printf-style-string-formatting + + """ + + POLL_CANT_CONNECT_ERR: str + """String for detecting communication errors in poll command output. + + A string containing an error message. If this is defined, when a poll + command returns a non-zero return code and its STDERR contains this + string, then the poll result will not be trusted, because it is assumed + that the job runner is currently unavailable. Jobs submitted to the + job runner will be assumed OK until we are able to connect to the + job runner again. + + """ + + SHOULD_KILL_PROC_GROUP: bool + """Kill jobs by killing the process group. + + A boolean to indicate whether it is necessary to kill a job by sending + a signal to its Unix process group. This boolean also indicates that + a job submitted via this job runner will physically run on the same + host it is submitted to. + + """ + + SHOULD_POLL_PROC_GROUP: bool + """Poll jobs by PID. + + A boolean to indicate whether it is necessary to poll a job by its PID + as well as the job ID. + + """ + + REC_ID_FROM_SUBMIT_OUT: re.Pattern + """Regular expression to extract job ids from submission stderr. + + A regular expression (compiled) to extract the job "id" from the standard + output or standard error of the job submission command. + + """ + + REC_ID_FROM_SUBMIT_ERR: re.Pattern + """Regular expression to extract job ids from submission stderr. + + See :py:attr:`ExampleHandler.REC_ID_FROM_SUBMIT_OUT`. + + """ + + SUBMIT_CMD_ENV: Iterable[str] + """Extra environment variables for the job runner command. + + A Python dict (or an iterable that can be used to update a dict) + containing extra environment variables for getting the job runner + command to submit a job file. + + """ + + SUBMIT_CMD_TMPL: str + """Command template for job submission. + + A Python string template for getting the job runner command to submit a + job file. The command is formed using the logic: + ``job_runner.SUBMIT_CMD_TMPL % {"job": job_file_path}`` + + For info on Python string template format see: + https://docs.python.org/3/library/stdtypes.html#printf-style-string-formatting + + """ + + def filter_poll_many_output(self, out: str) -> List[str]: + """Filter job ides out of poll output. + + Called after the job runner's poll many command. The method should read + the output and return a list of job IDs that are still in the + job runner. + + Args: + out: Job poll stdout. + + Returns: + List of job ids + + """ + raise NotImplementedError() + + def filter_submit_output(self, out: str, err: str) -> Tuple[str, str]: + """Filter job submission stdout/err. + + Filter the standard output and standard error of the job submission + command. This is useful if the job submission command returns + information that should just be ignored. + + See also :py:meth:`ExampleHandler.SUBMIT_CMD_TMPL`. + + Args: + out: Job submit stdout. + err: Job submit stderr. + + Returns: + (new_out, new_err) + """ + raise NotImplementedError() + + def format_directives(self, job_conf: dict) -> List[str]: + """Returns lines to be appended to the job script. + + If relevant, this method formats the job directives for a job file, if + job file directives are relevant for the job runner. The argument + "job_conf" is a dict containing the job configuration. + + Args: + job_conf: The Cylc configuration. + + Returns: + lines + + """ + raise NotImplementedError() + + def get_poll_many_cmd(self, job_id_list: List[str]) -> List[str]: + """Return a command to poll the specified jobs. + + Args: + job_id_list: The listof job ids to poll. + + Returns: + command e.g. ['foo', '--bar', 'baz'] + + """ + raise NotImplementedError() + + def get_submit_stdin(self, job_file_path: str, submit_opts: dict) -> Tuple: + """ + + Return a 2-element tuple ``(proc_stdin_arg, proc_stdin_value)``. + + * Element 1 is suitable for the ``stdin=...`` argument of + ``subprocess.Popen`` so it can be a file handle, ``subprocess.PIPE`` + or ``None``. + * Element 2 is the string content to pipe to stdin of the submit + command (relevant only if ``proc_stdin_arg`` is ``subprocess.PIPE``. + + Args: + job_file_path: The path to the job file for this submission. + submit_opts: Job submission options. + + Returns: + (proc_stdin_arg, proc_stdin_value) + + """ + raise NotImplementedError() + + def get_vacation_signal(self, job_conf: dict) -> str: + """Return the vacation signal. + + If relevant, return a string containing the name of the signal that + indicates the job has been vacated by the job runner. + + Args: + job_conf: The Cylc configuration. + + Returns: + signal + + """ + raise NotImplementedError() + + def submit( + self, + job_file_path: str, + submit_opts: dict, + ) -> Tuple[int, str, str]: + """Submit a job. + + Submit a job and return an instance of the Popen object for the + submission. This method is useful if the job submission requires logic + beyond just running a system or shell command. + + See also :py:attr:`ExampleHandler.SUBMIT_CMD_TMPL`. + + You must pass "env=submit_opts.get('env')" to Popen - see + :py:mod:`cylc.flow.job_runner_handlers.background` + for an example. + + Args: + job_file_path: The job file for this submission. + submit_opts: Job submission options. + + Returns: + (ret_code, out, err) + + """ + raise NotImplementedError() + + def manip_job_id(self, job_id: str) -> str: + """Modify the job ID that is returned by the job submit command. + + Args: + job_id: The job ID returned by the submit command. + + Returns: + job_id + + """ + raise NotImplementedError() diff --git a/cylc/flow/job_runner_mgr.py b/cylc/flow/job_runner_mgr.py index 2072f314132..60c68e7875c 100644 --- a/cylc/flow/job_runner_mgr.py +++ b/cylc/flow/job_runner_mgr.py @@ -16,100 +16,10 @@ """Manage submission, poll and kill of a job to the job runners. -Export the JobRunnerManager class. - -Job runner handler (a.k.a. job submission method) modules should be placed -under the "cylc.flow.job_runner_handlers" package. Each module should export -the symbol "JOB_RUNNER_HANDLER" for the singleton instance that implements the -job system handler logic. - -Each job runner handler class should instantiate with no argument, and may -have the following constants and methods: - -job_runner.filter_poll_many_output(out) => job_ids - * Called after the job runner's poll many command. The method should read - the output and return a list of job IDs that are still in the - job runner. - -job_runner.filter_submit_output(out, err) => new_out, new_err - * Filter the standard output and standard error of the job submission - command. This is useful if the job submission command returns information - that should just be ignored. See also "job_runner.SUBMIT_CMD_TMPL". - -job_runner.format_directives(job_conf) => lines - * If relevant, this method formats the job directives for a job file, if - job file directives are relevant for the job runner. The argument - "job_conf" is a dict containing the job configuration. - -job_runner.get_poll_many_cmd(job-id-list) => list - * Return a list containing the shell command to poll the jobs in the - argument list. - -job_runner.get_submit_stdin(job_file_path: str, submit_opts: dict) => tuple - * Return a 2-element tuple `(proc_stdin_arg, proc_stdin_value)`. - Element 1 is suitable for the `stdin=...` argument of `subprocess.Popen` - so it can be a file handle, `subprocess.PIPE` or `None`. Element 2 is the - string content to pipe to STDIN of the submit command (relevant only if - `proc_stdin_arg` is `subprocess.PIPE`. - -job_runner.get_vacation_signal(job_conf) => str - * If relevant, return a string containing the name of the signal that - indicates the job has been vacated by the job runner. - -job_runner.submit(job_file_path, submit_opts) => ret_code, out, err - * Submit a job and return an instance of the Popen object for the - submission. This method is useful if the job submission requires logic - beyond just running a system or shell command. See also - "job_runner.SUBMIT_CMD". You must pass "env=submit_opts.get('env')" to - Popen - see background.py for example. - -job_runner.manip_job_id(job_id) => job_id - * Modify the job ID that is returned by the job submit command. - -job_runner.FAIL_SIGNALS => tuple - * A tuple containing the names of signals to trap for reporting errors. - Default is ("EXIT", "ERR", "TERM", "XCPU"). ERR and EXIT are always - recommended. EXIT is used to report premature stopping of the job - script, and its trap is unset at the end of the script. - -job_runner.KILL_CMD_TMPL - * A Python string template for getting the job runner command to remove - and terminate a job ID. The command is formed using the logic: - job_runner.KILL_CMD_TMPL % {"job_id": job_id} - -job_runner.POLL_CANT_CONNECT_ERR - * A string containing an error message. If this is defined, when a poll - command returns a non-zero return code and its STDERR contains this - string, then the poll result will not be trusted, because it is assumed - that the job runner is currently unavailable. Jobs submitted to the - job runner will be assumed OK until we are able to connect to the - job runner again. - -job_runner.SHOULD_KILL_PROC_GROUP - * A boolean to indicate whether it is necessary to kill a job by sending - a signal to its Unix process group. This boolean also indicates that - a job submitted via this job runner will physically run on the same - host it is submitted to. - -job_runner.SHOULD_POLL_PROC_GROUP - * A boolean to indicate whether it is necessary to poll a job by its PID - as well as the job ID. - -job_runner.REC_ID_FROM_SUBMIT_ERR -job_runner.REC_ID_FROM_SUBMIT_OUT - * A regular expression (compiled) to extract the job "id" from the standard - output or standard error of the job submission command. - -job_runner.SUBMIT_CMD_ENV - * A Python dict (or an iterable that can be used to update a dict) - containing extra environment variables for getting the job runner - command to submit a job file. - -job_runner.SUBMIT_CMD_TMPL - * A Python string template for getting the job runner command to submit a - job file. The command is formed using the logic: - job_runner.SUBMIT_CMD_TMPL % {"job": job_file_path} - See also "job_runner._job_submit_impl". +The job runner interface is documented in +cylc.flow.job_runner_handlers.documentation. + +Please update this file as the interface changes. """ diff --git a/cylc/flow/task_job_mgr.py b/cylc/flow/task_job_mgr.py index da995bac846..6671cbe0c93 100644 --- a/cylc/flow/task_job_mgr.py +++ b/cylc/flow/task_job_mgr.py @@ -1285,7 +1285,8 @@ def get_job_conf( """ return { # NOTE: these fields should match get_simulation_job_conf - # TODO: turn this into a namedtuple or similar + # TODO: formalise this + # https://github.com/cylc/cylc-flow/issues/5387 'job_runner_name': itask.platform['job runner'], 'job_runner_command_template': ( itask.platform['job runner command template'] From aaf3490aee002de09e22311ed4f94c4be444f248 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Wed, 8 Mar 2023 13:02:42 +0000 Subject: [PATCH 23/57] tests/u: cycling/test_iso8601 - unittest -> pytest * Convert unit tests to pytests. * Fix issue where the setup of one test changed the environment of the next. * Fixed an issue where a test was missing a setup step so failed unless run after a test which performed the required setup. --- cylc/flow/cycling/iso8601.py | 1 + tests/unit/conftest.py | 5 +- tests/unit/cycling/test_iso8601.py | 1699 +++++++++++++++------------- 3 files changed, 935 insertions(+), 770 deletions(-) diff --git a/cylc/flow/cycling/iso8601.py b/cylc/flow/cycling/iso8601.py index 418005b08a6..9e7032efdc9 100644 --- a/cylc/flow/cycling/iso8601.py +++ b/cylc/flow/cycling/iso8601.py @@ -855,6 +855,7 @@ def init(num_expanded_year_digits=0, custom_dump_format=None, time_zone=None, WorkflowSpecifics.abbrev_util = CylcTimeParser( None, None, WorkflowSpecifics.iso8601_parsers ) + return WorkflowSpecifics def get_dump_format(): diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index c066d10c064..3023be487ed 100644 --- a/tests/unit/conftest.py +++ b/tests/unit/conftest.py @@ -180,7 +180,10 @@ class _DefaultCycler: monkeypatch.setattr( 'cylc.flow.cycling.loader.DefaultCycler', _DefaultCycler) if ctype == ISO8601_CYCLING_TYPE: - iso8601_init(time_zone=time_zone, custom_dump_format=dump_format) + monkeypatch.setattr( + 'cylc.flow.cycling.iso8601.WorkflowSpecifics', + iso8601_init(time_zone=time_zone, custom_dump_format=dump_format) + ) return _set_cycling_type diff --git a/tests/unit/cycling/test_iso8601.py b/tests/unit/cycling/test_iso8601.py index b12e0fdb72f..8e95b809c1a 100644 --- a/tests/unit/cycling/test_iso8601.py +++ b/tests/unit/cycling/test_iso8601.py @@ -15,780 +15,941 @@ # along with this program. If not, see . import pytest -import unittest from datetime import datetime -from cylc.flow.cycling.iso8601 import init, ISO8601Sequence, ISO8601Point,\ - ISO8601Interval, ingest_time -from cylc.flow.exceptions import CylcConfigError - - -class TestISO8601Sequence(unittest.TestCase): - """Contains unit tests for the ISO8601Sequence class.""" - - def setUp(self): - init(time_zone='Z') - - def test_exclusions_simple(self): - """Test the generation of points for sequences with exclusions.""" - init(time_zone='Z') - sequence = ISO8601Sequence('PT1H!20000101T02Z', '20000101T00Z') - - output = [] - point = sequence.get_start_point() - count = 0 - while point and count < 4: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - self.assertEqual(output, ['20000101T0000Z', '20000101T0100Z', - '20000101T0300Z', '20000101T0400Z']) - - def test_exclusions_offset(self): - """Test the generation of points for sequences with exclusions - that have an offset on the end""" - init(time_zone='Z') - sequence = ISO8601Sequence('PT1H!20000101T00Z+PT1H', '20000101T00Z') - - output = [] - point = sequence.get_start_point() - count = 0 - while point and count < 4: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - self.assertEqual(output, ['20000101T0000Z', '20000101T0200Z', - '20000101T0300Z', '20000101T0400Z']) - - def test_multiple_exclusions_complex1(self): - """Tests sequences that have multiple exclusions and a more - complicated format""" - - # A sequence that specifies a dep start time - sequence = ISO8601Sequence('20000101T01Z/PT1H!20000101T02Z', - '20000101T01Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make four sequence points - while point and count < 4: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - # We should expect one of the hours to be excluded: T02 - self.assertEqual(output, ['20000101T0100Z', '20000101T0300Z', - '20000101T0400Z', '20000101T0500Z']) - - def test_multiple_exclusions_complex2(self): - """Tests sequences that have multiple exclusions and a more - complicated format""" - - # A sequence that specifies a dep start time - sequence = ISO8601Sequence('20000101T01Z/PT1H!' - '(20000101T02Z,20000101T03Z)', - '20000101T00Z', - '20000101T05Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make four sequence points - while point and count < 3: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - # We should expect two of the hours to be excluded: T02, T03 - self.assertEqual(output, ['20000101T0100Z', '20000101T0400Z', - '20000101T0500Z']) - - def test_multiple_exclusions_simple(self): - """Tests generation of points for sequences with multiple exclusions - """ - init(time_zone='Z') - sequence = ISO8601Sequence('PT1H!(20000101T02Z,20000101T03Z)', - '20000101T00Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make four sequence points - while point and count < 4: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - # We should expect two of the hours to be excluded: T02 and T03 - self.assertEqual(output, ['20000101T0000Z', '20000101T0100Z', - '20000101T0400Z', '20000101T0500Z']) - - def test_advanced_exclusions_partial_datetime1(self): - """Advanced exclusions refers to exclusions that are not just - simple points but could be time periods or recurrences such as - '!T06' or similar""" - init(time_zone='Z') - # Run 3-hourly but not at 06:00 (from the ICP) - sequence = ISO8601Sequence('PT3H!T06', '20000101T00Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make ten sequence points - while point and count < 10: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - # We should expect every T06 to be excluded - self.assertEqual(output, ['20000101T0000Z', '20000101T0300Z', - '20000101T0900Z', '20000101T1200Z', - '20000101T1500Z', '20000101T1800Z', - '20000101T2100Z', '20000102T0000Z', - '20000102T0300Z', '20000102T0900Z']) - - def test_advanced_exclusions_partial_datetime2(self): - """Advanced exclusions refers to exclusions that are not just - simple points but could be time periods or recurrences such as - '!T06' or similar""" - init(time_zone='Z') - # Run hourly but not at 00:00, 06:00, 12:00, 18:00 - sequence = ISO8601Sequence('T-00!(T00, T06, T12, T18)', '20000101T00Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make 18 sequence points - while point and count < 18: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - # We should expect T00, T06, T12, and T18 to be excluded - self.assertEqual(output, ['20000101T0100Z', '20000101T0200Z', - '20000101T0300Z', '20000101T0400Z', - '20000101T0500Z', '20000101T0700Z', - '20000101T0800Z', '20000101T0900Z', - '20000101T1000Z', '20000101T1100Z', - '20000101T1300Z', '20000101T1400Z', - '20000101T1500Z', '20000101T1600Z', - '20000101T1700Z', '20000101T1900Z', - '20000101T2000Z', '20000101T2100Z']) - - def test_advanced_exclusions_partial_datetime3(self): - """Advanced exclusions refers to exclusions that are not just - simple points but could be time periods or recurrences such as - '!T06' or similar""" - init(time_zone='Z') - # Run 5 minutely but not at 15 minutes past the hour from ICP - sequence = ISO8601Sequence('PT5M!T-15', '20000101T00Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make 15 sequence points - while point and count < 15: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - # We should expect xx:15 (15 minutes past the hour) to be excluded - self.assertEqual(output, ['20000101T0000Z', '20000101T0005Z', - '20000101T0010Z', - '20000101T0020Z', '20000101T0025Z', - '20000101T0030Z', '20000101T0035Z', - '20000101T0040Z', '20000101T0045Z', - '20000101T0050Z', '20000101T0055Z', - '20000101T0100Z', '20000101T0105Z', - '20000101T0110Z', '20000101T0120Z']) - - def test_advanced_exclusions_partial_datetime4(self): - """Advanced exclusions refers to exclusions that are not just - simple points but could be time periods or recurrences such as - '!T06' or similar""" - init(time_zone='Z') - # Run daily at 00:00 except on Mondays - sequence = ISO8601Sequence('T00!W-1T00', '20170422T00Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make 19 sequence points - while point and count < 9: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - # We should expect Monday 24th April and Monday 1st May - # to be excluded. - self.assertEqual(output, ['20170422T0000Z', '20170423T0000Z', - '20170425T0000Z', '20170426T0000Z', - '20170427T0000Z', '20170428T0000Z', - '20170429T0000Z', '20170430T0000Z', - '20170502T0000Z']) - - def test_exclusions_to_string(self): - init(time_zone='Z') - # Check that exclusions are not included where they should not be. - basic = ISO8601Sequence('PT1H', '2000', '2001') - self.assertFalse('!' in str(basic)) - - # Check that exclusions are parsable. - sequence = ISO8601Sequence('PT1H!(20000101T10Z, PT6H)', '2000', '2001') - sequence2 = ISO8601Sequence(str(sequence), '2000', '2001') - self.assertEqual(sequence, sequence2) - - def test_advanced_exclusions_sequences1(self): - """Advanced exclusions refers to exclusions that are not just - simple points but could be time periods or recurrences such as - '!T06' or similar""" - init(time_zone='Z') - # Run hourly from the ICP but not 3-hourly - sequence = ISO8601Sequence('PT1H!PT3H', '20000101T01Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make six sequence points - while point and count < 6: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - # We should expect to see hourly from ICP but not 3 hourly - self.assertEqual(output, ['20000101T0200Z', '20000101T0300Z', - '20000101T0500Z', '20000101T0600Z', - '20000101T0800Z', '20000101T0900Z']) - - def test_advanced_exclusions_sequences2(self): - """Advanced exclusions refers to exclusions that are not just - simple points but could be time periods or recurrences such as - '!T06' or similar""" - init(time_zone='Z') - # Run hourly on the hour but not 3 hourly on the hour - sequence = ISO8601Sequence('T-00!T-00/PT3H', '20000101T00Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make six sequence points - while point and count < 6: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - - self.assertEqual(output, ['20000101T0100Z', '20000101T0200Z', - '20000101T0400Z', '20000101T0500Z', - '20000101T0700Z', '20000101T0800Z']) - - def test_advanced_exclusions_sequences3(self): - """Advanced exclusions refers to exclusions that are not just - simple points but could be time periods or recurrences such as - '!T06' or similar""" - init(time_zone='Z') - # Run daily at 12:00 except every 3rd day - sequence = ISO8601Sequence('T12!P3D', '20000101T12Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make six sequence points - while point and count < 6: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - - self.assertEqual(output, ['20000102T1200Z', '20000103T1200Z', - '20000105T1200Z', '20000106T1200Z', - '20000108T1200Z', '20000109T1200Z']) - - def test_advanced_exclusions_sequences4(self): - """Advanced exclusions refers to exclusions that are not just - simple points but could be time periods or recurrences such as - '!T06' or similar""" - init(time_zone='Z') - # Run every hour from 01:00 excluding every 3rd hour. - sequence = ISO8601Sequence('T01/PT1H!+PT3H/PT3H', '20000101T01Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make six sequence points - while point and count < 6: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - - self.assertEqual(output, ['20000101T0100Z', '20000101T0200Z', - '20000101T0300Z', '20000101T0500Z', - '20000101T0600Z', '20000101T0800Z']) - - def test_advanced_exclusions_sequences5(self): - """Advanced exclusions refers to exclusions that are not just - simple points but could be time periods or recurrences such as - '!T06' or similar""" - init(time_zone='Z') - # Run every hour from 01:00 excluding every 3rd hour. - sequence = ISO8601Sequence('T-00 ! 2000', '20000101T00Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make six sequence points - while point and count < 6: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - - self.assertEqual(output, ['20000101T0100Z', '20000101T0200Z', - '20000101T0300Z', '20000101T0400Z', - '20000101T0500Z', '20000101T0600Z']) - - def test_advanced_exclusions_sequences_mix_points_sequences(self): - """Advanced exclusions refers to exclusions that are not just - simple points but could be time periods or recurrences such as - '!T06' or similar""" - init(time_zone='Z') - # Run every hour from 01:00 excluding every 3rd hour. - sequence = ISO8601Sequence('T-00 ! (2000, PT2H)', '20000101T00Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make six sequence points - while point and count < 6: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - - self.assertEqual(output, ['20000101T0100Z', '20000101T0300Z', - '20000101T0500Z', '20000101T0700Z', - '20000101T0900Z', '20000101T1100Z']) - - def test_advanced_exclusions_sequences_implied_start_point(self): - """Advanced exclusions refers to exclusions that are not just - simple points but could be time periods or recurrences such as - '!T06' or similar""" - init(time_zone='Z') - # Run every hour from 01:00 excluding every 3rd hour. - sequence = ISO8601Sequence('T05/PT1H!PT3H', '20000101T00Z') - - output = [] - point = sequence.get_start_point() - count = 0 - # We are going to make six sequence points - while point and count < 6: - output.append(point) - point = sequence.get_next_point(point) - count += 1 - output = [str(out) for out in output] - - self.assertEqual(output, ['20000101T0600Z', '20000101T0700Z', - '20000101T0900Z', '20000101T1000Z', - '20000101T1200Z', '20000101T1300Z']) - - def test_exclusions_sequences_points(self): - """Test ISO8601Sequence methods for sequences with exclusions""" - init(time_zone='Z') - # Run every hour from 01:00 excluding every 3rd hour - sequence = ISO8601Sequence('T01/PT1H!PT3H', '20000101T01Z') - - point_0 = ISO8601Point('20000101T00Z') - point_1 = ISO8601Point('20000101T01Z') - point_2 = ISO8601Point('20000101T02Z') - point_3 = ISO8601Point('20000101T03Z') - point_4 = ISO8601Point('20000101T04Z') - - self.assertFalse(point_0 in sequence.exclusions) - self.assertTrue(point_1 in sequence.exclusions) - self.assertTrue(sequence.is_on_sequence(point_2)) - self.assertTrue(sequence.is_on_sequence(point_3)) - self.assertFalse(sequence.is_on_sequence(point_4)) - self.assertTrue(point_4 in sequence.exclusions) - - def test_exclusions_extensive(self): - """Test ISO8601Sequence methods for sequences with exclusions""" - init(time_zone='+05') - sequence = ISO8601Sequence('PT1H!20000101T02+05', '20000101T00', - '20000101T05') - - point_0 = ISO8601Point('20000101T0000+05') - point_1 = ISO8601Point('20000101T0100+05') - point_2 = ISO8601Point('20000101T0200+05') # The excluded point. - point_3 = ISO8601Point('20000101T0300+05') - - self.assertFalse(sequence.is_on_sequence(point_2)) - self.assertFalse(sequence.is_valid(point_2)) - self.assertEqual(sequence.get_prev_point(point_2), point_1) - self.assertEqual(sequence.get_prev_point(point_3), point_1) - self.assertEqual(sequence.get_prev_point(point_2), point_1) - self.assertEqual(sequence.get_nearest_prev_point(point_3), point_1) - self.assertEqual(sequence.get_next_point(point_1), point_3) - self.assertEqual(sequence.get_next_point(point_2), point_3) - - sequence = ISO8601Sequence('PT1H!20000101T00+05', '20000101T00+05') - self.assertEqual(sequence.get_first_point(point_0), point_1) - self.assertEqual(sequence.get_start_point(), point_1) - - def test_multiple_exclusions_extensive(self): - """Test ISO8601Sequence methods for sequences with multiple exclusions - """ - init(time_zone='+05') - sequence = ISO8601Sequence('PT1H!(20000101T02,20000101T03)', - '20000101T00', - '20000101T06') - - point_0 = ISO8601Point('20000101T0000+05') - point_1 = ISO8601Point('20000101T0100+05') - point_2 = ISO8601Point('20000101T0200+05') # First excluded point - point_3 = ISO8601Point('20000101T0300+05') # Second excluded point - point_4 = ISO8601Point('20000101T0400+05') - - # Check the excluded points are not on the sequence - self.assertFalse(sequence.is_on_sequence(point_2)) - self.assertFalse(sequence.is_on_sequence(point_3)) - self.assertFalse(sequence.is_valid(point_2)) # Should be excluded - self.assertFalse(sequence.is_valid(point_3)) # Should be excluded - # Check that we can correctly retrieve previous points - self.assertEqual(sequence.get_prev_point(point_2), point_1) - # Should skip two excluded points - self.assertEqual(sequence.get_prev_point(point_4), point_1) - self.assertEqual(sequence.get_prev_point(point_2), point_1) - self.assertEqual(sequence.get_nearest_prev_point(point_4), point_1) - self.assertEqual(sequence.get_next_point(point_1), point_4) - self.assertEqual(sequence.get_next_point(point_3), point_4) - - sequence = ISO8601Sequence('PT1H!20000101T00+05', '20000101T00') - # Check that the first point is after 00. - self.assertEqual(sequence.get_first_point(point_0), point_1) - self.assertEqual(sequence.get_start_point(), point_1) - - # Check a longer list of exclusions - # Also note you can change the format of the exclusion list - # (removing the parentheses) - sequence = ISO8601Sequence('PT1H!(20000101T02+05, 20000101T03+05,' - '20000101T04+05)', - '20000101T00', - '20000101T06') - self.assertEqual(sequence.get_prev_point(point_3), point_1) - self.assertEqual(sequence.get_prev_point(point_4), point_1) - - def test_simple(self): - """Run some simple tests for date-time cycling.""" - init(time_zone='Z') - p_start = ISO8601Point('20100808T00') - p_stop = ISO8601Point('20100808T02') - i = ISO8601Interval('PT6H') - self.assertEqual(p_start - i, ISO8601Point('20100807T18')) - self.assertEqual(p_stop + i, ISO8601Point('20100808T08')) - - sequence = ISO8601Sequence('PT10M', str(p_start), str(p_stop),) - sequence.set_offset(- ISO8601Interval('PT10M')) - point = sequence.get_next_point(ISO8601Point('20100808T0000')) - self.assertEqual(point, ISO8601Point('20100808T0010')) - output = [] - - # Test point generation forwards. - while point and point < p_stop: - output.append(point) - self.assertTrue(sequence.is_on_sequence(point)) - point = sequence.get_next_point(point) - self.assertEqual([str(out) for out in output], - ['20100808T0010Z', '20100808T0020Z', - '20100808T0030Z', '20100808T0040Z', - '20100808T0050Z', '20100808T0100Z', - '20100808T0110Z', '20100808T0120Z', - '20100808T0130Z', '20100808T0140Z', - '20100808T0150Z']) - - self.assertEqual(point, ISO8601Point('20100808T0200')) - - # Test point generation backwards. - output = [] - while point and point >= p_start: - output.append(point) - self.assertTrue(sequence.is_on_sequence(point)) - point = sequence.get_prev_point(point) - self.assertEqual([str(out) for out in output], - ['20100808T0200Z', '20100808T0150Z', - '20100808T0140Z', '20100808T0130Z', - '20100808T0120Z', '20100808T0110Z', - '20100808T0100Z', '20100808T0050Z', - '20100808T0040Z', '20100808T0030Z', - '20100808T0020Z', '20100808T0010Z', - '20100808T0000Z']) - - self.assertFalse( - sequence.is_on_sequence(ISO8601Point('20100809T0005'))) - - -class TestRelativeCyclePoint(unittest.TestCase): - """Contains unit tests for cycle point relative to current time.""" - - def setUp(self): - init(time_zone='Z') - - def test_next_simple(self): - """Test the generation of CP using 'next' from single input.""" - my_now = '20100808T1540Z' - sequence = ('next(T2100Z)', # 20100808T2100Z - 'next(T00)', # 20100809T0000Z - 'next(T-15)', # 20100808T1615Z - 'next(T-45)', # 20100808T1545Z - 'next(-10)', # 21100101T0000Z - 'next(-1008)', # 21100801T0000Z - 'next(--10)', # 20101001T0000Z - 'next(--0325)', # 20110325T0000Z - 'next(---10)', # 20100810T0000Z - 'next(---05T1200Z)') # 20100905T1200Z - - output = [] - - for point in sequence: - output.append(ingest_time(point, my_now)) - self.assertEqual(output, ['20100808T2100Z', - '20100809T0000Z', - '20100808T1615Z', - '20100808T1545Z', - '21100101T0000Z', - '21100801T0000Z', - '20101001T0000Z', - '20110325T0000Z', - '20100810T0000Z', - '20100905T1200Z']) - - def test_previous_simple(self): - """Test the generation of CP using 'previous' from single input.""" - my_now = '20100808T1540Z' - sequence = ('previous(T2100Z)', # 20100807T2100Z - 'previous(T00)', # 20100808T0000Z - 'previous(T-15)', # 20100808T1515Z - 'previous(T-45)', # 20100808T1445Z - 'previous(-10)', # 20100101T0000Z - 'previous(-1008)', # 20100801T0000Z - 'previous(--10)', # 20091001T0000Z - 'previous(--0325)', # 20100325T0000Z - 'previous(---10)', # 20100710T0000Z - 'previous(---05T1200Z)') # 20100805T1200Z - - output = [] - - for point in sequence: - output.append(ingest_time(point, my_now)) - self.assertEqual(output, ['20100807T2100Z', - '20100808T0000Z', - '20100808T1515Z', - '20100808T1445Z', - '20100101T0000Z', - '20100801T0000Z', - '20091001T0000Z', - '20100325T0000Z', - '20100710T0000Z', - '20100805T1200Z']) - - def test_sequence(self): - """Test the generation of CP from list input.""" - my_now = '20100808T1540Z' - sequence = ( - 'next(T-00;T-15;T-30;T-45)', # 20100808T1545Z - 'previous(T-00;T-15;T-30;T-45)', # 20100808T1530Z - 'next(T00;T06;T12;T18)', # 20100808T1800Z - 'previous(T00;T06;T12;T18)') # 20100808T1200Z - - output = [] - - for point in sequence: - output.append(ingest_time(point, my_now)) - self.assertEqual(output, ['20100808T1545Z', - '20100808T1530Z', - '20100808T1800Z', - '20100808T1200Z']) - - def test_offset_simple(self): - """Test the generation of offset CP.""" - my_now = '20100808T1540Z' - sequence = ('PT15M', # 20100808T1555Z - '-PT30M', # 20100808T1510Z - 'PT1H', # 20100808T1640Z - '-PT18H', # 20100807T2140Z - 'P3D', # 20100811T1540Z - '-P2W', # 20100725T1540Z - 'P6M', # 20110208T1540Z - '-P1M', # 20100708T1540Z - 'P1Y', # 20110808T1540Z - '-P5Y') # 20050808T1540Z - - output = [] - - for point in sequence: - output.append(ingest_time(point, my_now)) - self.assertEqual(output, ['20100808T1555Z', - '20100808T1510Z', - '20100808T1640Z', - '20100807T2140Z', - '20100811T1540Z', - '20100725T1540Z', - '20110208T1540Z', - '20100708T1540Z', - '20110808T1540Z', - '20050808T1540Z']) - - def test_offset(self): - """Test the generation of offset CP with 'next' and 'previous'.""" - my_now = '20100808T1540Z' - sequence = ( - 'next(T06) +P1D', # 20100810T0600Z - 'previous(T-30) -PT12H', # 20100808T0330Z - 'next(T00;T06;T12;T18) -P1W', # 20100801T1800Z - 'previous(T00;T06;T12;T18) +PT1H') # 20100808T1300Z - - output = [] - - for point in sequence: - output.append(ingest_time(point, my_now)) - self.assertEqual(output, ['20100810T0600Z', - '20100808T0330Z', - '20100801T1800Z', - '20100808T1300Z']) - - def test_weeks_days(self): - """Test the generation of CP with day-of-week, - ordinal day, and week (with day-of-week specified).""" - my_now = '20100808T1540Z' - sequence = ( - 'next(-W-1)', # 20100809T0000Z - 'previous(-W-4)', # 20100805T0000Z - 'next(-010)', # 20110110T0000Z - 'previous(-101)', # 20100411T0000Z - 'next(-W40-1)', # 20101004T0000Z - 'previous(-W05-1)', # 20100201T0000Z - 'next(-W05-5)', # 20110204T0000Z - 'previous(-W40-4)') # 20091001T0000Z - - output = [] - - for point in sequence: - output.append(ingest_time(point, my_now)) - self.assertEqual(output, ['20100809T0000Z', - '20100805T0000Z', - '20110110T0000Z', - '20100411T0000Z', - '20101004T0000Z', - '20100201T0000Z', - '20110204T0000Z', - '20091001T0000Z']) - - def test_cug(self): - """Test the offset CP examples in the Cylc user guide""" - my_now = '2018-03-14T15:12Z' - sequence = ( - 'next(T-00)', # 20180314T1600Z - 'previous(T-00)', # 20180314T1500Z - 'next(T-00; T-15; T-30; T-45)', # 20180314T1515Z - 'previous(T-00; T-15; T-30; T-45)', # 20180314T1500Z - 'next(T00)', # 20180315T0000Z - 'previous(T00)', # 20180314T0000Z - 'next(T06:30Z)', # 20180315T0630Z - 'previous(T06:30) -P1D', # 20180313T0630Z - 'next(T00; T06; T12; T18)', # 20180314T1800Z - 'previous(T00; T06; T12; T18)', # 20180314T1200Z - 'next(T00; T06; T12; T18)+P1W', # 20180321T1800Z - 'PT1H', # 20180314T1612Z - '-P1M', # 20180214T1512Z - 'next(-00)', # 21000101T0000Z - 'previous(--01)', # 20180101T0000Z - 'next(---01)', # 20180401T0000Z - 'previous(--1225)', # 20171225T0000Z - 'next(-2006)', # 20200601T0000Z - 'previous(-W101)', # 20180305T0000Z - 'next(-W-1; -W-3; -W-5)', # 20180314T0000Z - 'next(-001; -091; -181; -271)', # 20180401T0000Z - 'previous(-365T12Z)') # 20171231T1200Z - - output = [] - - for point in sequence: - output.append(ingest_time(point, my_now)) - self.assertEqual(output, ['20180314T1600Z', - '20180314T1500Z', - '20180314T1515Z', - '20180314T1500Z', - '20180315T0000Z', - '20180314T0000Z', - '20180315T0630Z', - '20180313T0630Z', - '20180314T1800Z', - '20180314T1200Z', - '20180321T1800Z', - '20180314T1612Z', - '20180214T1512Z', - '21000101T0000Z', - '20180101T0000Z', - '20180401T0000Z', - '20171225T0000Z', - '20200601T0000Z', - '20180305T0000Z', - '20180314T0000Z', - '20180401T0000Z', - '20171231T1200Z']) - - def test_next_simple_no_now(self): - """Test the generation of CP using 'next' with no value for `now`.""" - my_now = None - point = 'next(T00Z)+P1D' - output = ingest_time(point, my_now) - - current_time = datetime.utcnow() - # my_now is None, but ingest_time will have used a similar time, and - # the returned value must be after current_time - output_time = datetime.strptime(output, "%Y%m%dT%H%MZ") - self.assertTrue(current_time < output_time) - - def test_integer_cycling_is_returned(self): - """Test that when integer points are given, the same value is - returned.""" - integer_point = "1" - self.assertEqual(integer_point, ingest_time(integer_point, None)) - - def test_expanded_dates_are_returned(self): - """Test that when expanded dates are given, the same value is - returned.""" - expanded_date = "+0100400101T0000Z" - self.assertEqual(expanded_date, ingest_time(expanded_date, None)) - - def test_timepoint_truncated(self): - """Test that when a timepoint is given, and is truncated, then the - value is added to `now`.""" - my_now = '2018-03-14T15:12Z' - timepoint_truncated = "T15:00Z" # 20180315T1500Z - output = ingest_time(timepoint_truncated, my_now) - self.assertEqual("20180315T1500Z", output) - - def test_timepoint(self): - """Test that when a timepoint is given, and is not truncated, the - same value is returned.""" - my_now = '2018-03-14T15:12Z' - timepoint_truncated = "19951231T0630" # 19951231T0630 - output = ingest_time(timepoint_truncated, my_now) - self.assertEqual("19951231T0630", output) +from cylc.flow.cycling.iso8601 import ( + ISO8601Interval, + ISO8601Point, + ISO8601Sequence, + ingest_time, +) +from cylc.flow.cycling.loader import ISO8601_CYCLING_TYPE + + +def test_exclusions_simple(set_cycling_type): + """Test the generation of points for sequences with exclusions.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + sequence = ISO8601Sequence("PT1H!20000101T02Z", "20000101T00Z") + + output = [] + point = sequence.get_start_point() + count = 0 + while point and count < 4: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + assert output == [ + "20000101T0000Z", + "20000101T0100Z", + "20000101T0300Z", + "20000101T0400Z", + ] + + +def test_exclusions_offset(set_cycling_type): + """Test the generation of points for sequences with exclusions + that have an offset on the end""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + sequence = ISO8601Sequence("PT1H!20000101T00Z+PT1H", "20000101T00Z") + + output = [] + point = sequence.get_start_point() + count = 0 + while point and count < 4: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + assert output == [ + "20000101T0000Z", + "20000101T0200Z", + "20000101T0300Z", + "20000101T0400Z", + ] + + +def test_multiple_exclusions_complex1(set_cycling_type): + """Tests sequences that have multiple exclusions and a more + complicated format""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + + # A sequence that specifies a dep start time + sequence = ISO8601Sequence("20000101T01Z/PT1H!20000101T02Z", "20000101T01Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make four sequence points + while point and count < 4: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + # We should expect one of the hours to be excluded: T02 + assert output == [ + "20000101T0100Z", + "20000101T0300Z", + "20000101T0400Z", + "20000101T0500Z", + ] + + +def test_multiple_exclusions_complex2(set_cycling_type): + """Tests sequences that have multiple exclusions and a more + complicated format""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + + # A sequence that specifies a dep start time + sequence = ISO8601Sequence( + "20000101T01Z/PT1H!" "(20000101T02Z,20000101T03Z)", + "20000101T00Z", + "20000101T05Z", + ) + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make four sequence points + while point and count < 3: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + # We should expect two of the hours to be excluded: T02, T03 + assert output == [ + "20000101T0100Z", + "20000101T0400Z", + "20000101T0500Z", + ] + + +def test_multiple_exclusions_simple(set_cycling_type): + """Tests generation of points for sequences with multiple exclusions""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + sequence = ISO8601Sequence("PT1H!(20000101T02Z,20000101T03Z)", "20000101T00Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make four sequence points + while point and count < 4: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + # We should expect two of the hours to be excluded: T02 and T03 + assert output == [ + "20000101T0000Z", + "20000101T0100Z", + "20000101T0400Z", + "20000101T0500Z", + ] + + +def test_advanced_exclusions_partial_datetime1(set_cycling_type): + """Advanced exclusions refers to exclusions that are not just + simple points but could be time periods or recurrences such as + '!T06' or similar""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run 3-hourly but not at 06:00 (from the ICP) + sequence = ISO8601Sequence("PT3H!T06", "20000101T00Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make ten sequence points + while point and count < 10: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + # We should expect every T06 to be excluded + assert output == [ + "20000101T0000Z", + "20000101T0300Z", + "20000101T0900Z", + "20000101T1200Z", + "20000101T1500Z", + "20000101T1800Z", + "20000101T2100Z", + "20000102T0000Z", + "20000102T0300Z", + "20000102T0900Z", + ] + + +def test_advanced_exclusions_partial_datetime2(set_cycling_type): + """Advanced exclusions refers to exclusions that are not just + simple points but could be time periods or recurrences such as + '!T06' or similar""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run hourly but not at 00:00, 06:00, 12:00, 18:00 + sequence = ISO8601Sequence("T-00!(T00, T06, T12, T18)", "20000101T00Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make 18 sequence points + while point and count < 18: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + # We should expect T00, T06, T12, and T18 to be excluded + assert output == [ + "20000101T0100Z", + "20000101T0200Z", + "20000101T0300Z", + "20000101T0400Z", + "20000101T0500Z", + "20000101T0700Z", + "20000101T0800Z", + "20000101T0900Z", + "20000101T1000Z", + "20000101T1100Z", + "20000101T1300Z", + "20000101T1400Z", + "20000101T1500Z", + "20000101T1600Z", + "20000101T1700Z", + "20000101T1900Z", + "20000101T2000Z", + "20000101T2100Z", + ] + + +def test_advanced_exclusions_partial_datetime3(set_cycling_type): + """Advanced exclusions refers to exclusions that are not just + simple points but could be time periods or recurrences such as + '!T06' or similar""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run 5 minutely but not at 15 minutes past the hour from ICP + sequence = ISO8601Sequence("PT5M!T-15", "20000101T00Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make 15 sequence points + while point and count < 15: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + # We should expect xx:15 (15 minutes past the hour) to be excluded + assert output == [ + "20000101T0000Z", + "20000101T0005Z", + "20000101T0010Z", + "20000101T0020Z", + "20000101T0025Z", + "20000101T0030Z", + "20000101T0035Z", + "20000101T0040Z", + "20000101T0045Z", + "20000101T0050Z", + "20000101T0055Z", + "20000101T0100Z", + "20000101T0105Z", + "20000101T0110Z", + "20000101T0120Z", + ] + + +def test_advanced_exclusions_partial_datetime4(set_cycling_type): + """Advanced exclusions refers to exclusions that are not just + simple points but could be time periods or recurrences such as + '!T06' or similar""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run daily at 00:00 except on Mondays + sequence = ISO8601Sequence("T00!W-1T00", "20170422T00Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make 19 sequence points + while point and count < 9: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + # We should expect Monday 24th April and Monday 1st May + # to be excluded. + assert output == [ + "20170422T0000Z", + "20170423T0000Z", + "20170425T0000Z", + "20170426T0000Z", + "20170427T0000Z", + "20170428T0000Z", + "20170429T0000Z", + "20170430T0000Z", + "20170502T0000Z", + ] + + +def test_exclusions_to_string(set_cycling_type): + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Check that exclusions are not included where they should not be. + basic = ISO8601Sequence("PT1H", "2000", "2001") + assert not "!" in str(basic) + + # Check that exclusions are parsable. + sequence = ISO8601Sequence("PT1H!(20000101T10Z, PT6H)", "2000", "2001") + sequence2 = ISO8601Sequence(str(sequence), "2000", "2001") + assert sequence == sequence2 + + +def test_advanced_exclusions_sequences1(set_cycling_type): + """Advanced exclusions refers to exclusions that are not just + simple points but could be time periods or recurrences such as + '!T06' or similar""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run hourly from the ICP but not 3-hourly + sequence = ISO8601Sequence("PT1H!PT3H", "20000101T01Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make six sequence points + while point and count < 6: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + # We should expect to see hourly from ICP but not 3 hourly + assert output == [ + "20000101T0200Z", + "20000101T0300Z", + "20000101T0500Z", + "20000101T0600Z", + "20000101T0800Z", + "20000101T0900Z", + ] + + +def test_advanced_exclusions_sequences2(set_cycling_type): + """Advanced exclusions refers to exclusions that are not just + simple points but could be time periods or recurrences such as + '!T06' or similar""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run hourly on the hour but not 3 hourly on the hour + sequence = ISO8601Sequence("T-00!T-00/PT3H", "20000101T00Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make six sequence points + while point and count < 6: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + + assert output == [ + "20000101T0100Z", + "20000101T0200Z", + "20000101T0400Z", + "20000101T0500Z", + "20000101T0700Z", + "20000101T0800Z", + ] + + +def test_advanced_exclusions_sequences3(set_cycling_type): + """Advanced exclusions refers to exclusions that are not just + simple points but could be time periods or recurrences such as + '!T06' or similar""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run daily at 12:00 except every 3rd day + sequence = ISO8601Sequence("T12!P3D", "20000101T12Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make six sequence points + while point and count < 6: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + + assert output == [ + "20000102T1200Z", + "20000103T1200Z", + "20000105T1200Z", + "20000106T1200Z", + "20000108T1200Z", + "20000109T1200Z", + ] + + +def test_advanced_exclusions_sequences4(set_cycling_type): + """Advanced exclusions refers to exclusions that are not just + simple points but could be time periods or recurrences such as + '!T06' or similar""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run every hour from 01:00 excluding every 3rd hour. + sequence = ISO8601Sequence("T01/PT1H!+PT3H/PT3H", "20000101T01Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make six sequence points + while point and count < 6: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + + assert output == [ + "20000101T0100Z", + "20000101T0200Z", + "20000101T0300Z", + "20000101T0500Z", + "20000101T0600Z", + "20000101T0800Z", + ] + + +def test_advanced_exclusions_sequences5(set_cycling_type): + """Advanced exclusions refers to exclusions that are not just + simple points but could be time periods or recurrences such as + '!T06' or similar""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run every hour from 01:00 excluding every 3rd hour. + sequence = ISO8601Sequence("T-00 ! 2000", "20000101T00Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make six sequence points + while point and count < 6: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + + assert output == [ + "20000101T0100Z", + "20000101T0200Z", + "20000101T0300Z", + "20000101T0400Z", + "20000101T0500Z", + "20000101T0600Z", + ] + + +def test_advanced_exclusions_sequences_mix_points_sequences(set_cycling_type): + """Advanced exclusions refers to exclusions that are not just + simple points but could be time periods or recurrences such as + '!T06' or similar""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run every hour from 01:00 excluding every 3rd hour. + sequence = ISO8601Sequence("T-00 ! (2000, PT2H)", "20000101T00Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make six sequence points + while point and count < 6: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + + assert output == [ + "20000101T0100Z", + "20000101T0300Z", + "20000101T0500Z", + "20000101T0700Z", + "20000101T0900Z", + "20000101T1100Z", + ] + + +def test_advanced_exclusions_sequences_implied_start_point(set_cycling_type): + """Advanced exclusions refers to exclusions that are not just + simple points but could be time periods or recurrences such as + '!T06' or similar""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run every hour from 01:00 excluding every 3rd hour. + sequence = ISO8601Sequence("T05/PT1H!PT3H", "20000101T00Z") + + output = [] + point = sequence.get_start_point() + count = 0 + # We are going to make six sequence points + while point and count < 6: + output.append(point) + point = sequence.get_next_point(point) + count += 1 + output = [str(out) for out in output] + + assert output == [ + "20000101T0600Z", + "20000101T0700Z", + "20000101T0900Z", + "20000101T1000Z", + "20000101T1200Z", + "20000101T1300Z", + ] + + +def test_exclusions_sequences_points(set_cycling_type): + """Test ISO8601Sequence methods for sequences with exclusions""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + # Run every hour from 01:00 excluding every 3rd hour + sequence = ISO8601Sequence("T01/PT1H!PT3H", "20000101T01Z") + + point_0 = ISO8601Point("20000101T00Z") + point_1 = ISO8601Point("20000101T01Z") + point_2 = ISO8601Point("20000101T02Z") + point_3 = ISO8601Point("20000101T03Z") + point_4 = ISO8601Point("20000101T04Z") + + assert not point_0 in sequence.exclusions + assert point_1 in sequence.exclusions + assert sequence.is_on_sequence(point_2) + assert sequence.is_on_sequence(point_3) + assert not sequence.is_on_sequence(point_4) + assert point_4 in sequence.exclusions + + +def test_exclusions_extensive(set_cycling_type): + """Test ISO8601Sequence methods for sequences with exclusions""" + set_cycling_type(ISO8601_CYCLING_TYPE, "+05") + sequence = ISO8601Sequence("PT1H!20000101T02+05", "20000101T00", "20000101T05") + + point_0 = ISO8601Point("20000101T0000+05") + point_1 = ISO8601Point("20000101T0100+05") + point_2 = ISO8601Point("20000101T0200+05") # The excluded point. + point_3 = ISO8601Point("20000101T0300+05") + + assert not sequence.is_on_sequence(point_2) + assert not sequence.is_valid(point_2) + assert sequence.get_prev_point(point_2) == point_1 + assert sequence.get_prev_point(point_3) == point_1 + assert sequence.get_prev_point(point_2) == point_1 + assert sequence.get_nearest_prev_point(point_3) == point_1 + assert sequence.get_next_point(point_1) == point_3 + assert sequence.get_next_point(point_2) == point_3 + + sequence = ISO8601Sequence("PT1H!20000101T00+05", "20000101T00+05") + assert sequence.get_first_point(point_0) == point_1 + assert sequence.get_start_point() == point_1 + + +def test_multiple_exclusions_extensive(set_cycling_type): + """Test ISO8601Sequence methods for sequences with multiple exclusions""" + set_cycling_type(ISO8601_CYCLING_TYPE, "+05") + sequence = ISO8601Sequence( + "PT1H!(20000101T02,20000101T03)", "20000101T00", "20000101T06" + ) + + point_0 = ISO8601Point("20000101T0000+05") + point_1 = ISO8601Point("20000101T0100+05") + point_2 = ISO8601Point("20000101T0200+05") # First excluded point + point_3 = ISO8601Point("20000101T0300+05") # Second excluded point + point_4 = ISO8601Point("20000101T0400+05") + + # Check the excluded points are not on the sequence + assert not sequence.is_on_sequence(point_2) + assert not sequence.is_on_sequence(point_3) + assert not sequence.is_valid(point_2) # Should be excluded + assert not sequence.is_valid(point_3) # Should be excluded + # Check that we can correctly retrieve previous points + assert sequence.get_prev_point(point_2) == point_1 + # Should skip two excluded points + assert sequence.get_prev_point(point_4) == point_1 + assert sequence.get_prev_point(point_2) == point_1 + assert sequence.get_nearest_prev_point(point_4) == point_1 + assert sequence.get_next_point(point_1) == point_4 + assert sequence.get_next_point(point_3) == point_4 + + sequence = ISO8601Sequence("PT1H!20000101T00+05", "20000101T00") + # Check that the first point is after 00. + assert sequence.get_first_point(point_0) == point_1 + assert sequence.get_start_point() == point_1 + + # Check a longer list of exclusions + # Also note you can change the format of the exclusion list + # (removing the parentheses) + sequence = ISO8601Sequence( + "PT1H!(20000101T02+05, 20000101T03+05," "20000101T04+05)", + "20000101T00", + "20000101T06", + ) + assert sequence.get_prev_point(point_3) == point_1 + assert sequence.get_prev_point(point_4) == point_1 + + +def test_simple(set_cycling_type): + """Run some simple tests for date-time cycling.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + p_start = ISO8601Point("20100808T00") + p_stop = ISO8601Point("20100808T02") + i = ISO8601Interval("PT6H") + assert p_start - i == ISO8601Point("20100807T18") + assert p_stop + i == ISO8601Point("20100808T08") + + sequence = ISO8601Sequence( + "PT10M", + str(p_start), + str(p_stop), + ) + sequence.set_offset(-ISO8601Interval("PT10M")) + point = sequence.get_next_point(ISO8601Point("20100808T0000")) + assert point == ISO8601Point("20100808T0010") + output = [] + + # Test point generation forwards. + while point and point < p_stop: + output.append(point) + assert sequence.is_on_sequence(point) + point = sequence.get_next_point(point) + assert [str(out) for out in output] == [ + "20100808T0010Z", + "20100808T0020Z", + "20100808T0030Z", + "20100808T0040Z", + "20100808T0050Z", + "20100808T0100Z", + "20100808T0110Z", + "20100808T0120Z", + "20100808T0130Z", + "20100808T0140Z", + "20100808T0150Z", + ] + + assert point == ISO8601Point("20100808T0200") + + # Test point generation backwards. + output = [] + while point and point >= p_start: + output.append(point) + assert sequence.is_on_sequence(point) + point = sequence.get_prev_point(point) + assert [str(out) for out in output] == [ + "20100808T0200Z", + "20100808T0150Z", + "20100808T0140Z", + "20100808T0130Z", + "20100808T0120Z", + "20100808T0110Z", + "20100808T0100Z", + "20100808T0050Z", + "20100808T0040Z", + "20100808T0030Z", + "20100808T0020Z", + "20100808T0010Z", + "20100808T0000Z", + ] + assert not sequence.is_on_sequence(ISO8601Point("20100809T0005")) + + +def test_next_simple(set_cycling_type): + """Test the generation of CP using 'next' from single input.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + my_now = "20100808T1540Z" + sequence = ( + "next(T2100Z)", # 20100808T2100Z + "next(T00)", # 20100809T0000Z + "next(T-15)", # 20100808T1615Z + "next(T-45)", # 20100808T1545Z + "next(-10)", # 21100101T0000Z + "next(-1008)", # 21100801T0000Z + "next(--10)", # 20101001T0000Z + "next(--0325)", # 20110325T0000Z + "next(---10)", # 20100810T0000Z + "next(---05T1200Z)", # 20100905T1200Z + ) + + output = [] + + for point in sequence: + output.append(ingest_time(point, my_now)) + assert output == [ + "20100808T2100Z", + "20100809T0000Z", + "20100808T1615Z", + "20100808T1545Z", + "21100101T0000Z", + "21100801T0000Z", + "20101001T0000Z", + "20110325T0000Z", + "20100810T0000Z", + "20100905T1200Z", + ] + + +def test_previous_simple(set_cycling_type): + """Test the generation of CP using 'previous' from single input.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + my_now = "20100808T1540Z" + sequence = ( + "previous(T2100Z)", # 20100807T2100Z + "previous(T00)", # 20100808T0000Z + "previous(T-15)", # 20100808T1515Z + "previous(T-45)", # 20100808T1445Z + "previous(-10)", # 20100101T0000Z + "previous(-1008)", # 20100801T0000Z + "previous(--10)", # 20091001T0000Z + "previous(--0325)", # 20100325T0000Z + "previous(---10)", # 20100710T0000Z + "previous(---05T1200Z)", # 20100805T1200Z + ) + + output = [] + + for point in sequence: + output.append(ingest_time(point, my_now)) + assert output == [ + "20100807T2100Z", + "20100808T0000Z", + "20100808T1515Z", + "20100808T1445Z", + "20100101T0000Z", + "20100801T0000Z", + "20091001T0000Z", + "20100325T0000Z", + "20100710T0000Z", + "20100805T1200Z", + ] + + +def test_sequence(set_cycling_type): + """Test the generation of CP from list input.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + my_now = "20100808T1540Z" + sequence = ( + "next(T-00;T-15;T-30;T-45)", # 20100808T1545Z + "previous(T-00;T-15;T-30;T-45)", # 20100808T1530Z + "next(T00;T06;T12;T18)", # 20100808T1800Z + "previous(T00;T06;T12;T18)", + ) # 20100808T1200Z + + output = [] + + for point in sequence: + output.append(ingest_time(point, my_now)) + assert output == [ + "20100808T1545Z", + "20100808T1530Z", + "20100808T1800Z", + "20100808T1200Z", + ] + + +def test_offset_simple(set_cycling_type): + """Test the generation of offset CP.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + my_now = "20100808T1540Z" + sequence = ( + "PT15M", # 20100808T1555Z + "-PT30M", # 20100808T1510Z + "PT1H", # 20100808T1640Z + "-PT18H", # 20100807T2140Z + "P3D", # 20100811T1540Z + "-P2W", # 20100725T1540Z + "P6M", # 20110208T1540Z + "-P1M", # 20100708T1540Z + "P1Y", # 20110808T1540Z + "-P5Y", # 20050808T1540Z + ) + + output = [] + + for point in sequence: + output.append(ingest_time(point, my_now)) + assert output == [ + "20100808T1555Z", + "20100808T1510Z", + "20100808T1640Z", + "20100807T2140Z", + "20100811T1540Z", + "20100725T1540Z", + "20110208T1540Z", + "20100708T1540Z", + "20110808T1540Z", + "20050808T1540Z", + ] + + +def test_offset(set_cycling_type): + """Test the generation of offset CP with 'next' and 'previous'.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + my_now = "20100808T1540Z" + sequence = ( + "next(T06) +P1D", # 20100810T0600Z + "previous(T-30) -PT12H", # 20100808T0330Z + "next(T00;T06;T12;T18) -P1W", # 20100801T1800Z + "previous(T00;T06;T12;T18) +PT1H", # 20100808T1300Z + ) + + output = [] + + for point in sequence: + output.append(ingest_time(point, my_now)) + assert output == [ + "20100810T0600Z", + "20100808T0330Z", + "20100801T1800Z", + "20100808T1300Z", + ] + + +def test_weeks_days(set_cycling_type): + """Test the generation of CP with day-of-week, + ordinal day, and week (with day-of-week specified).""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + my_now = "20100808T1540Z" + sequence = ( + "next(-W-1)", # 20100809T0000Z + "previous(-W-4)", # 20100805T0000Z + "next(-010)", # 20110110T0000Z + "previous(-101)", # 20100411T0000Z + "next(-W40-1)", # 20101004T0000Z + "previous(-W05-1)", # 20100201T0000Z + "next(-W05-5)", # 20110204T0000Z + "previous(-W40-4)", # 20091001T0000Z + ) + + + output = [] + + for point in sequence: + output.append(ingest_time(point, my_now)) + assert output == [ + "20100809T0000Z", + "20100805T0000Z", + "20110110T0000Z", + "20100411T0000Z", + "20101004T0000Z", + "20100201T0000Z", + "20110204T0000Z", + "20091001T0000Z", + ] + + +def test_cug(set_cycling_type): + """Test the offset CP examples in the Cylc user guide""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + my_now = "2018-03-14T15:12Z" + sequence = ( + "next(T-00)", # 20180314T1600Z + "previous(T-00)", # 20180314T1500Z + "next(T-00; T-15; T-30; T-45)", # 20180314T1515Z + "previous(T-00; T-15; T-30; T-45)", # 20180314T1500Z + "next(T00)", # 20180315T0000Z + "previous(T00)", # 20180314T0000Z + "next(T06:30Z)", # 20180315T0630Z + "previous(T06:30) -P1D", # 20180313T0630Z + "next(T00; T06; T12; T18)", # 20180314T1800Z + "previous(T00; T06; T12; T18)", # 20180314T1200Z + "next(T00; T06; T12; T18)+P1W", # 20180321T1800Z + "PT1H", # 20180314T1612Z + "-P1M", # 20180214T1512Z + "next(-00)", # 21000101T0000Z + "previous(--01)", # 20180101T0000Z + "next(---01)", # 20180401T0000Z + "previous(--1225)", # 20171225T0000Z + "next(-2006)", # 20200601T0000Z + "previous(-W101)", # 20180305T0000Z + "next(-W-1; -W-3; -W-5)", # 20180314T0000Z + "next(-001; -091; -181; -271)", # 20180401T0000Z + "previous(-365T12Z)", # 20171231T1200Z + ) + + output = [] + + for point in sequence: + output.append(ingest_time(point, my_now)) + assert output == [ + "20180314T1600Z", + "20180314T1500Z", + "20180314T1515Z", + "20180314T1500Z", + "20180315T0000Z", + "20180314T0000Z", + "20180315T0630Z", + "20180313T0630Z", + "20180314T1800Z", + "20180314T1200Z", + "20180321T1800Z", + "20180314T1612Z", + "20180214T1512Z", + "21000101T0000Z", + "20180101T0000Z", + "20180401T0000Z", + "20171225T0000Z", + "20200601T0000Z", + "20180305T0000Z", + "20180314T0000Z", + "20180401T0000Z", + "20171231T1200Z", + ] + + +def test_next_simple_no_now(set_cycling_type): + """Test the generation of CP using 'next' with no value for `now`.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + my_now = None + point = "next(T00Z)+P1D" + output = ingest_time(point, my_now) + + current_time = datetime.utcnow() + # my_now is None, but ingest_time will have used a similar time, and + # the returned value must be after current_time + output_time = datetime.strptime(output, "%Y%m%dT%H%MZ") + assert current_time < output_time + + +def test_integer_cycling_is_returned(set_cycling_type): + """Test that when integer points are given, the same value is + returned.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + integer_point = "1" + assert integer_point, ingest_time(integer_point == None) + + +def test_expanded_dates_are_returned(set_cycling_type): + """Test that when expanded dates are given, the same value is + returned.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + expanded_date = "+0100400101T0000Z" + assert expanded_date, ingest_time(expanded_date == None) + + +def test_timepoint_truncated(set_cycling_type): + """Test that when a timepoint is given, and is truncated, then the + value is added to `now`.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + my_now = "2018-03-14T15:12Z" + timepoint_truncated = "T15:00Z" # 20180315T1500Z + output = ingest_time(timepoint_truncated, my_now) + assert "20180315T1500Z" == output + + +def test_timepoint(set_cycling_type): + """Test that when a timepoint is given, and is not truncated, the + same value is returned.""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") + my_now = "2018-03-14T15:12Z" + timepoint_truncated = "19951231T0630" # 19951231T0630 + output = ingest_time(timepoint_truncated, my_now) + assert "19951231T0630" == output + @pytest.mark.parametrize( - '_input, errortext', + "_input, errortext", ( - ('next (T-00, T-30)', 'T-00;T-30'), - ('next (wildebeest)', 'Invalid ISO 8601 date') - ) + ("next (T-00, T-30)", "T-00;T-30"), + ("next (wildebeest)", "Invalid ISO 8601 date"), + ), ) -def test_validate_fails_comma_sep_offset_list(_input, errortext): - """It raises an exception if validating a list separated by commas - """ +def test_validate_fails_comma_sep_offset_list(_input, errortext, set_cycling_type): + """It raises an exception if validating a list separated by commas""" + set_cycling_type(ISO8601_CYCLING_TYPE, "Z") with pytest.raises(Exception, match=errortext): ingest_time(_input) From 18d1e4b0e5a9c989c21a3c18ecf3fc15ab61e498 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Tue, 7 Mar 2023 13:50:13 +0000 Subject: [PATCH 24/57] Stop searching platform groups after first match. --- CHANGES.md | 3 +++ cylc/flow/platforms.py | 4 ++++ tests/unit/test_platforms_get_platform.py | 17 ++++++++++++----- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index f47d0fc1cc7..d381efb7d79 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -14,6 +14,9 @@ ones in. --> ### Fixes +[5398](https://github.com/cylc/cylc-flow/pull/5398) - Fix platform from +group selection order bug. + [#5384](https://github.com/cylc/cylc-flow/pull/5384) - Fixes `cylc set-verbosity`. diff --git a/cylc/flow/platforms.py b/cylc/flow/platforms.py index 35ffee6bdb5..b40360724d7 100644 --- a/cylc/flow/platforms.py +++ b/cylc/flow/platforms.py @@ -191,6 +191,9 @@ def platform_from_name( platform_name = 'localhost' platform_group = None + # The list is reversed to allow user-set platform groups (which are loaded + # later than site set platforms) to be matched first and override site + # defined platforms. for platform_name_re in reversed(list(platform_groups)): # Platform is member of a group. if re.fullmatch(platform_name_re, platform_name): @@ -198,6 +201,7 @@ def platform_from_name( platform_groups[platform_name_re], group_name=platform_name, bad_hosts=bad_hosts ) + break for platform_name_re in list(platforms): if ( diff --git a/tests/unit/test_platforms_get_platform.py b/tests/unit/test_platforms_get_platform.py index d26af0575b2..5e6a7ff1892 100644 --- a/tests/unit/test_platforms_get_platform.py +++ b/tests/unit/test_platforms_get_platform.py @@ -237,18 +237,25 @@ def test_get_platform_using_platform_name_from_job_info( def test_get_platform_groups_basic(mock_glbl_cfg): - # get platform from group works. + """get platform from group works. + + Additionally, ensure that we stop after selecting the first + appropriate platform. + """ mock_glbl_cfg( 'cylc.flow.platforms.glbl_cfg', ''' [platforms] - [[aleph]] - hosts = aleph - [[bet]] - hosts = bet + [[aleph, bet, alpha, beta]] [platform groups] [[hebrew_letters]] + platforms = alpha, beta + [[[selection]]] + method = definition order + [[aleph]] + platforms = alpha + [[.*_letters]] platforms = aleph, bet [[[selection]]] method = definition order From f8a0ce1e8a29afd0ce1171abe777c177eda66218 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 9 Mar 2023 09:43:28 +0000 Subject: [PATCH 25/57] Update Gedit syntax highlighting instructions (#5402) --- cylc/flow/etc/syntax/cylc.lang | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cylc/flow/etc/syntax/cylc.lang b/cylc/flow/etc/syntax/cylc.lang index 43af726f773..2de9106f360 100644 --- a/cylc/flow/etc/syntax/cylc.lang +++ b/cylc/flow/etc/syntax/cylc.lang @@ -10,8 +10,7 @@ /usr/share/gtksourceview-2.0/language-specs/ If your installation uses GNOME 3, the '2.0' in the paths will need - to be '3.0', and the version="2.0" string below will need changing - as well. + to be '3.0'. If using gedit, gedit will need to be totally closed down and reloaded. From 23d27b60b822266314312b19a05f7088d9835c59 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 9 Mar 2023 10:31:51 +0000 Subject: [PATCH 26/57] Update tests/unit/test_platforms_get_platform.py Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- tests/unit/test_platforms_get_platform.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/tests/unit/test_platforms_get_platform.py b/tests/unit/test_platforms_get_platform.py index 5e6a7ff1892..d5af21d5834 100644 --- a/tests/unit/test_platforms_get_platform.py +++ b/tests/unit/test_platforms_get_platform.py @@ -254,6 +254,9 @@ def test_get_platform_groups_basic(mock_glbl_cfg): [[[selection]]] method = definition order [[aleph]] + # Group with same name as platform to try and + # trip up the platform selection logic after it + # has processed [[.*_letters]] below platforms = alpha [[.*_letters]] platforms = aleph, bet From e043222e8d2b0ec7579a1b23c1f9d6b899321409 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Fri, 10 Mar 2023 15:55:06 +0000 Subject: [PATCH 27/57] Apply suggestions from code review Co-authored-by: Hilary James Oliver --- cylc/flow/job_runner_handlers/documentation.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/cylc/flow/job_runner_handlers/documentation.py b/cylc/flow/job_runner_handlers/documentation.py index ff931ff25ea..cc32b0d7dd9 100644 --- a/cylc/flow/job_runner_handlers/documentation.py +++ b/cylc/flow/job_runner_handlers/documentation.py @@ -74,6 +74,7 @@ class MyHandler(): You can then define a Cylc platform using the handler: .. code-block:: cylc + :caption: global.cylc [platforms] [[my_platform]] @@ -83,6 +84,7 @@ class MyHandler(): And configure tasks to submit to it: .. code-block:: cylc + :caption: flow.cylc [runtime] [[my_task]] @@ -185,7 +187,7 @@ class QSUBHandler(PBSHandler): .. note:: - Don't subclass this module as it provides optional interfaces which + Don't subclass this class as it provides optional interfaces which you may not want to inherit. """ @@ -243,7 +245,7 @@ class QSUBHandler(PBSHandler): """ REC_ID_FROM_SUBMIT_OUT: re.Pattern - """Regular expression to extract job ids from submission stderr. + """Regular expression to extract job IDs from submission stderr. A regular expression (compiled) to extract the job "id" from the standard output or standard error of the job submission command. @@ -251,7 +253,7 @@ class QSUBHandler(PBSHandler): """ REC_ID_FROM_SUBMIT_ERR: re.Pattern - """Regular expression to extract job ids from submission stderr. + """Regular expression to extract job IDs from submission stderr. See :py:attr:`ExampleHandler.REC_ID_FROM_SUBMIT_OUT`. @@ -289,7 +291,7 @@ def filter_poll_many_output(self, out: str) -> List[str]: out: Job poll stdout. Returns: - List of job ids + List of job IDs """ raise NotImplementedError() @@ -315,7 +317,7 @@ def filter_submit_output(self, out: str, err: str) -> Tuple[str, str]: def format_directives(self, job_conf: dict) -> List[str]: """Returns lines to be appended to the job script. - If relevant, this method formats the job directives for a job file, if + This method formats the job directives for a job file, if job file directives are relevant for the job runner. The argument "job_conf" is a dict containing the job configuration. @@ -332,7 +334,7 @@ def get_poll_many_cmd(self, job_id_list: List[str]) -> List[str]: """Return a command to poll the specified jobs. Args: - job_id_list: The listof job ids to poll. + job_id_list: The list of job IDs to poll. Returns: command e.g. ['foo', '--bar', 'baz'] From 068d9a6407b20f477c719721f162fb6401e5c5cf Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Tue, 14 Mar 2023 10:14:43 +0000 Subject: [PATCH 28/57] Fix bandit warnings - Unused rundb.py code - remote-init contact file extraction - Trusted input in subprocess call Plus tidy --- cylc/flow/pathutil.py | 6 ++++- cylc/flow/rundb.py | 43 ------------------------------ cylc/flow/subprocpool.py | 3 ++- cylc/flow/task_remote_cmd.py | 7 ++--- cylc/flow/task_remote_mgr.py | 41 +++++++++++++++------------- tests/unit/test_rundb.py | 30 +-------------------- tests/unit/test_task_remote_mgr.py | 31 +++++++++++++++++++-- 7 files changed, 64 insertions(+), 97 deletions(-) diff --git a/cylc/flow/pathutil.py b/cylc/flow/pathutil.py index 0d62ef37ad4..52066bc4a52 100644 --- a/cylc/flow/pathutil.py +++ b/cylc/flow/pathutil.py @@ -427,7 +427,11 @@ def parse_rm_dirs(rm_dirs: Iterable[str]) -> Set[str]: def is_relative_to(path1: Union[Path, str], path2: Union[Path, str]) -> bool: - """Return whether or not path1 is relative to path2.""" + """Return whether or not path1 is relative to path2. + + Normalizes both paths to avoid trickery with paths containing `..` + somewhere in them. + """ # In future, we can just use pathlib.Path.is_relative_to() # when Python 3.9 becomes the minimum supported version try: diff --git a/cylc/flow/rundb.py b/cylc/flow/rundb.py index 0ef237b41bf..e0a18b61c8d 100644 --- a/cylc/flow/rundb.py +++ b/cylc/flow/rundb.py @@ -1055,46 +1055,3 @@ def select_jobs_for_datastore( def vacuum(self): """Vacuum to the database.""" return self.connect().execute("VACUUM") - - def remove_columns(self, table, to_drop): - conn = self.connect() - - # get list of columns to keep - schema = conn.execute( - rf''' - PRAGMA table_info({table}) - ''' - ) - new_cols = [ - name - for _, name, *_ in schema - if name not in to_drop - ] - - # copy table - conn.execute( - rf''' - CREATE TABLE {table}_new AS - SELECT {', '.join(new_cols)} - FROM {table} - ''' - ) - - # remove original - conn.execute( - rf''' - DROP TABLE {table} - ''' - ) - - # copy table - conn.execute( - rf''' - CREATE TABLE {table} AS - SELECT {', '.join(new_cols)} - FROM {table}_new - ''' - ) - - # done - conn.commit() diff --git a/cylc/flow/subprocpool.py b/cylc/flow/subprocpool.py index 05648631fb3..e4e64755fc4 100644 --- a/cylc/flow/subprocpool.py +++ b/cylc/flow/subprocpool.py @@ -575,9 +575,10 @@ def rsync_255_fail(ctx, platform=None) -> bool: f'testing connectivity for {ctx.host} using {ssh_test_cmd}' ) ssh_test = run( - shlex.split(f'{ssh_cmd} {ctx.host} true'), + shlex.split(f'{ssh_cmd} {ctx.host} true'), # nosec B603 * capture_output=True ) + # * (command is trusted input) if ssh_test.returncode == 255: rsync_255_fail = True elif ( diff --git a/cylc/flow/task_remote_cmd.py b/cylc/flow/task_remote_cmd.py index aa55ab2a972..28774220b55 100644 --- a/cylc/flow/task_remote_cmd.py +++ b/cylc/flow/task_remote_cmd.py @@ -151,10 +151,11 @@ def remote_init(install_target: str, rund: str, *dirs_to_symlink: str) -> None: 'job.sh', os.path.join(WorkflowFiles.Service.DIRNAME, 'etc') ) + # Extract sent contact file from stdin: try: - tarhandle = tarfile.open(fileobj=sys.stdin.buffer, mode='r|') - tarhandle.extractall() - tarhandle.close() + with tarfile.open(fileobj=sys.stdin.buffer, mode='r|') as tarhandle: + tarhandle.extractall() # nosec B202 - there should not be any + # untrusted members in the tar stream, only the contact file finally: os.chdir(oldcwd) print("KEYSTART", end='') diff --git a/cylc/flow/task_remote_mgr.py b/cylc/flow/task_remote_mgr.py index 5a5f39ece1d..59b6e04cf42 100644 --- a/cylc/flow/task_remote_mgr.py +++ b/cylc/flow/task_remote_mgr.py @@ -212,21 +212,22 @@ def remote_init( # Determine what items to install comms_meth: CommsMeth = CommsMeth(platform['communication method']) - items = self._remote_init_items(comms_meth) + remote_init_items = self._remote_init_items(comms_meth) # Create a TAR archive with the service files, # so they can be sent later via SSH's STDIN to the task remote. tmphandle = self.proc_pool.get_temporary_file() - tarhandle = tarfile.open(fileobj=tmphandle, mode='w') - for path, arcname in items: - tarhandle.add(path, arcname=arcname) - tarhandle.close() + with tarfile.open(fileobj=tmphandle, mode='w') as tarhandle: + for path, arcname in remote_init_items: + tarhandle.add(path, arcname=arcname) tmphandle.seek(0) # Build the remote-init command to be run over ssh - cmd = ['remote-init'] - cmd.extend(verbosity_to_opts(cylc.flow.flags.verbosity)) - cmd.append(str(install_target)) - cmd.append(get_remote_workflow_run_dir(self.workflow)) + cmd = [ + 'remote-init', + *verbosity_to_opts(cylc.flow.flags.verbosity), + str(install_target), + get_remote_workflow_run_dir(self.workflow) + ] dirs_to_symlink = get_dirs_to_symlink(install_target, self.workflow) for key, value in dirs_to_symlink.items(): if value is not None: @@ -584,23 +585,27 @@ def get_log_file_name( file_name = f"{log_num:02d}-{load_type}-{install_target}.log" return file_name - def _remote_init_items(self, comms_meth: CommsMeth): + def _remote_init_items( + self, comms_meth: CommsMeth + ) -> List[Tuple[str, str]]: """Return list of items to install based on communication method. + (At the moment this is only the contact file.) + Return (list): Each item is (source_path, dest_path) where: - source_path is the path to the source file to install. - dest_path is relative path under workflow run directory at target remote. """ - items = [] - - if comms_meth in [CommsMeth.SSH, CommsMeth.ZMQ]: - # Contact file - items.append(( + if comms_meth not in {CommsMeth.SSH, CommsMeth.ZMQ}: + return [] + return [ + ( get_contact_file_path(self.workflow), os.path.join( WorkflowFiles.Service.DIRNAME, - WorkflowFiles.Service.CONTACT))) - - return items + WorkflowFiles.Service.CONTACT + ) + ) + ] diff --git a/tests/unit/test_rundb.py b/tests/unit/test_rundb.py index c17e9955513..a0754659dc6 100644 --- a/tests/unit/test_rundb.py +++ b/tests/unit/test_rundb.py @@ -98,35 +98,7 @@ def create_temp_db(): conn.close() # doesn't raise error on re-invocation -def test_remove_columns(): - """Test workaround for dropping columns in sqlite3.""" - with create_temp_db() as (temp_db, conn): - conn.execute( - r''' - CREATE TABLE foo ( - bar, - baz, - pub - ) - ''' - ) - conn.execute( - r''' - INSERT INTO foo - VALUES (?,?,?) - ''', - ['BAR', 'BAZ', 'PUB'] - ) - conn.commit() - conn.close() - - with CylcWorkflowDAO(temp_db) as dao: - dao.remove_columns('foo', ['bar', 'baz']) - data = list(dao.connect().execute(r'SELECT * from foo')) - assert data == [('PUB',)] - - -def test_operational_error(monkeypatch, tmp_path, caplog): +def test_operational_error(tmp_path, caplog): """Test logging on operational error.""" # create a db object db_file = tmp_path / 'db' diff --git a/tests/unit/test_task_remote_mgr.py b/tests/unit/test_task_remote_mgr.py index 0127ecd8dde..bb7b8cea085 100644 --- a/tests/unit/test_task_remote_mgr.py +++ b/tests/unit/test_task_remote_mgr.py @@ -23,10 +23,37 @@ from cylc.flow.network.client_factory import CommsMeth from cylc.flow.task_remote_mgr import ( REMOTE_FILE_INSTALL_DONE, REMOTE_INIT_IN_PROGRESS, TaskRemoteMgr) +from cylc.flow.workflow_files import WorkflowFiles, get_workflow_srv_dir Fixture = Any +@pytest.mark.parametrize( + 'comms_meth, expected', + [ + (CommsMeth.SSH, True), + (CommsMeth.ZMQ, True), + (CommsMeth.POLL, False) + ] +) +def test__remote_init_items(comms_meth: CommsMeth, expected: bool): + """Test _remote_init_items(). + + Should only includes files under .service/ + """ + reg = 'barclay' + mock_mgr = Mock(workflow=reg) + srv_dir = get_workflow_srv_dir(reg) + items = TaskRemoteMgr._remote_init_items(mock_mgr, comms_meth) + if expected: + assert items + for src_path, dst_path in items: + Path(src_path).relative_to(srv_dir) + Path(dst_path).relative_to(WorkflowFiles.Service.DIRNAME) + else: + assert not items + + @pytest.mark.parametrize( 'install_target, skip_expected, expected_status', [('localhost', True, REMOTE_FILE_INSTALL_DONE), @@ -81,7 +108,7 @@ def test_remote_init_skip( '03-reload-another_install_target.log') ] ) -def test_get_log_file_name(tmp_path, +def test_get_log_file_name(tmp_path: Path, install_target: str, load_type: Optional[str], expected: str): @@ -92,7 +119,7 @@ def test_get_log_file_name(tmp_path, task_remote_mgr.is_reload = True # else load type is start (no flag required) run_dir = tmp_path - log_dir = Path(run_dir/'some_workflow'/'log'/'remote-install') + log_dir = run_dir / 'some_workflow' / 'log' / 'remote-install' log_dir.mkdir(parents=True) for log_num in range(1, 3): Path(f"{log_dir}/{log_num:02d}-start-{install_target}.log").touch() From 955d5e34616ddc6525e1ee9a10734e730b2d3752 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 17 Mar 2023 11:54:13 +0000 Subject: [PATCH 29/57] Fix test (#5415) add upgrade switch to a test using an older db --- .../job-submission/03-job-nn-remote-host-with-shared-fs.t | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/functional/job-submission/03-job-nn-remote-host-with-shared-fs.t b/tests/functional/job-submission/03-job-nn-remote-host-with-shared-fs.t index 3d3922701a5..a9152e3a094 100755 --- a/tests/functional/job-submission/03-job-nn-remote-host-with-shared-fs.t +++ b/tests/functional/job-submission/03-job-nn-remote-host-with-shared-fs.t @@ -25,7 +25,7 @@ run_ok "${TEST_NAME_BASE}-validate" cylc validate "${WORKFLOW_NAME}" mkdir "${WORKFLOW_RUN_DIR}/.service" sqlite3 "${WORKFLOW_RUN_DIR}/.service/db" <'db.sqlite3' workflow_run_ok "${TEST_NAME_BASE}-restart" \ - cylc play --reference-test --debug --no-detach "${WORKFLOW_NAME}" + cylc play --reference-test --debug --no-detach "${WORKFLOW_NAME}" --upgrade purge exit From ff0dbb8e6f2380884ff9081fe74fe5c41a7ad247 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 2 Feb 2023 10:50:13 +0000 Subject: [PATCH 30/57] Avoid running host check on platform names - this doesn't make any sense. --- CHANGES.md | 6 +++ cylc/flow/task_job_mgr.py | 4 +- .../10-do-not-host-check-platforms.t | 52 +++++++++++++++++++ 3 files changed, 61 insertions(+), 1 deletion(-) create mode 100755 tests/functional/platforms/10-do-not-host-check-platforms.t diff --git a/CHANGES.md b/CHANGES.md index d381efb7d79..03150dba968 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -38,6 +38,12 @@ Rose options (`-O`, `-S` & `-D`) with `cylc view`. [#5363](https://github.com/cylc/cylc-flow/pull/5363) Improvements and bugfixes for `cylc lint`. +------------------------------------------------------------------------------- +## __cylc-8.1.2 (Coming Soon)__ + +[#5343](https://github.com/cylc/cylc-flow/pull/5343) - Fix a bug causing +platform names to be checked as if they were hosts. + ------------------------------------------------------------------------------- ## __cylc-8.1.1 (Released 2023-01-31)__ diff --git a/cylc/flow/task_job_mgr.py b/cylc/flow/task_job_mgr.py index da995bac846..1406b496f11 100644 --- a/cylc/flow/task_job_mgr.py +++ b/cylc/flow/task_job_mgr.py @@ -1125,7 +1125,9 @@ def _prep_submit_task_job( ) else: platform_name = self.task_remote_mgr.subshell_eval( - rtconfig['platform'], PLATFORM_REC_COMMAND + rtconfig['platform'], + PLATFORM_REC_COMMAND, + host_check=False ) except PlatformError as exc: itask.waiting_on_job_prep = False diff --git a/tests/functional/platforms/10-do-not-host-check-platforms.t b/tests/functional/platforms/10-do-not-host-check-platforms.t new file mode 100755 index 00000000000..7ba5dc0218f --- /dev/null +++ b/tests/functional/platforms/10-do-not-host-check-platforms.t @@ -0,0 +1,52 @@ +#!/usr/bin/env bash +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) 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 . +#------------------------------------------------------------------------------- +# Check that we don't check whether the platform name is a valid host. +. "$(dirname "$0")/test_header" + +set_test_number 2 + +# shellcheck disable=SC2016 +create_test_global_config '' ' +[platforms] + [[localhost_spice]] + hosts = unreachable +' + +make_rnd_workflow + +cat > "${RND_WORKFLOW_SOURCE}/flow.cylc" <<__HEREDOC__ +[scheduler] + [[events]] + stall timeout = PT0S +[scheduling] + [[graph]] + R1 = foo +[runtime] + [[foo]] + platform = localhost_spice +__HEREDOC__ + +ERR_STR='Unable to find valid host for localhost_spice' + +TEST_NAME="${TEST_NAME_BASE}-vip-workflow" +run_fail "${TEST_NAME}" cylc vip "${RND_WORKFLOW_SOURCE}" --no-detach +grep_ok "${ERR_STR}" \ + "${TEST_NAME}.stderr" -F + +purge_rnd_workflow +exit From eaa11604ba6a1564db13420d200f83da8590957e Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 17 Feb 2023 15:01:42 +0000 Subject: [PATCH 31/57] clarification of nomenclature --- cylc/flow/task_remote_mgr.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/cylc/flow/task_remote_mgr.py b/cylc/flow/task_remote_mgr.py index 59b6e04cf42..7235bf73679 100644 --- a/cylc/flow/task_remote_mgr.py +++ b/cylc/flow/task_remote_mgr.py @@ -149,7 +149,7 @@ def subshell_eval(self, command, command_pattern, host_check=True): elif value is None: return # command not yet ready else: - command = value # command succeeded + platform_or_host_str = value # command succeeded else: # Command not launched (or already reset) self.proc_pool.put_command( @@ -164,16 +164,17 @@ def subshell_eval(self, command, command_pattern, host_check=True): return self.remote_command_map[cmd_str] # Environment variable substitution - command = os.path.expandvars(command) + platform_or_host_str = os.path.expandvars(platform_or_host_str) + # Remote? # TODO - Remove at Cylc 8.x as this only makes sense with host logic if host_check is True: - if is_remote_host(command): - return command + if is_remote_host(platform_or_host_str): + return platform_or_host_str else: return 'localhost' else: - return command + return platform_or_host_str def subshell_eval_reset(self): """Reset remote eval subshell results. From 6a211574f5ebc8fefd7048ec87a4123463541578 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 17 Feb 2023 15:45:36 +0000 Subject: [PATCH 32/57] undo mistake --- cylc/flow/task_remote_mgr.py | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/cylc/flow/task_remote_mgr.py b/cylc/flow/task_remote_mgr.py index 7235bf73679..59b6e04cf42 100644 --- a/cylc/flow/task_remote_mgr.py +++ b/cylc/flow/task_remote_mgr.py @@ -149,7 +149,7 @@ def subshell_eval(self, command, command_pattern, host_check=True): elif value is None: return # command not yet ready else: - platform_or_host_str = value # command succeeded + command = value # command succeeded else: # Command not launched (or already reset) self.proc_pool.put_command( @@ -164,17 +164,16 @@ def subshell_eval(self, command, command_pattern, host_check=True): return self.remote_command_map[cmd_str] # Environment variable substitution - platform_or_host_str = os.path.expandvars(platform_or_host_str) - + command = os.path.expandvars(command) # Remote? # TODO - Remove at Cylc 8.x as this only makes sense with host logic if host_check is True: - if is_remote_host(platform_or_host_str): - return platform_or_host_str + if is_remote_host(command): + return command else: return 'localhost' else: - return platform_or_host_str + return command def subshell_eval_reset(self): """Reset remote eval subshell results. From 83484ef1cd8e4163ba184ab9b595bb3c37ec6b74 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Mon, 13 Mar 2023 10:42:09 +0000 Subject: [PATCH 33/57] update comment on localhost check and add test for case localhost4.localhost42 --- cylc/flow/hostuserutil.py | 2 +- tests/unit/test_hostuserutil.py | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/cylc/flow/hostuserutil.py b/cylc/flow/hostuserutil.py index 782a193746a..36154965246 100644 --- a/cylc/flow/hostuserutil.py +++ b/cylc/flow/hostuserutil.py @@ -198,7 +198,7 @@ def is_remote_host(self, name): """ if name not in self._remote_hosts: if not name or name.split(".")[0].startswith("localhost"): - # e.g. localhost.localdomain + # e.g. localhost42.localdomain42 self._remote_hosts[name] = False else: try: diff --git a/tests/unit/test_hostuserutil.py b/tests/unit/test_hostuserutil.py index f9d9d745262..aa21632530a 100644 --- a/tests/unit/test_hostuserutil.py +++ b/tests/unit/test_hostuserutil.py @@ -35,10 +35,11 @@ def test_is_remote_user_on_current_user(): assert not is_remote_user(os.getenv('USER')) -def test_is_remote_host_on_localhost(): +def test_is_remote_host_on_localhost(monkeypatch): """is_remote_host with localhost.""" assert not is_remote_host(None) assert not is_remote_host('localhost') + assert not is_remote_host('localhost4.localhost42') assert not is_remote_host(os.getenv('HOSTNAME')) assert not is_remote_host(get_host()) From 5cd1a63b78597658f6a54eb724c1e9e545eecae2 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Mon, 20 Mar 2023 14:55:54 +0000 Subject: [PATCH 34/57] small changlog error fix --- CHANGES.md | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 03150dba968..cf193545197 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -24,6 +24,9 @@ Fixes `cylc set-verbosity`. absence of `job name length maximum` in PBS platform settings would cause Cylc to crash when preparing the job script. +[#5343](https://github.com/cylc/cylc-flow/pull/5343) - Fix a bug causing +platform names to be checked as if they were hosts. + ------------------------------------------------------------------------------- ## __cylc-8.1.2 (Released 2023-02-20)__ @@ -38,12 +41,6 @@ Rose options (`-O`, `-S` & `-D`) with `cylc view`. [#5363](https://github.com/cylc/cylc-flow/pull/5363) Improvements and bugfixes for `cylc lint`. -------------------------------------------------------------------------------- -## __cylc-8.1.2 (Coming Soon)__ - -[#5343](https://github.com/cylc/cylc-flow/pull/5343) - Fix a bug causing -platform names to be checked as if they were hosts. - ------------------------------------------------------------------------------- ## __cylc-8.1.1 (Released 2023-01-31)__ From 121eba69c2bb8d8345bd83282e5af4b6e4671d4d Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Wed, 22 Mar 2023 17:05:08 +0000 Subject: [PATCH 35/57] Fix cleanup_sysargv (for remote play in vip and vr) (#5377) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fix bug in cylc vr * Cylc VR parses workflow id and passes the parsed id to the sys.argv cleanup function * As a result the sys.argv cleaner adds a second copy of the workflow name if the parsed id ≠ to the unparsed i; so the sys.argv is `cylc play workflowid/run workflowid` which fails to work. --- cylc/flow/option_parsers.py | 6 +- cylc/flow/platforms.py | 4 +- cylc/flow/scripts/validate_reinstall.py | 5 +- .../cylc-combination-scripts/01-vr-reload.t | 1 - .../cylc-combination-scripts/02-vr-restart.t | 9 +-- .../cylc-combination-scripts/07-vr-remote.t | 55 +++++++++++++++++++ .../vr_workflow_stop/flow.cylc | 9 +++ 7 files changed, 76 insertions(+), 13 deletions(-) create mode 100644 tests/functional/cylc-combination-scripts/07-vr-remote.t create mode 100644 tests/functional/cylc-combination-scripts/vr_workflow_stop/flow.cylc diff --git a/cylc/flow/option_parsers.py b/cylc/flow/option_parsers.py index 3a0ea9d6212..51fdfa2a8d6 100644 --- a/cylc/flow/option_parsers.py +++ b/cylc/flow/option_parsers.py @@ -846,12 +846,14 @@ def cleanup_sysargv( sys.argv.append(workflow_id) -def log_subcommand(command, workflow_id): +def log_subcommand(*args): """Log a command run as part of a sequence. Example: >>> log_subcommand('ruin', 'my_workflow') \x1b[1m\x1b[36m$ cylc ruin my_workflow\x1b[0m\x1b[1m\x1b[0m\n """ + # Args might be posixpath or similar. + args = [str(a) for a in args] print(cparse( - f'$ cylc {command} {workflow_id}')) + f'$ cylc {" ".join(args)}')) diff --git a/cylc/flow/platforms.py b/cylc/flow/platforms.py index b40360724d7..6f1b4b1be03 100644 --- a/cylc/flow/platforms.py +++ b/cylc/flow/platforms.py @@ -192,8 +192,8 @@ def platform_from_name( platform_group = None # The list is reversed to allow user-set platform groups (which are loaded - # later than site set platforms) to be matched first and override site - # defined platforms. + # later than site set platform groups) to be matched first and override + # site defined platform groups. for platform_name_re in reversed(list(platform_groups)): # Platform is member of a group. if re.fullmatch(platform_name_re, platform_name): diff --git a/cylc/flow/scripts/validate_reinstall.py b/cylc/flow/scripts/validate_reinstall.py index 0efbe667a81..5341b2167c2 100644 --- a/cylc/flow/scripts/validate_reinstall.py +++ b/cylc/flow/scripts/validate_reinstall.py @@ -125,6 +125,7 @@ def vro_cli(parser: COP, options: 'Values', workflow_id: str): # Attempt to work out whether the workflow is running. # We are trying to avoid reinstalling then subsequently being # unable to play or reload because we cannot identify workflow state. + unparsed_wid = workflow_id workflow_id, *_ = parse_id( workflow_id, constraint='workflows', @@ -178,7 +179,7 @@ def vro_cli(parser: COP, options: 'Values', workflow_id: str): else: cleanup_sysargv( 'play', - workflow_id, + unparsed_wid, options, compound_script_opts=VR_OPTIONS, script_opts=( @@ -187,5 +188,5 @@ def vro_cli(parser: COP, options: 'Values', workflow_id: str): ), source='', # Intentionally blank ) - log_subcommand('play', workflow_id) + log_subcommand(*sys.argv[1:]) scheduler_cli(options, workflow_id) diff --git a/tests/functional/cylc-combination-scripts/01-vr-reload.t b/tests/functional/cylc-combination-scripts/01-vr-reload.t index 2091b01d111..3b63c64d896 100644 --- a/tests/functional/cylc-combination-scripts/01-vr-reload.t +++ b/tests/functional/cylc-combination-scripts/01-vr-reload.t @@ -36,7 +36,6 @@ poll_workflow_running # It validates and reloads: -sed -i 's@P1Y@P5Y@' flow.cylc run_ok "${TEST_NAME_BASE}-runs" cylc vr "${WORKFLOW_NAME}" # Grep for VR reporting revalidation, reinstallation and reloading diff --git a/tests/functional/cylc-combination-scripts/02-vr-restart.t b/tests/functional/cylc-combination-scripts/02-vr-restart.t index 2a746a4c769..9238c45b723 100644 --- a/tests/functional/cylc-combination-scripts/02-vr-restart.t +++ b/tests/functional/cylc-combination-scripts/02-vr-restart.t @@ -28,23 +28,20 @@ WORKFLOW_NAME="cylctb-x$(< /dev/urandom tr -dc _A-Z-a-z-0-9 | head -c6)" cp "${TEST_SOURCE_DIR}/vr_workflow/flow.cylc" . run_ok "setup (install)" \ cylc install \ - --workflow-name "${WORKFLOW_NAME}" \ - --no-run-name + --workflow-name "${WORKFLOW_NAME}" export WORKFLOW_RUN_DIR="${RUN_DIR}/${WORKFLOW_NAME}" # It validates and restarts: -# Change source workflow and run vr: -sed -i 's@P1Y@P5Y@' flow.cylc +# Run VR run_ok "${TEST_NAME_BASE}-runs" cylc vr "${WORKFLOW_NAME}" # Grep for vr reporting revalidation, reinstallation and playing the workflow. grep "\$" "${TEST_NAME_BASE}-runs.stdout" > VIPOUT.txt named_grep_ok "${TEST_NAME_BASE}-it-revalidated" "$ cylc validate --against-source" "VIPOUT.txt" named_grep_ok "${TEST_NAME_BASE}-it-installed" "$ cylc reinstall" "VIPOUT.txt" -named_grep_ok "${TEST_NAME_BASE}-it-played" "$ cylc play" "VIPOUT.txt" - +named_grep_ok "${TEST_NAME_BASE}-it-played" "cylc play" "VIPOUT.txt" # Clean Up. run_ok "teardown (stop workflow)" cylc stop "${WORKFLOW_NAME}" --now --now diff --git a/tests/functional/cylc-combination-scripts/07-vr-remote.t b/tests/functional/cylc-combination-scripts/07-vr-remote.t new file mode 100644 index 00000000000..cb343501acf --- /dev/null +++ b/tests/functional/cylc-combination-scripts/07-vr-remote.t @@ -0,0 +1,55 @@ +#!/usr/bin/env bash +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) 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 `cylc vr` (Validate Reinstall restart) +# Test that args for re-invocation are correct: +export REQUIRE_PLATFORM='loc:remote runner:background fs:shared' +. "$(dirname "$0")/test_header" + +set_test_number 3 + +create_test_global_config '' """ +[scheduler] + [[run hosts]] + available = ${CYLC_TEST_HOST} + +""" + +# Setup +WORKFLOW_NAME="cylctb-x$(< /dev/urandom tr -dc _A-Z-a-z-0-9 | head -c6)" +cp "${TEST_SOURCE_DIR}/vr_workflow_stop/flow.cylc" . +run_ok "${TEST_NAME_BASE}-install" \ + cylc vip \ + --workflow-name "${WORKFLOW_NAME}" \ + --no-detach + +# It validates and restarts: +# Change source workflow and run vr: +TEST_NAME="${TEST_NAME_BASE}-reinvoke" +run_ok "${TEST_NAME}" cylc vr "${WORKFLOW_NAME}" --no-detach + +ls "${RUN_DIR}/${WORKFLOW_NAME}/runN/log/scheduler" > logdir.txt +cmp_ok logdir.txt <<__HERE__ +01-start-01.log +02-start-01.log +log +__HERE__ + +# Clean Up. +purge +exit 0 diff --git a/tests/functional/cylc-combination-scripts/vr_workflow_stop/flow.cylc b/tests/functional/cylc-combination-scripts/vr_workflow_stop/flow.cylc new file mode 100644 index 00000000000..0b278155d7c --- /dev/null +++ b/tests/functional/cylc-combination-scripts/vr_workflow_stop/flow.cylc @@ -0,0 +1,9 @@ +[scheduler] + allow implicit tasks = true + [[events]] + startup handlers = cylc stop --now --now %(workflow)s + +[scheduling] + initial cycle point = 1500 + [[graph]] + P1Y = foo From 65683fabf2a93f4b661487fb213be4aa89f633d6 Mon Sep 17 00:00:00 2001 From: Melanie Hall <37735232+datamel@users.noreply.github.com> Date: Tue, 28 Mar 2023 11:15:32 +0100 Subject: [PATCH 36/57] Fix cylc clean while cat-log running (#5359) clean: fix issue where cat-log can block clean on NFS Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Co-authored-by: Oliver Sanders --- CHANGES.md | 6 ++++- cylc/flow/cfgspec/globalcfg.py | 3 ++- cylc/flow/pathutil.py | 49 +++++++++++++++++++++++++++++++--- cylc/flow/scripts/clean.py | 10 ++++--- 4 files changed, 58 insertions(+), 10 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 47630ac289b..5410f3c2376 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -23,10 +23,14 @@ Fixes `cylc set-verbosity`. [#5394](https://github.com/cylc/cylc-flow/pull/5394) - Fixes a possible scheduler traceback observed with remote task polling. -[#5386](https://github.com/cylc/cylc-flow/pull/5386) Fix bug where +[#5386](https://github.com/cylc/cylc-flow/pull/5386) - Fix bug where absence of `job name length maximum` in PBS platform settings would cause Cylc to crash when preparing the job script. +[#5359](https://github.com/cylc/cylc-flow/pull/5359) - Fix bug where viewing +a workflow's log in the GUI or using `cylc cat-log` would prevent `cylc clean` +from working. + ------------------------------------------------------------------------------- ## __cylc-8.1.2 (Released 2023-02-20)__ diff --git a/cylc/flow/cfgspec/globalcfg.py b/cylc/flow/cfgspec/globalcfg.py index 17ca5d98ddd..669e4ec6464 100644 --- a/cylc/flow/cfgspec/globalcfg.py +++ b/cylc/flow/cfgspec/globalcfg.py @@ -1426,7 +1426,8 @@ def default_for( "[remote]retrieve job logs retry delays")} ''') Conf('tail command template', - VDR.V_STRING, 'tail -n +1 -F %(filename)s', desc=f''' + VDR.V_STRING, 'tail -n +1 --follow=name -F %(filename)s', + desc=f''' A command template (with ``%(filename)s`` substitution) to tail-follow job logs this platform, by ``cylc cat-log``. diff --git a/cylc/flow/pathutil.py b/cylc/flow/pathutil.py index 52066bc4a52..c08b638e1f4 100644 --- a/cylc/flow/pathutil.py +++ b/cylc/flow/pathutil.py @@ -15,16 +15,18 @@ # along with this program. If not, see . """Functions to return paths to common workflow files and directories.""" +import errno import os from pathlib import Path import re from shutil import rmtree +from time import sleep from typing import Dict, Iterable, Set, Union, Optional, Any from cylc.flow import LOG from cylc.flow.cfgspec.glbl_cfg import glbl_cfg from cylc.flow.exceptions import ( - InputError, WorkflowFilesError, handle_rmtree_err + FileRemovalError, InputError, WorkflowFilesError ) from cylc.flow.platforms import get_localhost_install_target @@ -297,7 +299,7 @@ def remove_dir_and_target(path: Union[Path, str]) -> None: "Removing symlink and its target directory: " f"{path} -> {target}" ) - rmtree(target, onerror=handle_rmtree_err) + _rmtree(target) else: LOG.info(f'Removing broken symlink: {path}') os.remove(path) @@ -305,7 +307,46 @@ def remove_dir_and_target(path: Union[Path, str]) -> None: raise FileNotFoundError(path) else: LOG.info(f'Removing directory: {path}') - rmtree(path, onerror=handle_rmtree_err) + _rmtree(path) + + +def _rmtree( + target: Union[Path, str], + retries: int = 10, + sleep_time: float = 1, +): + """Make rmtree more robust to nfs issues. + + If a file is deleted which is being held open for reading by + another process. NFS will create a ".nfs" file in the + containing directory to handle this. + + If you try to delete the directory which contains these + files you will get either a ENOTEMPTY or EBUSY error. + + A likely cause of open file handles in cylc-run directories + is `cylc cat-log -m t`. If the file being cat-log'ged is removed, + the command will fail on its next poll. The default poll + interval is one second, so if we wait a couple of seconds and + retry the removal it will likely work. + + This command retries removal a specified number + of times at a specified interval before failing to + give cat-log process a chance to die gracefully and + release their filesystem locks. For more info see: + https://github.com/cylc/cylc-flow/pull/5359#issuecomment-1479989975 + """ + for _try_num in range(retries): + try: + rmtree(target) + return + except OSError as exc: + if exc.errno in {errno.ENOTEMPTY, errno.EBUSY}: + err = exc + sleep(sleep_time) + else: + raise + raise FileRemovalError(err) def remove_dir_or_file(path: Union[Path, str]) -> None: @@ -325,7 +366,7 @@ def remove_dir_or_file(path: Union[Path, str]) -> None: os.remove(path) else: LOG.info(f"Removing directory: {path}") - rmtree(path, onerror=handle_rmtree_err) + _rmtree(path) def remove_empty_parents( diff --git a/cylc/flow/scripts/clean.py b/cylc/flow/scripts/clean.py index b53e9120692..317ed77b192 100644 --- a/cylc/flow/scripts/clean.py +++ b/cylc/flow/scripts/clean.py @@ -193,15 +193,17 @@ async def run(*ids: str, opts: 'Values') -> None: if multi_mode and not opts.skip_interactive: prompt(workflows) # prompt for approval or exit - failed = [] + failed = {} for workflow in sorted(workflows): try: init_clean(workflow, opts) except Exception as exc: - failed.append(workflow) - LOG.warning(exc) + failed[workflow] = exc if failed: - raise CylcError(f"Clean failed: {', '.join(failed)}") + msg = "Clean failed:" + for workflow, exc_message in failed.items(): + msg += f"\nWorkflow: {workflow}\nError: {exc_message}" + raise CylcError(msg) @cli_function(get_option_parser) From caa90456f7671322cff19b16bbc7fbe7b3f216f5 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Tue, 28 Mar 2023 16:40:51 +0100 Subject: [PATCH 37/57] task_pool: avoid listing tasks where not necessary * Re-use the cached task list where appropriate. * Switch to using the task dictionary for "in" comparisons for faster performance. --- cylc/flow/task_pool.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cylc/flow/task_pool.py b/cylc/flow/task_pool.py index 700727ba892..b5f7adf09a4 100644 --- a/cylc/flow/task_pool.py +++ b/cylc/flow/task_pool.py @@ -128,7 +128,7 @@ def __init__( self.task_name_list = self.config.get_task_name_list() self.task_queue_mgr = IndepQueueManager( self.config.cfg['scheduling']['queues'], - self.config.get_task_name_list(), + self.task_name_list, self.config.runtime['descendants'] ) self.tasks_to_hold: Set[Tuple[str, 'PointBase']] = set() @@ -137,7 +137,7 @@ def set_stop_task(self, task_id): """Set stop after a task.""" tokens = Tokens(task_id, relative=True) name = tokens['task'] - if name in self.config.get_task_name_list(): + if name in self.config.taskdefs: task_id = TaskID.get_standardised_taskid(task_id) LOG.info("Setting stop task: " + task_id) self.stop_task_id = task_id @@ -174,7 +174,7 @@ def load_from_point(self): flow_num = self.flow_mgr.get_new_flow( f"original flow from {self.config.start_point}") self.compute_runahead() - for name in self.config.get_task_name_list(): + for name in self.task_name_list: tdef = self.config.get_taskdef(name) point = tdef.first_point(self.config.start_point) self.spawn_to_rh_limit(tdef, point, {flow_num}) @@ -925,7 +925,7 @@ def reload_taskdefs(self) -> None: del self.task_queue_mgr self.task_queue_mgr = IndepQueueManager( self.config.cfg['scheduling']['queues'], - self.config.get_task_name_list(), + self.task_name_list, self.config.runtime['descendants'] ) @@ -1385,7 +1385,7 @@ def spawn_on_all_outputs( def can_spawn(self, name: str, point: 'PointBase') -> bool: """Return True if the task with the given name & point is within various workflow limits.""" - if name not in self.config.get_task_name_list(): + if name not in self.config.taskdefs: LOG.debug('No task definition %s', name) return False # Don't spawn outside of graph limits. From 3b28f21cd4e95764708d181f7a2353789518ef6a Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 30 Mar 2023 17:08:09 +0100 Subject: [PATCH 38/57] cat-log: reject non-normalised paths --- cylc/flow/scripts/cat_log.py | 46 +++++++++++++++++++++++ tests/functional/cylc-cat-log/00-local.t | 7 +++- tests/functional/cylc-cat-log/01-remote.t | 9 ++++- 3 files changed, 60 insertions(+), 2 deletions(-) diff --git a/cylc/flow/scripts/cat_log.py b/cylc/flow/scripts/cat_log.py index 24fdc6fbb38..eb9747dc89d 100755 --- a/cylc/flow/scripts/cat_log.py +++ b/cylc/flow/scripts/cat_log.py @@ -156,6 +156,48 @@ def colorise_cat_log(cat_proc, color=False, stdout=None): cat_proc.wait() +def _check_fs_path(path): + """Ensure a path is relative and normalised. + + Useful for checking input paths which are intended to be + relative to a specified directory. + + Examples: + # good paths + >>> _check_fs_path('a') + >>> _check_fs_path('a/b') + >>> _check_fs_path('a/b/') + + # bad paths + >>> _check_fs_path('/a') + Traceback (most recent call last): + ... + InputError: ... + >>> _check_fs_path('a/../b') + Traceback (most recent call last): + ... + InputError: ... + >>> _check_fs_path('../a') + Traceback (most recent call last): + ... + InputError: ... + >>> _check_fs_path('./a') + Traceback (most recent call last): + ... + InputError: ... + + Raises: + InputError + + """ + # join the path onto something to test normalisation + _path = os.path.join('x', path).rstrip(os.sep) + if os.path.isabs(path) or os.path.normpath(_path) != _path: + raise InputError( + f'File paths must be relative to the job log directory: {path}' + ) + + def view_log(logpath, mode, tailer_tmpl, batchview_cmd=None, remote=False, color=False): """View (by mode) local log file. This is only called on the file host. @@ -297,10 +339,14 @@ def main( b) remote: re-invoke cylc cat-log as a) on the remote account """ + if options.filename is not None: + _check_fs_path(options.filename) + if options.remote_args: # Invoked on job hosts for job logs only, as a wrapper to view_log(). # Tail and batchview commands from global config on workflow host). logpath, mode, tail_tmpl = options.remote_args[0:3] + _check_fs_path(logpath) logpath = expand_path(logpath) tail_tmpl = expand_path(tail_tmpl) try: diff --git a/tests/functional/cylc-cat-log/00-local.t b/tests/functional/cylc-cat-log/00-local.t index 7d4d25185ca..c427c13c1b5 100755 --- a/tests/functional/cylc-cat-log/00-local.t +++ b/tests/functional/cylc-cat-log/00-local.t @@ -18,7 +18,7 @@ # Test "cylc cat-log" on the workflow host. . "$(dirname "$0")/test_header" #------------------------------------------------------------------------------- -set_test_number 29 +set_test_number 31 install_workflow "${TEST_NAME_BASE}" "${TEST_NAME_BASE}" #------------------------------------------------------------------------------- TEST_NAME="${TEST_NAME_BASE}-validate" @@ -107,5 +107,10 @@ TEST_NAME=${TEST_NAME_BASE}-task-job-path run_ok "${TEST_NAME}" cylc cat-log -f j -m p "${WORKFLOW_NAME}//1/a-task" grep_ok "${WORKFLOW_NAME}/log/job/1/a-task/NN/job$" "${TEST_NAME}.stdout" #------------------------------------------------------------------------------- +# it shouldn't let you modify the file path to access other resources +# use the dedicated options +TEST_NAME=${TEST_NAME_BASE}-un-norm-path +run_fail "${TEST_NAME}" cylc cat-log -f j/../02/j "${WORKFLOW_NAME}//1/a-task" +grep_ok 'InputError' "${TEST_NAME}.stderr" purge exit diff --git a/tests/functional/cylc-cat-log/01-remote.t b/tests/functional/cylc-cat-log/01-remote.t index 9ff6429c9d6..4d1d4a0a3ac 100755 --- a/tests/functional/cylc-cat-log/01-remote.t +++ b/tests/functional/cylc-cat-log/01-remote.t @@ -19,7 +19,7 @@ export REQUIRE_PLATFORM='loc:remote' . "$(dirname "$0")/test_header" #------------------------------------------------------------------------------- -set_test_number 14 +set_test_number 16 create_test_global_config "" " [platforms] [[${CYLC_TEST_PLATFORM}]] @@ -111,6 +111,13 @@ TEST_NAME=${TEST_NAME_BASE}-task-job-path cylc cat-log -m p -f j "${WORKFLOW_NAME}//1/a-task" >"${TEST_NAME}.out" grep_ok "${WORKFLOW_NAME}/log/job/1/a-task/NN/job$" "${TEST_NAME}.out" #------------------------------------------------------------------------------- +TEST_NAME=${TEST_NAME_BASE}-un-norm-path +run_fail "${TEST_NAME}" cylc cat-log "${WORKFLOW_NAME}//1/a-task" \ + --remote-arg=j/../02/j \ + --remote-arg=cat \ + --remote-arg='tail -f' +grep_ok 'InputError' "${TEST_NAME}.stderr" +#------------------------------------------------------------------------------- # Clean up the task host. purge exit From c3cda0aa95f1d30410bf2dbff1d96a255bf57df7 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Mon, 3 Apr 2023 16:13:02 +0100 Subject: [PATCH 39/57] tests/f: add test for cat-log/clean interaction on NFS (#5432) --- tests/functional/cylc-clean/06-nfs.t | 82 ++++++++++++++++++++++++++++ 1 file changed, 82 insertions(+) create mode 100644 tests/functional/cylc-clean/06-nfs.t diff --git a/tests/functional/cylc-clean/06-nfs.t b/tests/functional/cylc-clean/06-nfs.t new file mode 100644 index 00000000000..991242cea25 --- /dev/null +++ b/tests/functional/cylc-clean/06-nfs.t @@ -0,0 +1,82 @@ +#!/usr/bin/env bash +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) 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 . +# ----------------------------------------------------------------------------- +# This test covers the interaction between "cylc clean" and "cylc cat-log -m t" +# on the NFS filesystem. The "cat-log" should not block the "clean". +# +# Tests: https://github.com/cylc/cylc-flow/pull/5359 +# +# If you try to delete a file that is stored on NFS, which is open for reading +# by another process (e.g. `tail -f`), NFS will remove the file, but put a +# ".nfs" file in its place. This ".nfs" file will cause "rm" operations on +# the directory containing the NFS files to fail with one of two errors: +# * https://docs.python.org/3/library/errno.html#errno.EBUSY +# * https://docs.python.org/3/library/errno.html#errno.ENOTEMPTY +# +# To prevent "cylc cat-log -m t" which calls "tail -f" from blocking +# "cylc clean" commands, we retry the "rm" operation with a delay. This +# allows the "tail -f" to fail and release its file lock allowing the +# "rm" to pass on a subsequent attempt. + +. "$(dirname "$0")/test_header" +if [[ $OSTYPE == darwin* ]]; then + skip_all "don't run test on Mac OS (BSD uses different error messages)" +fi +set_test_number 4 + +# install a blank source workflow +init_workflow "${TEST_NAME_BASE}" <<< '# blank workflow' + +# add a scheduler log file with something written to it +WORKFLOW_LOG_DIR="${WORKFLOW_RUN_DIR}/log/scheduler" +mkdir -p "$WORKFLOW_LOG_DIR" +LOG_STUFF='foo bar baz' +echo "${LOG_STUFF}" > "${WORKFLOW_LOG_DIR}/log" + +# start cat-log running - this runs "tail -f" +cylc cat-log -m t "$WORKFLOW_NAME" > out 2>err & PID="$!" + +# wait for cat-log to reach the end of the file +for _retry in $(seq 1 5); do + echo "# try $_retry" + if [[ "$(cat out)" != "$LOG_STUFF" ]]; then + sleep 1 + fi +done +cmp_ok out <<< "$LOG_STUFF" + +# try to clean the workflow +run_ok "${TEST_NAME_BASE}-clean" cylc clean -y "${WORKFLOW_NAME}" + +# the tail command should have detected that the file isn't there any more +# and released the file handle +grep_ok 'has become inaccessible' err + +# ensure the log dir was removed correctly +# run_ok "${TEST_NAME_BASE}-dir-removed" [[ ! -d "${WORKFLOW_LOG_DIR}" ]] +TEST_NAME="${TEST_NAME_BASE}-log-dir-removed" +if [[ -d "${WORKFLOW_LOG_DIR}" ]]; then + fail "${TEST_NAME}" +else + ok "${TEST_NAME}" +fi + +# kill the cat-log process group (will include the tail process) +pkill -P "${PID}" 2>/dev/null || true + +purge +exit From fc7324b1005798b36f2b487a0fd29b5c0cabcb02 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Tue, 4 Apr 2023 11:34:04 +0100 Subject: [PATCH 40/57] job runner handlers: docs++ --- .../flow/job_runner_handlers/documentation.py | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/cylc/flow/job_runner_handlers/documentation.py b/cylc/flow/job_runner_handlers/documentation.py index cc32b0d7dd9..735b1801b16 100644 --- a/cylc/flow/job_runner_handlers/documentation.py +++ b/cylc/flow/job_runner_handlers/documentation.py @@ -214,6 +214,20 @@ class QSUBHandler(PBSHandler): """ + POLL_CMD: str + """Command for checking job submissions. + + A list of job IDs to poll will be provided as arguments. + + The command should write valid submitted/running job IDs to stdout. + + * To filter out invalid/failed jobs use + :py:meth:`ExampleHandler.filter_poll_many_output`. + * To build a more advanced command than is possible with this configuration + use :py:meth:`ExampleHandler.get_poll_many_cmd`. + + """ + POLL_CANT_CONNECT_ERR: str """String for detecting communication errors in poll command output. @@ -283,7 +297,7 @@ class QSUBHandler(PBSHandler): def filter_poll_many_output(self, out: str) -> List[str]: """Filter job ides out of poll output. - Called after the job runner's poll many command. The method should read + Called after the job runner's poll command. The method should read the output and return a list of job IDs that are still in the job runner. @@ -333,6 +347,9 @@ def format_directives(self, job_conf: dict) -> List[str]: def get_poll_many_cmd(self, job_id_list: List[str]) -> List[str]: """Return a command to poll the specified jobs. + If specified, this will be called instead of + :py:attr:`ExampleHandler.POLL_CMD`. + Args: job_id_list: The list of job IDs to poll. From fae99ad3e8d4a3addb5f21057db1364b94bb8e7d Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 1 Dec 2022 13:41:37 +0000 Subject: [PATCH 41/57] actions: add build test --- .github/workflows/build.yml | 42 +++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 .github/workflows/build.yml diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml new file mode 100644 index 00000000000..ca4571c8097 --- /dev/null +++ b/.github/workflows/build.yml @@ -0,0 +1,42 @@ +name: Build + +# build the project whenever the configuration is changed + +on: + workflow_dispatch: + pull_request: + paths: + - 'README.md' # check markdown is valid + - 'MANIFEST.in' # check packaging + - 'pyproject.toml' # check build config + - 'setup.cfg' # check deps and project config + +jobs: + test: + runs-on: ${{ matrix.os }} + timeout-minutes: 10 + strategy: + matrix: + os: ['ubuntu-latest'] + python: ['3.7', '3.8', '3.9', '3.10'] + include: + - os: 'macos-latest' + python: '3.7' + steps: + - name: Checkout + uses: actions/checkout@v2 + + - name: Setup Python + uses: actions/setup-python@v2 + with: + python-version: ${{ matrix.python }} + + - name: Build + uses: cylc/release-actions/build-python-package@v1 + + - name: Inspect + run: | + unzip -l dist/*.whl | tee files + grep 'cylc/flow/py.typed' files + grep 'cylc/flow/etc' files + grep 'cylc/flow/etc/cylc-completion.bash' files From ccdd6cf47241339121243e1c148a5ee3dfe38f07 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Thu, 13 Apr 2023 09:25:47 +0100 Subject: [PATCH 42/57] Improve readability of host/platform eval code (#53) * WIP improve readability of host/platform eval code * Address review --- cylc/flow/hostuserutil.py | 4 +- cylc/flow/task_job_mgr.py | 12 ++--- cylc/flow/task_remote_mgr.py | 92 +++++++++++++++++++++--------------- 3 files changed, 59 insertions(+), 49 deletions(-) diff --git a/cylc/flow/hostuserutil.py b/cylc/flow/hostuserutil.py index 36154965246..34e033b2d7b 100644 --- a/cylc/flow/hostuserutil.py +++ b/cylc/flow/hostuserutil.py @@ -197,8 +197,8 @@ def is_remote_host(self, name): """ if name not in self._remote_hosts: - if not name or name.split(".")[0].startswith("localhost"): - # e.g. localhost42.localdomain42 + if not name or name.startswith("localhost"): + # e.g. localhost4.localdomain4 self._remote_hosts[name] = False else: try: diff --git a/cylc/flow/task_job_mgr.py b/cylc/flow/task_job_mgr.py index 8e440340dc9..a5e63f6bdfc 100644 --- a/cylc/flow/task_job_mgr.py +++ b/cylc/flow/task_job_mgr.py @@ -58,8 +58,6 @@ ) from cylc.flow.pathutil import get_remote_workflow_run_job_dir from cylc.flow.platforms import ( - HOST_REC_COMMAND, - PLATFORM_REC_COMMAND, get_host_from_platform, get_install_target_from_platform, get_localhost_install_target, @@ -1120,14 +1118,12 @@ def _prep_submit_task_job( host_n, platform_name = None, None try: if rtconfig['remote']['host'] is not None: - host_n = self.task_remote_mgr.subshell_eval( - rtconfig['remote']['host'], HOST_REC_COMMAND + host_n = self.task_remote_mgr.eval_host( + rtconfig['remote']['host'] ) else: - platform_name = self.task_remote_mgr.subshell_eval( - rtconfig['platform'], - PLATFORM_REC_COMMAND, - host_check=False + platform_name = self.task_remote_mgr.eval_platform( + rtconfig['platform'] ) except PlatformError as exc: itask.waiting_on_job_prep = False diff --git a/cylc/flow/task_remote_mgr.py b/cylc/flow/task_remote_mgr.py index 59b6e04cf42..c38c2ce1e4c 100644 --- a/cylc/flow/task_remote_mgr.py +++ b/cylc/flow/task_remote_mgr.py @@ -32,12 +32,14 @@ from subprocess import Popen, PIPE, DEVNULL import tarfile from time import sleep, time -from typing import Any, Deque, Dict, TYPE_CHECKING, List, NamedTuple, Tuple +from typing import ( + Any, Deque, Dict, TYPE_CHECKING, List, + NamedTuple, Optional, Tuple +) from cylc.flow import LOG from cylc.flow.exceptions import PlatformError import cylc.flow.flags -from cylc.flow.hostuserutil import is_remote_host from cylc.flow.network.client_factory import CommsMeth from cylc.flow.pathutil import ( get_dirs_to_symlink, @@ -46,6 +48,8 @@ get_workflow_run_dir, ) from cylc.flow.platforms import ( + HOST_REC_COMMAND, + PLATFORM_REC_COMMAND, NoHostsError, PlatformLookupError, get_host_from_platform, @@ -67,6 +71,7 @@ ) from cylc.flow.loggingutil import get_next_log_number, get_sorted_logs_by_time +from cylc.flow.hostuserutil import is_remote_host if TYPE_CHECKING: from zmq.auth.thread import ThreadAuthenticator @@ -106,39 +111,31 @@ def __init__(self, workflow, proc_pool, bad_hosts): self.is_reload = False self.is_restart = False - def subshell_eval(self, command, command_pattern, host_check=True): - """Evaluate a task platform from a subshell string. - - At Cylc 7, from a host string. + def _subshell_eval( + self, eval_str: str, command_pattern: re.Pattern + ) -> Optional[str]: + """Evaluate a platform or host from a possible subshell string. Arguments: - command (str): - An explicit host name, a command in back-tick or $(command) - format, or an environment variable holding a hostname. - command_pattern (re.Pattern): + eval_str: + An explicit host/platform name, a command, or an environment + variable holding a host/patform name. + command_pattern: A compiled regex pattern designed to match subshell strings. - host_check (bool): - A flag to enable remote testing. If True, and if the command - is running locally, then it will return 'localhost'. - Return (str): + Return: - None if evaluation of command is still taking place. - - If command is not defined or the evaluated name is equivalent - to 'localhost', _and_ host_check is set to True then - 'localhost' - - Otherwise, return the evaluated host name on success. + - 'localhost' if string is empty/not defined. + - Otherwise, return the evaluated host/platform name on success. Raise PlatformError on error. """ - # BACK COMPAT: references to "host" - # remove at: - # Cylc8.x - if not command: + if not eval_str: return 'localhost' # Host selection command: $(command) or `command` - match = command_pattern.match(command) + match = command_pattern.match(eval_str) if match: cmd_str = match.groups()[1] if cmd_str in self.remote_command_map: @@ -146,34 +143,51 @@ def subshell_eval(self, command, command_pattern, host_check=True): value = self.remote_command_map[cmd_str] if isinstance(value, PlatformError): raise value # command failed - elif value is None: - return # command not yet ready - else: - command = value # command succeeded + if value is None: + return None # command not yet ready + eval_str = value # command succeeded else: # Command not launched (or already reset) self.proc_pool.put_command( SubProcContext( 'remote-host-select', ['bash', '-c', cmd_str], - env=dict(os.environ)), + env=dict(os.environ) + ), callback=self._subshell_eval_callback, callback_args=[cmd_str] ) self.remote_command_map[cmd_str] = None - return self.remote_command_map[cmd_str] + return None # Environment variable substitution - command = os.path.expandvars(command) - # Remote? - # TODO - Remove at Cylc 8.x as this only makes sense with host logic - if host_check is True: - if is_remote_host(command): - return command - else: - return 'localhost' - else: - return command + return os.path.expandvars(eval_str) + + # BACK COMPAT: references to "host" + # remove at: + # Cylc8.x + def eval_host(self, host_str: str) -> Optional[str]: + """Evaluate a host from a possible subshell string. + + Args: + host_str: An explicit host name, a command in back-tick or + $(command) format, or an environment variable holding + a hostname. + + Returns 'localhost' if evaluated name is equivalent + (e.g. localhost4.localdomain4). + """ + host = self._subshell_eval(host_str, HOST_REC_COMMAND) + return host if is_remote_host(host) else 'localhost' + + def eval_platform(self, platform_str: str) -> Optional[str]: + """Evaluate a platform from a possible subshell string. + + Args: + platform_str: An explicit platform name, a command in $(command) + format, or an environment variable holding a platform name. + """ + return self._subshell_eval(platform_str, PLATFORM_REC_COMMAND) def subshell_eval_reset(self): """Reset remote eval subshell results. From f17a55f1d3ffe5c3635ed20b20b4377b95fb917f Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 13 Apr 2023 11:44:23 +0100 Subject: [PATCH 43/57] Fix flake8-comprehensions C419 Don't use any([i for i in iterable]) use any(i for i in iterable). It's more efficient because we don't have to expand the entire thing. --- cylc/flow/option_parsers.py | 2 +- cylc/flow/platforms.py | 4 ++-- cylc/flow/scripts/lint.py | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/cylc/flow/option_parsers.py b/cylc/flow/option_parsers.py index 51fdfa2a8d6..d8037e0dddc 100644 --- a/cylc/flow/option_parsers.py +++ b/cylc/flow/option_parsers.py @@ -104,7 +104,7 @@ def __sub__(self, other): def _in_list(self, others): """CLI arguments for this option found in any of a list of other options.""" - return any([self & other for other in others]) + return any(self & other for other in others) def _update_sources(self, other): """Update the sources from this and 1 other OptionSettings object""" diff --git a/cylc/flow/platforms.py b/cylc/flow/platforms.py index 6f1b4b1be03..04cd427a74b 100644 --- a/cylc/flow/platforms.py +++ b/cylc/flow/platforms.py @@ -487,10 +487,10 @@ def generic_items_match( # Get a set of items actually set in both platform and task_section. shared_items = set(task_section).intersection(set(platform_spec)) # If any set items do not match, we can't use this platform. - if not all([ + if not all( platform_spec[item] == task_section[item] for item in shared_items - ]): + ): return False return True diff --git a/cylc/flow/scripts/lint.py b/cylc/flow/scripts/lint.py index fdcd8a67273..12c58525340 100755 --- a/cylc/flow/scripts/lint.py +++ b/cylc/flow/scripts/lint.py @@ -273,7 +273,7 @@ def get_pyproject_toml(dir_): raise CylcError(f'pyproject.toml did not load: {exc}') if any( - [i in loadeddata for i in ['cylc-lint', 'cylclint', 'cylc_lint']] + i in loadeddata for i in ['cylc-lint', 'cylclint', 'cylc_lint'] ): for key in keys: tomldata[key] = loadeddata.get('cylc-lint').get(key, []) From 52438846f0bb2cdb562ae9f563937527744338a2 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Thu, 13 Apr 2023 20:49:17 +0100 Subject: [PATCH 44/57] Update tests/functional/platforms/10-do-not-host-check-platforms.t Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> --- tests/functional/platforms/10-do-not-host-check-platforms.t | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/functional/platforms/10-do-not-host-check-platforms.t b/tests/functional/platforms/10-do-not-host-check-platforms.t index 7ba5dc0218f..c52ef7e8383 100755 --- a/tests/functional/platforms/10-do-not-host-check-platforms.t +++ b/tests/functional/platforms/10-do-not-host-check-platforms.t @@ -15,7 +15,9 @@ # You should have received a copy of the GNU General Public License # along with this program. If not, see . #------------------------------------------------------------------------------- -# Check that we don't check whether the platform name is a valid host. +# Check that platform names are not treated as host names. E.g. a platform +# name starting with "localhost" should not be treated as localhost. +# https://github.com/cylc/cylc-flow/issues/5342 . "$(dirname "$0")/test_header" set_test_number 2 From 3aa4860d977c38ca7ea2420aa0aa316dc6fe73ce Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 14 Apr 2023 11:09:06 +0100 Subject: [PATCH 45/57] upgrade cylc message internal help with details of severity levels --- cylc/flow/scripts/message.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/cylc/flow/scripts/message.py b/cylc/flow/scripts/message.py index 9e9e1f258ef..c10a5627ffa 100755 --- a/cylc/flow/scripts/message.py +++ b/cylc/flow/scripts/message.py @@ -59,6 +59,20 @@ The default message severity is INFO. The --severity=SEVERITY option can be used to set the default severity level for all unprefixed messages. +Increased severity will make messages more visible in workflow logs, using +colour and format changes. DEBUG messages will not be shown in logs by default. + +The severity levels are those of the Python Logging Library +https://docs.python.org/3/library/logging.html#logging-levels: + +- CRITICAL +- ERROR +- WARNING +- INFO +- DEBUG +- NOTSET + + Note: To abort a job script with a custom error message, use cylc__job_abort: cylc__job_abort 'message...' From c9a216eb44b8012eb070d0fd68b2b8964067c4d1 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Thu, 13 Apr 2023 10:02:57 +0100 Subject: [PATCH 46/57] Upload coverage to Codecov in separate job (#5459) GH Actions: upload coverage to Codecov in separate job - Reduces how hard we hit Codecov API - Allows easy re-attempt to upload - remove unnecessary step on MacOS (cherry picked from commit 1d4da4ce2438fec968a788a74d79983d77b1e188) --- .github/workflows/test_fast.yml | 48 +++++++++++++++------------ .github/workflows/test_functional.yml | 27 ++++++++++++--- 2 files changed, 50 insertions(+), 25 deletions(-) diff --git a/.github/workflows/test_fast.yml b/.github/workflows/test_fast.yml index 1bbf67b699d..78e1add6d3a 100644 --- a/.github/workflows/test_fast.yml +++ b/.github/workflows/test_fast.yml @@ -35,20 +35,6 @@ jobs: with: python-version: ${{ matrix.python-version }} - - name: Brew Install - if: startsWith(matrix.os, 'macos') - run: | - # speed up install (https://docs.brew.sh/Manpage#environment) - export HOMEBREW_NO_AUTO_UPDATE=1 HOMEBREW_NO_INSTALL_CLEANUP=1 - echo "[command]brew update" - brew update - echo "[command]brew install ..." - brew install bash coreutils - # add GNU coreutils and sed to the user PATH - # (see instructions in brew install output) - echo "$(brew --prefix)/opt/coreutils/libexec/gnubin" \ - >> "${GITHUB_PATH}" - - name: Apt-Get Install if: startsWith(matrix.os, 'ubuntu') run: | @@ -93,7 +79,7 @@ jobs: run: | pytest tests/integration - - name: Upload artifact + - name: Upload failed tests artifact if: failure() uses: actions/upload-artifact@v3 with: @@ -105,15 +91,35 @@ jobs: coverage xml coverage report + - name: Upload coverage artifact + uses: actions/upload-artifact@v3 + with: + name: coverage_${{ matrix.os }}_py-${{ matrix.python-version }} + path: coverage.xml + retention-days: 7 + + - name: Linkcheck + if: startsWith(matrix.python-version, '3.10') + run: pytest -m linkcheck --dist=load tests/unit + + codecov: + needs: test + runs-on: ubuntu-latest + timeout-minutes: 2 + steps: + - name: Checkout + uses: actions/checkout@v3 + + - name: Download coverage artifacts + uses: actions/download-artifact@v3 + - name: Codecov upload uses: codecov/codecov-action@v3 with: - name: '${{ github.workflow }} ${{ matrix.os }} py-${{ matrix.python-version }}' + name: ${{ github.workflow }} flags: fast-tests fail_ci_if_error: true verbose: true - token: ${{ secrets.CODECOV_TOKEN }} # Token not required for public repos, but might reduce chance of random 404 error? - - - name: Linkcheck - if: startsWith(matrix.python-version, '3.10') - run: pytest -m linkcheck --dist=load tests/unit + # Token not required for public repos, but avoids upload failure due + # to rate-limiting (but not for PRs opened from forks) + token: ${{ secrets.CODECOV_TOKEN }} diff --git a/.github/workflows/test_functional.yml b/.github/workflows/test_functional.yml index 21967c85f76..3514713b7e9 100644 --- a/.github/workflows/test_functional.yml +++ b/.github/workflows/test_functional.yml @@ -248,14 +248,13 @@ jobs: -exec echo '====== {} ======' \; -exec cat '{}' \; - name: Set artifact upload name - if: failure() && steps.test.outcome == 'failure' id: uploadname run: | # artifact name cannot contain '/' characters CID="$(sed 's|/|-|g' <<< "${{ matrix.name || matrix.chunk }}")" echo "uploadname=$CID" >> $GITHUB_OUTPUT - - name: Upload artifact + - name: Upload failed tests artifact if: failure() && steps.test.outcome == 'failure' uses: actions/upload-artifact@v3 with: @@ -294,11 +293,31 @@ jobs: coverage xml coverage report + - name: Upload coverage artifact + uses: actions/upload-artifact@v3 + with: + name: coverage_${{ steps.uploadname.outputs.uploadname }} + path: coverage.xml + retention-days: 7 + + codecov: + needs: test + runs-on: ubuntu-latest + timeout-minutes: 2 + steps: + - name: Checkout + uses: actions/checkout@v3 + + - name: Download coverage artifacts + uses: actions/download-artifact@v3 + - name: Codecov upload uses: codecov/codecov-action@v3 with: - name: '${{ github.workflow }} ${{ matrix.name }} ${{ matrix.chunk }}' + name: ${{ github.workflow }} flags: functional-tests fail_ci_if_error: true verbose: true - token: ${{ secrets.CODECOV_TOKEN }} # Token not required for public repos, but might reduce chance of random 404 error? + # Token not required for public repos, but avoids upload failure due + # to rate-limiting (but not for PRs opened from forks) + token: ${{ secrets.CODECOV_TOKEN }} From 574e43c5d976bcf734ef8dc88b830e8a2663faa6 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 14 Apr 2023 16:49:12 +0100 Subject: [PATCH 47/57] Update cylc/flow/scripts/message.py Co-authored-by: Oliver Sanders --- cylc/flow/scripts/message.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/cylc/flow/scripts/message.py b/cylc/flow/scripts/message.py index c10a5627ffa..b2fb01f431c 100755 --- a/cylc/flow/scripts/message.py +++ b/cylc/flow/scripts/message.py @@ -70,8 +70,6 @@ - WARNING - INFO - DEBUG -- NOTSET - Note: To abort a job script with a custom error message, use cylc__job_abort: From fddd828ce1efb5f629ddbabc238d5e5d2c2352cb Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Wed, 19 Apr 2023 10:34:06 +0100 Subject: [PATCH 48/57] Fix workflow shutdown on no platforms error (#5395) platforms: catch & handle platform errors * fix four instances of get_platform not being protected by try/except loops * separate log messages for different errors * make sure that failure to resolve platform on restart causes restart to fail * fail on restart updated to warn of all undefined platforms * Fix flake8-comprehensions C419 --- CHANGES.md | 3 + cylc/flow/exceptions.py | 2 +- cylc/flow/platforms.py | 40 ++++++++--- cylc/flow/rundb.py | 24 ++++++- cylc/flow/subprocpool.py | 10 ++- cylc/flow/task_events_mgr.py | 16 ++++- cylc/flow/task_job_mgr.py | 17 ++++- cylc/flow/task_pool.py | 14 +++- .../platforms/10-platform-gone-on-restart.t | 67 +++++++++++++++++++ tests/integration/test_task_events_mgr.py | 44 ++++++++++++ tests/integration/test_task_job_mgr.py | 24 +++++++ tests/integration/test_task_pool.py | 16 +++++ tests/unit/test_rundb.py | 22 ++++++ tests/unit/test_subprocpool.py | 13 ++++ 14 files changed, 292 insertions(+), 20 deletions(-) create mode 100644 tests/functional/platforms/10-platform-gone-on-restart.t create mode 100644 tests/integration/test_task_events_mgr.py diff --git a/CHANGES.md b/CHANGES.md index ebb19ab6eaa..014dc887841 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -17,6 +17,9 @@ ones in. --> [5398](https://github.com/cylc/cylc-flow/pull/5398) - Fix platform from group selection order bug. +[#5395](https://github.com/cylc/cylc-flow/pull/5395) - Fix bug where workflow +shuts down if all hosts for all platforms in a platform group are unreachable. + [#5384](https://github.com/cylc/cylc-flow/pull/5384) - Fixes `cylc set-verbosity`. diff --git a/cylc/flow/exceptions.py b/cylc/flow/exceptions.py index 94d90f25066..073c8261b29 100644 --- a/cylc/flow/exceptions.py +++ b/cylc/flow/exceptions.py @@ -435,7 +435,7 @@ def __str__(self): return f'Unable to find valid host for {self.platform_name}' -class NoPlatformsError(CylcError): +class NoPlatformsError(PlatformLookupError): """None of the platforms of a given group were reachable.""" def __init__(self, platform_group): self.platform_group = platform_group diff --git a/cylc/flow/platforms.py b/cylc/flow/platforms.py index 04cd427a74b..86806bd26b7 100644 --- a/cylc/flow/platforms.py +++ b/cylc/flow/platforms.py @@ -117,11 +117,22 @@ def get_platform( task_conf: If str this is assumed to be the platform name, otherwise this should be a configuration for a task. task_name: Help produce more helpful error messages. + bad_hosts: A set of hosts known to be unreachable (had an ssh 255 + error) Returns: platform: A platform definition dictionary. Uses either get_platform() or platform_name_from_job_info(), but to the user these look the same. + + Raises: + NoPlatformsError: + Platform group has no platforms with usable hosts. + This should be caught if this function is used on a raw config, + or in any other context where a platform group might be selected. + PlatformLookupError: + Raised if the name of a platform cannot be selected based on the + information given. """ if task_conf is None or isinstance(task_conf, str): # noqa: SIM 114 # task_conf is a platform name, or get localhost if None @@ -178,10 +189,15 @@ def platform_from_name( Args: platform_name: name of platform to be retrieved. platforms: global.cylc platforms given as a dict. + bad_hosts: A set of hosts known to be unreachable (had an ssh 255 + error) Returns: platform: object containing settings for a platform, loaded from Global Config. + + Raises: + NoPlatformsError: Platform group has no platforms with usable hosts. """ if platforms is None: platforms = glbl_cfg().get(['platforms']) @@ -190,9 +206,8 @@ def platform_from_name( if platform_name is None: platform_name = 'localhost' - platform_group = None - # The list is reversed to allow user-set platform groups (which are loaded - # later than site set platform groups) to be matched first and override + # The list is reversed to allow user-set platform groups (which are + # appended to site set platform groups) to be matched first and override # site defined platform groups. for platform_name_re in reversed(list(platform_groups)): # Platform is member of a group. @@ -214,9 +229,10 @@ def platform_from_name( 'regular expression. See the documentation for ' '"global.cylc[platforms][localhost]" for more information.' ) - # The list is reversed to allow user-set platforms (which are loaded - # later than site set platforms) to be matched first and override site - # defined platforms. + + # The list is reversed to allow user-set platforms (which are appended to + # than site set platforms) to be matched first and override site defined + # platforms. for platform_name_re in reversed(list(platforms)): # We substitute commas with or without spaces to # allow lists of platforms @@ -246,8 +262,6 @@ def platform_from_name( platform_data['hosts'] = [platform_name] # Fill in the "private" name field. platform_data['name'] = platform_name - if platform_group: - platform_data['group'] = platform_group return platform_data raise PlatformLookupError( @@ -507,6 +521,10 @@ def get_host_from_platform( Returns: hostname: The name of a host. + + Raises: + NoHostsError: + This error should be caught by caller to prevent workflow shutdown. """ # Get list of goodhosts: if bad_hosts: @@ -666,7 +684,11 @@ def get_all_platforms_for_install_target( def get_random_platform_for_install_target( install_target: str ) -> Dict[str, Any]: - """Return a randomly selected platform (dict) for given install target.""" + """Return a randomly selected platform (dict) for given install target. + + Raises: + PlatformLookupError: We can't get a platform for this install target. + """ platforms = get_all_platforms_for_install_target(install_target) try: return random.choice(platforms) # nosec (not crypto related) diff --git a/cylc/flow/rundb.py b/cylc/flow/rundb.py index e0a18b61c8d..2637fadf473 100644 --- a/cylc/flow/rundb.py +++ b/cylc/flow/rundb.py @@ -24,6 +24,7 @@ from typing import TYPE_CHECKING, List, Optional, Tuple, Union from cylc.flow import LOG +from cylc.flow.exceptions import PlatformLookupError from cylc.flow.util import deserialise import cylc.flow.flags @@ -837,6 +838,11 @@ def select_task_pool_for_restart(self, callback): Invoke callback(row_idx, row) on each row, where each row contains: the fields in the SELECT statement below. + + Raises: + PlatformLookupError: Do not start up if platforms for running + tasks cannot be found in global.cylc. This exception should + not be caught. """ form_stmt = r""" SELECT @@ -890,8 +896,24 @@ def select_task_pool_for_restart(self, callback): "task_outputs": self.TABLE_TASK_OUTPUTS, } stmt = form_stmt % form_data + + # Run the callback, collecting any platform errors to be handled later: + platform_errors = [] for row_idx, row in enumerate(self.connect().execute(stmt)): - callback(row_idx, list(row)) + platform_error = callback(row_idx, list(row)) + if platform_error: + platform_errors.append(platform_error) + + # If any of the platforms could not be found, raise an exception + # and stop trying to play this workflow: + if platform_errors: + msg = ( + "The following platforms are not defined in" + " the global.cylc file:" + ) + for platform in platform_errors: + msg += f"\n * {platform}" + raise PlatformLookupError(msg) def select_task_prerequisites( self, cycle: str, name: str, flow_nums: str diff --git a/cylc/flow/subprocpool.py b/cylc/flow/subprocpool.py index e4e64755fc4..076c08ed755 100644 --- a/cylc/flow/subprocpool.py +++ b/cylc/flow/subprocpool.py @@ -31,6 +31,7 @@ from cylc.flow import LOG from cylc.flow.cfgspec.glbl_cfg import glbl_cfg from cylc.flow.cylc_subproc import procopen +from cylc.flow.exceptions import PlatformLookupError from cylc.flow.hostuserutil import is_remote_host from cylc.flow.platforms import ( log_platform_event, @@ -487,7 +488,14 @@ def _run_callback(callback, args_=None): # that you can use that platform's ssh command. platform = None if isinstance(ctx.cmd_key, TaskJobLogsRetrieveContext): - platform = get_platform(ctx.cmd_key.platform_name) + try: + platform = get_platform(ctx.cmd_key.platform_name) + except PlatformLookupError: + log_platform_event( + 'Unable to retrieve job logs.', + {'name': ctx.cmd_key.platform_name}, + level='warning', + ) elif callback_args: platform = callback_args[0] if not ( diff --git a/cylc/flow/task_events_mgr.py b/cylc/flow/task_events_mgr.py index ea2c7c1be7d..0145237e74e 100644 --- a/cylc/flow/task_events_mgr.py +++ b/cylc/flow/task_events_mgr.py @@ -37,7 +37,7 @@ from cylc.flow import LOG, LOG_LEVELS from cylc.flow.cfgspec.glbl_cfg import glbl_cfg -from cylc.flow.exceptions import NoHostsError +from cylc.flow.exceptions import NoHostsError, PlatformLookupError from cylc.flow.hostuserutil import get_host, get_user, is_remote_platform from cylc.flow.parsec.config import ItemNotFoundError from cylc.flow.pathutil import ( @@ -48,7 +48,10 @@ TaskActionTimer, TimerFlags ) -from cylc.flow.platforms import get_platform, get_host_from_platform +from cylc.flow.platforms import ( + get_platform, get_host_from_platform, + log_platform_event +) from cylc.flow.task_job_logs import ( get_task_job_log, get_task_job_activity_log, @@ -941,8 +944,8 @@ def _get_events_conf(self, itask, key, default=None): def _process_job_logs_retrieval(self, schd, ctx, id_keys): """Process retrieval of task job logs from remote user@host.""" # get a host to run retrieval on - platform = get_platform(ctx.platform_name) try: + platform = get_platform(ctx.platform_name) host = get_host_from_platform(platform, bad_hosts=self.bad_hosts) except NoHostsError: # All of the platforms hosts have been found to be uncontactable. @@ -961,6 +964,13 @@ def _process_job_logs_retrieval(self, schd, ctx, id_keys): for id_key in id_keys: self.unset_waiting_event_timer(id_key) return + except PlatformLookupError: + log_platform_event( + 'Unable to retrieve job logs.', + {'name': ctx.platform_name}, + level='warning', + ) + return # construct the retrieval command ssh_str = str(platform["ssh command"]) diff --git a/cylc/flow/task_job_mgr.py b/cylc/flow/task_job_mgr.py index a5e63f6bdfc..6c940d48842 100644 --- a/cylc/flow/task_job_mgr.py +++ b/cylc/flow/task_job_mgr.py @@ -308,7 +308,7 @@ def submit_task_jobs(self, workflow, itasks, curve_auth, itask.tdef.rtconfig['platform'], bad_hosts=self.bad_hosts ) - except NoPlatformsError: + except PlatformLookupError: pass else: # If were able to select a new platform; @@ -910,7 +910,19 @@ def _run_job_cmd( # Go through each list of itasks and carry out commands as required. for platform_name, itasks in sorted(auth_itasks.items()): - platform = get_platform(platform_name) + try: + platform = get_platform(platform_name) + except NoPlatformsError: + LOG.error( + f'Unable to run command {cmd_key}: Unable to find' + f' platform {platform_name} with accessible hosts.' + ) + except PlatformLookupError: + LOG.error( + f'Unable to run command {cmd_key}: Unable to find' + f' platform {platform_name}.' + ) + continue if is_remote_platform(platform): remote_mode = True cmd = [cmd_key] @@ -1163,7 +1175,6 @@ def _prep_submit_task_job( platform = get_platform( rtconfig, itask.tdef.name, bad_hosts=self.bad_hosts ) - except PlatformLookupError as exc: itask.waiting_on_job_prep = False itask.summary['platforms_used'][itask.submit_num] = '' diff --git a/cylc/flow/task_pool.py b/cylc/flow/task_pool.py index b5f7adf09a4..5959bc34452 100644 --- a/cylc/flow/task_pool.py +++ b/cylc/flow/task_pool.py @@ -29,7 +29,8 @@ import cylc.flow.flags from cylc.flow import LOG from cylc.flow.cycling.loader import get_point, standardise_point_string -from cylc.flow.exceptions import WorkflowConfigError, PointParsingError +from cylc.flow.exceptions import ( + WorkflowConfigError, PointParsingError, PlatformLookupError) from cylc.flow.id import Tokens, detokenise from cylc.flow.id_cli import contains_fnmatch from cylc.flow.id_match import filter_ids @@ -428,6 +429,9 @@ def load_db_task_pool_for_restart(self, row_idx, row): as submitted or running are polled to confirm their true status. Tasks are added to queues again on release from runahead pool. + Returns: + Names of platform if attempting to look up that platform + has led to a PlatformNotFoundError. """ if row_idx == 0: LOG.info("LOADING task proxies") @@ -447,6 +451,7 @@ def load_db_task_pool_for_restart(self, row_idx, row): flow_wait=bool(flow_wait), is_manual_submit=bool(is_manual_submit) ) + except WorkflowConfigError: LOG.exception( f'ignoring task {name} from the workflow run database\n' @@ -461,7 +466,12 @@ def load_db_task_pool_for_restart(self, row_idx, row): TASK_STATUS_SUCCEEDED ): # update the task proxy with platform - itask.platform = get_platform(platform_name) + # If we get a failure from the platform selection function + # set task status to submit-failed. + try: + itask.platform = get_platform(platform_name) + except PlatformLookupError: + return platform_name if time_submit: itask.set_summary_time('submitted', time_submit) diff --git a/tests/functional/platforms/10-platform-gone-on-restart.t b/tests/functional/platforms/10-platform-gone-on-restart.t new file mode 100644 index 00000000000..329dbae2236 --- /dev/null +++ b/tests/functional/platforms/10-platform-gone-on-restart.t @@ -0,0 +1,67 @@ +#!/usr/bin/env bash +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) 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 . +#------------------------------------------------------------------------------- +# If a platform is deleted from global config and restart cannot find an +# approproiate match, don't keep going. +# 1. Run a workflow which stops leaving a job running on a platform. +# 2. Delete the platform from global.cylc +# 3. Attempt to restart. +# 4. Check that restart fails in the desired manner. +. "$(dirname "$0")/test_header" + +set_test_number 3 + +create_test_global_config "" " + [platforms] + [[myplatform]] + hosts = localhost + install target = localhost +" + +init_workflow "${TEST_NAME_BASE}" <<'__FLOW_CONF__' +[scheduling] + initial cycle point = 2934 + [[graph]] + R1 = foo + +[runtime] + [[foo]] + script = """ + cylc stop ${CYLC_WORKFLOW_ID} --now --now + """ + platform = myplatform +__FLOW_CONF__ + +run_ok "${TEST_NAME_BASE}-play" \ + cylc play "${WORKFLOW_NAME}" + +# Wait for workflow to stop, then wreck the global config: +poll_workflow_stopped +create_test_global_config "" " +" + +# Test that restart fails: +run_fail "${TEST_NAME_BASE}-restart" \ + cylc play "${WORKFLOW_NAME}" --no-detach +named_grep_ok \ + "${TEST_NAME_BASE}-cannot-restart" \ + "platforms are not defined in the global.cylc" \ + "${RUN_DIR}/${WORKFLOW_NAME}/log/scheduler/log" + +purge +exit 0 + diff --git a/tests/integration/test_task_events_mgr.py b/tests/integration/test_task_events_mgr.py new file mode 100644 index 00000000000..72ab6807714 --- /dev/null +++ b/tests/integration/test_task_events_mgr.py @@ -0,0 +1,44 @@ +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) 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 . + +from cylc.flow.task_events_mgr import TaskJobLogsRetrieveContext +from cylc.flow.scheduler import Scheduler + +from typing import Any as Fixture + + +async def test_process_job_logs_retrieval_warns_no_platform( + one_conf: Fixture, flow: Fixture, scheduler: Fixture, run: Fixture, + db_select: Fixture, caplog: Fixture +): + """Job log retrieval handles `NoHostsError`""" + + ctx = TaskJobLogsRetrieveContext( + ctx_type='raa', + platform_name='skarloey', + max_size=256, + key='skarloey' + ) + reg: str = flow(one_conf) + schd: 'Scheduler' = scheduler(reg, paused_start=True) + # Run + async with run(schd): + schd.task_events_mgr._process_job_logs_retrieval( + schd, ctx, 'foo' + ) + warning = caplog.records[-1] + assert warning.levelname == 'WARNING' + assert 'Unable to retrieve' in warning.msg diff --git a/tests/integration/test_task_job_mgr.py b/tests/integration/test_task_job_mgr.py index a8413caca44..d0c99abdb56 100644 --- a/tests/integration/test_task_job_mgr.py +++ b/tests/integration/test_task_job_mgr.py @@ -15,11 +15,14 @@ # along with this program. If not, see . import logging +from typing import Any as Fixture from cylc.flow import CYLC_LOG +from cylc.flow.scheduler import Scheduler from cylc.flow.task_state import TASK_STATUS_RUNNING + async def test_run_job_cmd_no_hosts_error( flow, scheduler, @@ -104,3 +107,24 @@ async def test_run_job_cmd_no_hosts_error( log, contains='No available hosts for no-host-platform', ) + + +async def test__run_job_cmd_logs_platform_lookup_fail( + one_conf: Fixture, flow: Fixture, scheduler: Fixture, run: Fixture, + db_select: Fixture, caplog: Fixture +) -> None: + """TaskJobMg._run_job_cmd handles failure to get platform.""" + reg: str = flow(one_conf) + schd: 'Scheduler' = scheduler(reg, paused_start=True) + # Run + async with run(schd): + from types import SimpleNamespace + schd.task_job_mgr._run_job_cmd( + schd.task_job_mgr.JOBS_POLL, + 'foo', + [SimpleNamespace(platform={'name': 'culdee fell summit'})], + None + ) + warning = caplog.records[-1] + assert warning.levelname == 'ERROR' + assert 'Unable to run command jobs-poll' in warning.msg diff --git a/tests/integration/test_task_pool.py b/tests/integration/test_task_pool.py index dc5dc5a16e3..bbc9fcc744e 100644 --- a/tests/integration/test_task_pool.py +++ b/tests/integration/test_task_pool.py @@ -23,6 +23,7 @@ from cylc.flow.cycling import PointBase from cylc.flow.cycling.integer import IntegerPoint +from cylc.flow.exceptions import PlatformLookupError from cylc.flow.scheduler import Scheduler from cylc.flow.flow_mgr import FLOW_ALL from cylc.flow.task_state import ( @@ -577,3 +578,18 @@ async def test_runahead_after_remove( # Should update after removing the first point. task_pool.remove_tasks(['1/*']) assert int(task_pool.runahead_limit_point) == 5 + + +async def test_load_db_bad_platform( + flow: Callable, scheduler: Callable, start: Callable, one_conf: Callable +): + """Test that loading an unavailable platform from the database doesn't + cause calamitous failure.""" + schd: Scheduler = scheduler(flow(one_conf)) + + async with start(schd): + result = schd.pool.load_db_task_pool_for_restart(0, ( + '1', 'one', '{"1": 1}', "0", False, False, "failed", + False, 1, '', 'culdee-fell-summit', '', '', '', '{}' + )) + assert result == 'culdee-fell-summit' diff --git a/tests/unit/test_rundb.py b/tests/unit/test_rundb.py index a0754659dc6..372162b1387 100644 --- a/tests/unit/test_rundb.py +++ b/tests/unit/test_rundb.py @@ -21,9 +21,11 @@ import unittest from unittest import mock from tempfile import mktemp +from types import SimpleNamespace import pytest +from cylc.flow.exceptions import PlatformLookupError from cylc.flow.rundb import CylcWorkflowDAO @@ -153,3 +155,23 @@ def test_context_manager_exit( mock_close.assert_called_once() # Close connection for real: dao.close() + + +def test_select_task_pool_for_restart_if_not_platforms(tmp_path): + """Returns error if platform error or errors raised by callback. + """ + # Setup a fake callback function which returns the fake "platform_name": + def callback(index, row): + return ''.join(row) + + db_file = tmp_path / 'db' + dao = CylcWorkflowDAO(db_file, create_tables=True) + # Fiddle the connect method to return a list of fake "platform_names": + dao.connect = lambda: SimpleNamespace(execute=lambda _: ['foo', 'bar']) + + # Assert that an error is raised and that it mentions both fake platforms: + with pytest.raises( + PlatformLookupError, + match='not defined.*\n.*foo.*\n.*bar' + ): + dao.select_task_pool_for_restart(callback) diff --git a/tests/unit/test_subprocpool.py b/tests/unit/test_subprocpool.py index dc55fb00fd6..7af9738afdf 100644 --- a/tests/unit/test_subprocpool.py +++ b/tests/unit/test_subprocpool.py @@ -273,6 +273,19 @@ def test__run_command_exit_no_255_callback(caplog, mock_ctx): assert 'callback called' in caplog.records[0].msg +def test__run_command_exit_no_gettable_platform(caplog, mock_ctx): + """It logs being unable to select a platform""" + ret_ctx = TaskJobLogsRetrieveContext( + ctx_type='raa', + platform_name='rhenas', + max_size=256, + key='rhenas' + ) + ctx = mock_ctx(cmd_key=ret_ctx, cmd=['ssh'], ret_code=255) + SubProcPool._run_command_exit(ctx, callback=_test_callback) + assert 'platform: rhenas' in caplog.records[0].msg + + def test__run_command_exit_no_255_args(caplog, mock_ctx): """It runs the 255 callback with the args of the callback if no callback 255 args provided. From 8cc4fc3351cfc607e2356eb3bc5ebf2b460467ec Mon Sep 17 00:00:00 2001 From: Mel Hall <37735232+datamel@users.noreply.github.com> Date: Tue, 14 Mar 2023 09:42:35 +0000 Subject: [PATCH 49/57] cat-log: support other workflow files * Update cat log to request specific workflow log file with -f option --- CHANGES.md | 6 ++++++ cylc/flow/scripts/cat_log.py | 12 ++++++++---- tests/functional/cylc-cat-log/00-local.t | 10 ++++++---- 3 files changed, 20 insertions(+), 8 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 014dc887841..4ca2902a059 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -10,8 +10,14 @@ creating a new release entry be sure to copy & paste the span tag with the updated. Only the first match gets replaced, so it's fine to leave the old ones in. --> ------------------------------------------------------------------------------- + ## __cylc-8.1.3 (Upcoming)__ +### Enhancements + +[#5414](https://github.com/cylc/cylc-flow/pull/5414) - +Enable cat-log to view workflow logs with -f option. + ### Fixes [5398](https://github.com/cylc/cylc-flow/pull/5398) - Fix platform from diff --git a/cylc/flow/scripts/cat_log.py b/cylc/flow/scripts/cat_log.py index eb9747dc89d..b10217c2123 100755 --- a/cylc/flow/scripts/cat_log.py +++ b/cylc/flow/scripts/cat_log.py @@ -44,6 +44,9 @@ # Print workflow log: $ cylc cat-log foo + # Print specific workflow log: + $ cylc cat-log foo -f 02-start-01.log + # Print task stdout: $ cylc cat-log foo//2020/bar $ cylc cat-log -f o foo//2020/bar @@ -76,6 +79,7 @@ get_remote_workflow_run_job_dir, get_workflow_run_job_dir, get_workflow_run_pub_db_path, + get_workflow_run_scheduler_log_dir, get_workflow_run_scheduler_log_path) from cylc.flow.remote import remote_cylc_cmd, watch_and_kill from cylc.flow.rundb import CylcWorkflowDAO @@ -368,10 +372,6 @@ def main( mode = options.mode if not tokens or not tokens.get('task'): - # Cat workflow logs, local only. - if options.filename is not None: - raise InputError("The '-f' option is for job logs only.") - logpath = get_workflow_run_scheduler_log_path(workflow_id) if options.rotation_num: log_dir = Path(logpath).parent @@ -382,6 +382,10 @@ def main( except IndexError: raise InputError( "max rotation %d" % (len(logs) - 1)) + # Cat workflow logs, local only. + if options.filename is not None and not options.rotation_num: + logpath = os.path.join(get_workflow_run_scheduler_log_dir( + workflow_id), str(options.filename)) tail_tmpl = os.path.expandvars( get_platform()["tail command template"] ) diff --git a/tests/functional/cylc-cat-log/00-local.t b/tests/functional/cylc-cat-log/00-local.t index c427c13c1b5..44134052e6f 100755 --- a/tests/functional/cylc-cat-log/00-local.t +++ b/tests/functional/cylc-cat-log/00-local.t @@ -30,10 +30,12 @@ TEST_NAME=${TEST_NAME_BASE}-workflow-log-log run_ok "${TEST_NAME}" cylc cat-log "${WORKFLOW_NAME}" contains_ok "${TEST_NAME}.stdout" "${WORKFLOW_RUN_DIR}/log/scheduler/log" #------------------------------------------------------------------------------- -TEST_NAME=${TEST_NAME_BASE}-workflow-log-fail -run_fail "${TEST_NAME}" cylc cat-log -f e "${WORKFLOW_NAME}" -contains_ok "${TEST_NAME}.stderr" - << __END__ -InputError: The '-f' option is for job logs only. +TEST_NAME=${TEST_NAME_BASE}-workflow-log-ok +LOG_DIR="$(dirname "$(cylc cat-log -m p "${WORKFLOW_NAME}")")" +echo "This is file 03-restart-02.log" > "${LOG_DIR}/03-restart-02.log" +run_ok "${TEST_NAME}" cylc cat-log -f 03-restart-02.log "${WORKFLOW_NAME}" +contains_ok "${TEST_NAME}.stdout" - << __END__ +This is file 03-restart-02.log __END__ #------------------------------------------------------------------------------- TEST_NAME=${TEST_NAME_BASE}-task-out From e01ce1b75373e3d9c2f78c46295f03f7bc05f9f5 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Thu, 30 Mar 2023 14:48:36 +0100 Subject: [PATCH 50/57] cat-log: change interface for scheduler logs to include the dirname * Support specifying the job submit number in the ID. * Support specifying the most recent workflow log file via a one letter filename as with job logs. * Add --prepend-path option which print the host:filepath before cat or tail operations. This is used by the UI to display the host:path where the log file is located. --- CHANGES.md | 5 +- cylc/flow/scripts/cat_log.py | 193 ++++++++++++++---- tests/functional/cylc-cat-log/00-local.t | 32 ++- .../cylc-cat-log/00-local/flow.cylc | 18 +- .../functional/cylc-cat-log/06-log-rotation.t | 2 +- tests/functional/cylc-clean/06-nfs.t | 2 +- tests/functional/job-submission/16-timeout.t | 3 +- 7 files changed, 194 insertions(+), 61 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 4ca2902a059..a4aa28b9954 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -15,8 +15,9 @@ ones in. --> ### Enhancements -[#5414](https://github.com/cylc/cylc-flow/pull/5414) - -Enable cat-log to view workflow logs with -f option. +[#5453](https://github.com/cylc/cylc-flow/pull/5453) - `cylc cat-log` can now +list and view workflow log files including install logs and workflow +configuration files. ### Fixes diff --git a/cylc/flow/scripts/cat_log.py b/cylc/flow/scripts/cat_log.py index b10217c2123..5b2223bacb0 100755 --- a/cylc/flow/scripts/cat_log.py +++ b/cylc/flow/scripts/cat_log.py @@ -45,7 +45,7 @@ $ cylc cat-log foo # Print specific workflow log: - $ cylc cat-log foo -f 02-start-01.log + $ cylc cat-log foo -f scheduler/02-start-01.log # Print task stdout: $ cylc cat-log foo//2020/bar @@ -53,6 +53,12 @@ # Print task stderr: $ cylc cat-log -f e foo//2020/bar + + # Print a custom file in a job's log directory: + $ cylc cat-log -f my-log-file foo//2020/bar + + # Follow a log file: + $ cylc cat-log foo//2020/bar -m f """ import os @@ -68,7 +74,6 @@ import cylc.flow.flags from cylc.flow.hostuserutil import is_remote_platform from cylc.flow.id_cli import parse_id -from cylc.flow.loggingutil import LOG_FILE_EXTENSION from cylc.flow.option_parsers import ( ID_MULTI_ARG_DOC, CylcOptionParser as COP, @@ -79,8 +84,8 @@ get_remote_workflow_run_job_dir, get_workflow_run_job_dir, get_workflow_run_pub_db_path, - get_workflow_run_scheduler_log_dir, - get_workflow_run_scheduler_log_path) + get_workflow_run_dir, +) from cylc.flow.remote import remote_cylc_cmd, watch_and_kill from cylc.flow.rundb import CylcWorkflowDAO from cylc.flow.task_job_logs import ( @@ -93,6 +98,37 @@ from optparse import Values +WORKFLOW_LOG_OPTS = { + 'c': ('workflow configuration file (raw)', r'config/*-start-*.cylc'), + 'p': ( + 'workflow configuration file (processed)', + r'config/flow-processed.cylc' + ), + 'i': ('install log', r'install/*-*install.log'), + 's': ('scheduler log', r'scheduler/*-*start*.log'), +} + + +# add workflow and job log file options to the CLI help output +__doc__ += r''' + +Log Files: + Select the log file to view with the --file option. + Either provide the file path or use one of the short options: + + Job Logs: +''' + ' ' + '\n '.join( + f'{key:4} {value}' + for key, value in JOB_LOG_OPTS.items() +) + ''' + + Workflow Logs: +''' + ' ' + '\n '.join( + f'{key:4} {value[0]}' + for key, value in WORKFLOW_LOG_OPTS.items() +) + '\n\n Use "--mode=l" to list available log files for a workflow/job.' + + # Immortal tail-follow processes on job hosts can be cleaned up by killing # my subprocesses if my PPID or PPPID changes (due to parent ssh connection # dying). This works even if the sshd-invoked @@ -202,8 +238,15 @@ def _check_fs_path(path): ) -def view_log(logpath, mode, tailer_tmpl, batchview_cmd=None, remote=False, - color=False): +def view_log( + logpath, + mode, + tailer_tmpl, + batchview_cmd=None, + remote=False, + color=False, + prepend_path=False, +): """View (by mode) local log file. This is only called on the file host. batchview_cmd is a job-runner-specific job stdout or stderr cat or tail @@ -218,19 +261,22 @@ def view_log(logpath, mode, tailer_tmpl, batchview_cmd=None, remote=False, # Print location even if the workflow does not exist yet. print(logpath) return 0 - elif not os.path.exists(logpath) and batchview_cmd is None: + if not os.path.exists(logpath) and batchview_cmd is None: # Note: batchview_cmd may not need to have access to logpath, so don't # test for existence of path if it is set. sys.stderr.write('file not found: %s\n' % logpath) return 1 - elif mode == 'print-dir': + if mode == 'print-dir': print(os.path.dirname(logpath)) return 0 - elif mode == 'list-dir': + if mode == 'list-dir': for entry in sorted(os.listdir(os.path.dirname(logpath))): print(entry) return 0 - elif mode == 'cat': + if prepend_path: + from cylc.flow.hostuserutil import get_host + print(f'# {get_host()}:{logpath}') + if mode == 'cat': # print file contents to stdout. if batchview_cmd is not None: cmd = shlex.split(batchview_cmd) @@ -244,7 +290,7 @@ def view_log(logpath, mode, tailer_tmpl, batchview_cmd=None, remote=False, # * batchview command is user configurable colorise_cat_log(proc1, color=color) return 0 - elif mode == 'tail': + if mode == 'tail': if batchview_cmd is not None: cmd = batchview_cmd else: @@ -267,11 +313,16 @@ def get_option_parser() -> COP: parser.add_option( "-f", "--file", - help=" Job log: %s; default o(out)." % ( - ', '.join(['%s(%s)' % (i, j) - for i, j in JOB_LOG_OPTS.items()])) + - " Or for custom (and standard) job logs.", - metavar="LOG", action="store", default=None, dest="filename") + help=( + 'The file to view. Default for job logs "out", default for' + ' workflow logs "scheduler/log". See "Log Files" above for' + ' possible values.' + ), + metavar="LOG", + action="store", + default=None, + dest="filename", + ) parser.add_option( "-m", "--mode", @@ -284,7 +335,7 @@ def get_option_parser() -> COP: "-r", "--rotation", help="Workflow log integer rotation number. 0 for current, 1 for " "next oldest, etc.", - metavar="INT", action="store", dest="rotation_num") + metavar="INT", action="store", dest="rotation_num", type=int) parser.add_option( "-o", "--force-remote", @@ -295,13 +346,20 @@ def get_option_parser() -> COP: parser.add_option( "-s", "--submit-number", "-t", "--try-number", help="Job submit number (default=%s, i.e. latest)." % NN, - metavar="INT", action="store", dest="submit_num", default=NN) + metavar="INT", action="store", dest="submit_num", default=None) parser.add_option( "--remote-arg", help="(for internal use: continue processing on job host)", action="append", dest="remote_args") + parser.add_option( + '--prepend-path', + help='Prepend the file path to the output in the format :', + action='store_true', + default=False, + ) + return parser @@ -357,8 +415,15 @@ def main( batchview_cmd = options.remote_args[3] except IndexError: batchview_cmd = None - res = view_log(logpath, mode, tail_tmpl, batchview_cmd, remote=True, - color=color) + res = view_log( + logpath, + mode, + tail_tmpl, + batchview_cmd, + remote=True, + color=color, + prepend_path=options.prepend_path, + ) if res == 1: sys.exit(res) return @@ -371,28 +436,58 @@ def main( except KeyError: mode = options.mode + if tokens and tokens.get('cycle') and not tokens.get('task'): + print('Please provide a workflow, task or job ID', file=sys.stderr) + sys.exit(1) + if not tokens or not tokens.get('task'): - logpath = get_workflow_run_scheduler_log_path(workflow_id) - if options.rotation_num: - log_dir = Path(logpath).parent - logs = glob(f'{log_dir}/*{LOG_FILE_EXTENSION}') - logs.sort(key=os.path.getmtime, reverse=True) + # no task provided - user has requested a workflow log + log_dir: str = get_workflow_run_dir(workflow_id, 'log') + file_name: str = options.filename or 's' + log_file_path: Path + + if mode == 'list-dir': + # list workflow logs + print('\n'.join(sorted( + str(path.relative_to(log_dir)) + for dirpath in { + # set of log/ directories to scan for files in + Path(log_dir, _file_name).parent + for _, _file_name in WORKFLOW_LOG_OPTS.values() + } + for path in dirpath.iterdir() + # strip out file aliases such as scheduler/log + if not path.is_symlink() + ))) + return + + if file_name in WORKFLOW_LOG_OPTS: + rotation_number = options.rotation_num or 0 + pattern = WORKFLOW_LOG_OPTS[file_name][1] + logs = sorted( + glob( + str(Path(log_dir, pattern)) + ), + reverse=True + ) try: - logpath = logs[int(options.rotation_num)] + log_file_path = Path(logs[rotation_number]) except IndexError: - raise InputError( - "max rotation %d" % (len(logs) - 1)) - # Cat workflow logs, local only. - if options.filename is not None and not options.rotation_num: - logpath = os.path.join(get_workflow_run_scheduler_log_dir( - workflow_id), str(options.filename)) + raise InputError("max rotation %d" % (len(logs) - 1)) + else: + log_file_path = Path(log_dir, file_name) + tail_tmpl = os.path.expandvars( get_platform()["tail command template"] ) - out = view_log(logpath, mode, tail_tmpl, color=color) - if out == 1: - sys.exit(1) - return + out = view_log( + log_file_path, + mode, + tail_tmpl, + color=color, + prepend_path=options.prepend_path, + ) + sys.exit(out) else: # Cat task job logs, may be on workflow or job host. @@ -401,11 +496,13 @@ def main( "only workflow (not job) logs get rotated") task = tokens['task'] point = tokens['cycle'] - if options.submit_num != NN: + + submit_num = options.submit_num or tokens.get('job') or NN + if submit_num != NN: try: - options.submit_num = "%02d" % int(options.submit_num) + submit_num = "%02d" % int(submit_num) except ValueError: - parser.error("Illegal submit number: %s" % options.submit_num) + parser.error("Illegal submit number: %s" % submit_num) if options.filename is None: options.filename = JOB_LOG_OUT else: @@ -414,7 +511,7 @@ def main( options.filename = JOB_LOG_OPTS[options.filename] # KeyError: Is already long form (standard log, or custom). platform_name, job_runner_name, live_job_id = get_task_job_attrs( - workflow_id, point, task, options.submit_num) + workflow_id, point, task, submit_num) platform = get_platform(platform_name) batchview_cmd = None if live_job_id is not None: @@ -445,7 +542,7 @@ def main( and live_job_id is None) if log_is_remote and (not log_is_retrieved or options.force_remote): logpath = os.path.normpath(get_remote_workflow_run_job_dir( - workflow_id, point, task, options.submit_num, + workflow_id, point, task, submit_num, options.filename)) tail_tmpl = platform["tail command template"] # Reinvoke the cat-log command on the remote account. @@ -454,6 +551,8 @@ def main( cmd.append('--remote-arg=%s' % shlex.quote(item)) if batchview_cmd: cmd.append('--remote-arg=%s' % shlex.quote(batchview_cmd)) + if options.prepend_path: + cmd.append('--prepend-path') cmd.append(workflow_id) # TODO: Add Intelligent Host selection to this with suppress(KeyboardInterrupt): @@ -470,9 +569,15 @@ def main( else: # Local task job or local job log. logpath = os.path.normpath(get_workflow_run_job_dir( - workflow_id, point, task, options.submit_num, + workflow_id, point, task, submit_num, options.filename)) tail_tmpl = os.path.expandvars(platform["tail command template"]) - out = view_log(logpath, mode, tail_tmpl, batchview_cmd, - color=color) + out = view_log( + logpath, + mode, + tail_tmpl, + batchview_cmd, + color=color, + prepend_path=options.prepend_path, + ) sys.exit(out) diff --git a/tests/functional/cylc-cat-log/00-local.t b/tests/functional/cylc-cat-log/00-local.t index 44134052e6f..bc0e97f29c4 100755 --- a/tests/functional/cylc-cat-log/00-local.t +++ b/tests/functional/cylc-cat-log/00-local.t @@ -18,7 +18,7 @@ # Test "cylc cat-log" on the workflow host. . "$(dirname "$0")/test_header" #------------------------------------------------------------------------------- -set_test_number 31 +set_test_number 40 install_workflow "${TEST_NAME_BASE}" "${TEST_NAME_BASE}" #------------------------------------------------------------------------------- TEST_NAME="${TEST_NAME_BASE}-validate" @@ -32,11 +32,33 @@ contains_ok "${TEST_NAME}.stdout" "${WORKFLOW_RUN_DIR}/log/scheduler/log" #------------------------------------------------------------------------------- TEST_NAME=${TEST_NAME_BASE}-workflow-log-ok LOG_DIR="$(dirname "$(cylc cat-log -m p "${WORKFLOW_NAME}")")" +echo "This is file 02-restart-02.log" > "${LOG_DIR}/02-restart-02.log" echo "This is file 03-restart-02.log" > "${LOG_DIR}/03-restart-02.log" -run_ok "${TEST_NAME}" cylc cat-log -f 03-restart-02.log "${WORKFLOW_NAME}" +# it should accept file paths relative to the scheduler log directory +run_ok "${TEST_NAME}" cylc cat-log -f scheduler/03-restart-02.log "${WORKFLOW_NAME}" contains_ok "${TEST_NAME}.stdout" - << __END__ This is file 03-restart-02.log __END__ +# it should pick the latest scheduler log file if no rotation number is provided +run_ok "${TEST_NAME}" cylc cat-log --file s "${WORKFLOW_NAME}" +contains_ok "${TEST_NAME}.stdout" - << __END__ +This is file 03-restart-02.log +__END__ +# it should apply rotation number to scheduler log files +run_ok "${TEST_NAME}" cylc cat-log -f s -r 1 "${WORKFLOW_NAME}" +contains_ok "${TEST_NAME}.stdout" - << __END__ +This is file 02-restart-02.log +__END__ +# it should list scheduler log files +run_ok "${TEST_NAME}" cylc cat-log -m l "${WORKFLOW_NAME}" +cmp_ok "${TEST_NAME}.stdout" - << __END__ +config/01-start-01.cylc +config/flow-processed.cylc +install/01-install.log +scheduler/01-start-01.log +scheduler/02-restart-02.log +scheduler/03-restart-02.log +__END__ #------------------------------------------------------------------------------- TEST_NAME=${TEST_NAME_BASE}-task-out run_ok "${TEST_NAME}" cylc cat-log -f o "${WORKFLOW_NAME}//1/a-task" @@ -114,5 +136,11 @@ grep_ok "${WORKFLOW_NAME}/log/job/1/a-task/NN/job$" "${TEST_NAME}.stdout" TEST_NAME=${TEST_NAME_BASE}-un-norm-path run_fail "${TEST_NAME}" cylc cat-log -f j/../02/j "${WORKFLOW_NAME}//1/a-task" grep_ok 'InputError' "${TEST_NAME}.stderr" +#------------------------------------------------------------------------------- +TEST_NAME=${TEST_NAME_BASE}-prepend-path +run_ok "${TEST_NAME}-get-path" cylc cat-log -m p "${WORKFLOW_NAME}//1/a-task" +run_ok "${TEST_NAME}" cylc cat-log --prepend-path "${WORKFLOW_NAME}//1/a-task" +grep_ok "$(cat "#.*${TEST_NAME}-get-path.stdout")" "${TEST_NAME}.stdout" +#------------------------------------------------------------------------------- purge exit diff --git a/tests/functional/cylc-cat-log/00-local/flow.cylc b/tests/functional/cylc-cat-log/00-local/flow.cylc index 57acf58d2b6..383443109a0 100644 --- a/tests/functional/cylc-cat-log/00-local/flow.cylc +++ b/tests/functional/cylc-cat-log/00-local/flow.cylc @@ -10,12 +10,12 @@ [runtime] [[a-task]] script = """ -# Write to task stdout log -echo "the quick brown fox" -# Write to task stderr log -echo "jumped over the lazy dog" >&2 -# Write to a custom log file -echo "drugs and money" > ${CYLC_TASK_LOG_ROOT}.custom-log -# Generate a warning message in the workflow log. -cylc message -p WARNING 'marmite and squashed bananas' -""" + # Write to task stdout log + echo "the quick brown fox" + # Write to task stderr log + echo "jumped over the lazy dog" >&2 + # Write to a custom log file + echo "drugs and money" > ${CYLC_TASK_LOG_ROOT}.custom-log + # Generate a warning message in the workflow log. + cylc message -p WARNING 'marmite and squashed bananas' + """ diff --git a/tests/functional/cylc-cat-log/06-log-rotation.t b/tests/functional/cylc-cat-log/06-log-rotation.t index 888bb16fbc9..cff6e88f423 100755 --- a/tests/functional/cylc-cat-log/06-log-rotation.t +++ b/tests/functional/cylc-cat-log/06-log-rotation.t @@ -21,7 +21,7 @@ set_test_number 1 init_workflow "${TEST_NAME_BASE}" '/dev/null' # Populate its cylc-run dir with empty log files. -LOG_DIR="$(dirname "$(cylc cat-log -m p "${WORKFLOW_NAME}")")" +LOG_DIR="$HOME/cylc-run/$WORKFLOW_NAME/log/scheduler" mkdir -p "${LOG_DIR}" touch -t '201001011200.00' "${LOG_DIR}/01-start-01.log" touch -t '201001011200.01' "${LOG_DIR}/02-start-01.log" diff --git a/tests/functional/cylc-clean/06-nfs.t b/tests/functional/cylc-clean/06-nfs.t index 991242cea25..e9d94da2d4b 100644 --- a/tests/functional/cylc-clean/06-nfs.t +++ b/tests/functional/cylc-clean/06-nfs.t @@ -45,7 +45,7 @@ init_workflow "${TEST_NAME_BASE}" <<< '# blank workflow' WORKFLOW_LOG_DIR="${WORKFLOW_RUN_DIR}/log/scheduler" mkdir -p "$WORKFLOW_LOG_DIR" LOG_STUFF='foo bar baz' -echo "${LOG_STUFF}" > "${WORKFLOW_LOG_DIR}/log" +echo "${LOG_STUFF}" > "${WORKFLOW_LOG_DIR}/01-start-01.log" # start cat-log running - this runs "tail -f" cylc cat-log -m t "$WORKFLOW_NAME" > out 2>err & PID="$!" diff --git a/tests/functional/job-submission/16-timeout.t b/tests/functional/job-submission/16-timeout.t index c7eefb7af81..266b04b0164 100755 --- a/tests/functional/job-submission/16-timeout.t +++ b/tests/functional/job-submission/16-timeout.t @@ -40,8 +40,7 @@ workflow_run_ok "${TEST_NAME_BASE}-workflow-run" \ cylc cat-log "${WORKFLOW_NAME}" \ | grep -E -m 1 -A 2 "ERROR - \[jobs-submit cmd\]" \ | sed -e 's/^.* \(ERROR\)/\1/' > log - -WORKFLOW_LOG_DIR=$(cylc cat-log -m p "${WORKFLOW_NAME}") +WORKFLOW_LOG_DIR=$(cylc cat-log -m p "${WORKFLOW_NAME}" | sed 's/01-start-01\.//') JOB_LOG_DIR="${WORKFLOW_LOG_DIR%scheduler/log}" JOB_LOG_DIR="${JOB_LOG_DIR/$HOME/\$HOME}" From 0f924f6ca9cc19afd2846d56d87784f7a9bfcd57 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Thu, 20 Apr 2023 17:37:57 +0100 Subject: [PATCH 51/57] Fix broken `cylc help license` command --- .github/workflows/build.yml | 1 + conda-environment.yml | 1 + cylc/flow/scripts/cylc.py | 12 ++++++------ setup.cfg | 1 + 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index ca4571c8097..798521a1874 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -37,6 +37,7 @@ jobs: - name: Inspect run: | unzip -l dist/*.whl | tee files + grep 'cylc_flow.*.dist-info/COPYING' files grep 'cylc/flow/py.typed' files grep 'cylc/flow/etc' files grep 'cylc/flow/etc/cylc-completion.bash' files diff --git a/conda-environment.yml b/conda-environment.yml index 2a33dada664..3169c6ddc22 100644 --- a/conda-environment.yml +++ b/conda-environment.yml @@ -17,6 +17,7 @@ dependencies: - python - pyzmq >=22,<23 - setuptools >=49, <67 + - importlib_metadata - urwid >=2,<3 # Add # [py<3.11] for tomli once Python 3.11 Released - tomli >=2 diff --git a/cylc/flow/scripts/cylc.py b/cylc/flow/scripts/cylc.py index bd85cfa8e89..b524d681a0f 100644 --- a/cylc/flow/scripts/cylc.py +++ b/cylc/flow/scripts/cylc.py @@ -23,6 +23,7 @@ from typing import Iterator, NoReturn, Optional, Tuple from ansimarkup import parse as cparse +from importlib_metadata import files import pkg_resources from cylc.flow import __version__, iter_entry_points @@ -391,12 +392,11 @@ def print_id_help(): print(ID_HELP) -def print_license(): - print( - pkg_resources.get_distribution('cylc-flow').get_resource_string( - __name__, 'COPYING' - ).decode() - ) +def print_license() -> None: + license_file = next(filter( + lambda f: f.name == 'COPYING', files('cylc-flow') + )) + print(license_file.read_text()) def print_command_list(commands=None, indent=0): diff --git a/setup.cfg b/setup.cfg index bdd3b5abc2b..087ad0f31ee 100644 --- a/setup.cfg +++ b/setup.cfg @@ -74,6 +74,7 @@ install_requires = pyzmq==22.* # https://github.com/pypa/setuptools/issues/3802 setuptools>=49, <67 + importlib_metadata urwid==2.* # unpinned transient dependencies used for type checking rx From ee1e4812eb04c2bb0ee1f1b7e47095ccf628888a Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Tue, 4 Apr 2023 11:09:36 +0100 Subject: [PATCH 52/57] parsec: better error messages for section/setting mixups * Closes #4955 --- CHANGES.md | 4 ++ cylc/flow/config.py | 7 --- cylc/flow/parsec/upgrade.py | 8 ++- cylc/flow/parsec/validate.py | 22 ++++++- .../validate/76-section-section-transpose.t | 61 +++++++++++++++++++ tests/integration/test_config.py | 16 ----- 6 files changed, 92 insertions(+), 26 deletions(-) create mode 100644 tests/functional/validate/76-section-section-transpose.t diff --git a/CHANGES.md b/CHANGES.md index 014dc887841..988136ea43c 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -14,6 +14,10 @@ ones in. --> ### Fixes +[5450](https://github.com/cylc/cylc-flow/pull/5450) - Validation provides +better error messages if [sections] and settings are mixed up in a +configuration. + [5398](https://github.com/cylc/cylc-flow/pull/5398) - Fix platform from group selection order bug. diff --git a/cylc/flow/config.py b/cylc/flow/config.py index a549c9ed771..e5a6e7a1e54 100644 --- a/cylc/flow/config.py +++ b/cylc/flow/config.py @@ -356,13 +356,6 @@ def __init__( except KeyError: parameter_templates = {} - # Check that parameter templates are a section - if not hasattr(parameter_templates, 'update'): - raise WorkflowConfigError( - '[task parameters][templates] is a section. Don\'t use it ' - 'as a parameter.' - ) - # parameter values and templates are normally needed together. self.parameters = (parameter_values, parameter_templates) diff --git a/cylc/flow/parsec/upgrade.py b/cylc/flow/parsec/upgrade.py index 0c15b4ff1a3..e2e9840bca6 100644 --- a/cylc/flow/parsec/upgrade.py +++ b/cylc/flow/parsec/upgrade.py @@ -104,7 +104,13 @@ def obsolete(self, vn, oldkeys, silent=False, is_section=False): def get_item(self, keys): item = self.cfg for key in keys: - item = item[key] + try: + item = item[key] + except TypeError: + raise UpgradeError( + f'{self.show_keys(keys[:-1], True)}' + f' ("{keys[-2]}" should be a [section] not a setting)' + ) return item def put_item(self, keys, val): diff --git a/cylc/flow/parsec/validate.py b/cylc/flow/parsec/validate.py index 70bde7678bb..f6e40e6a912 100644 --- a/cylc/flow/parsec/validate.py +++ b/cylc/flow/parsec/validate.py @@ -204,10 +204,28 @@ def validate(self, cfg_root, spec_root): else: speckey = key specval = spec[speckey] - if isinstance(value, dict) and not specval.is_leaf(): + + cfg_is_section = isinstance(value, dict) + spec_is_section = not specval.is_leaf() + if cfg_is_section and not spec_is_section: + # config is a [section] but it should be a setting= + raise IllegalItemError( + keys, + key, + msg=f'"{key}" should be a setting not a [section]', + ) + if (not cfg_is_section) and spec_is_section: + # config is a setting= but it should be a [section] + raise IllegalItemError( + keys, + key, + msg=f'"{key}" should be a [section] not a setting', + ) + + if cfg_is_section and spec_is_section: # Item is dict, push to queue queue.append([value, specval, keys + [key]]) - elif value is not None and specval.is_leaf(): + elif value is not None and not spec_is_section: # Item is value, coerce according to value type cfg[key] = self.coercers[specval.vdr](value, keys + [key]) if specval.options: diff --git a/tests/functional/validate/76-section-section-transpose.t b/tests/functional/validate/76-section-section-transpose.t new file mode 100644 index 00000000000..6ca90ff0c2e --- /dev/null +++ b/tests/functional/validate/76-section-section-transpose.t @@ -0,0 +1,61 @@ +#!/usr/bin/env bash +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) 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 handling of mixed up sections vs settings + +. "$(dirname "$0")/test_header" + +set_test_number 6 + +# 1. section as setting (normal) +TEST_NAME='section-as-setting-normal' +cat > 'flow.cylc' <<__HEREDOC__ +[runtime] + [[foo]] + environment = 42 +__HEREDOC__ +run_fail "${TEST_NAME}-validate" cylc validate . +grep_ok \ + 'IllegalItemError: \[runtime\]\[foo\]environment - ("environment" should be a \[section\] not a setting)' \ + "${TEST_NAME}-validate.stderr" + + +# 2. section as setting (via upgrader) +# NOTE: if this test fails it is likely because the upgrader for "scheduling" +# has been removed, convert this to use a new deprecated section +TEST_NAME='section-as-setting-upgrader' +cat > 'flow.cylc' <<__HEREDOC__ +scheduling = 22 +__HEREDOC__ + +run_fail "${TEST_NAME}-validate" cylc validate . +grep_ok \ + 'UpgradeError: \[scheduling\] ("scheduling" should be a \[section\] not a setting' \ + "${TEST_NAME}-validate.stderr" + + +# 3. setting as section +TEST_NAME='setting-as-section' +cat > 'flow.cylc' <<__HEREDOC__ +[scheduling] + [[initial cycle point]] +__HEREDOC__ + +run_fail "${TEST_NAME}-validate" cylc validate . +grep_ok \ + 'IllegalItemError: \[scheduling\]initial cycle point - ("initial cycle point" should be a setting not a \[section\])' \ + "${TEST_NAME}-validate.stderr" diff --git a/tests/integration/test_config.py b/tests/integration/test_config.py index bc256f7cb60..b3799f213da 100644 --- a/tests/integration/test_config.py +++ b/tests/integration/test_config.py @@ -172,22 +172,6 @@ def test_no_graph(flow, validate): assert 'missing [scheduling][[graph]] section.' in str(exc_ctx.value) -def test_parameter_templates_setting(flow, one_conf, validate): - """It should fail if [task parameter]templates is a setting. - - It should be a section. - """ - reg = flow({ - **one_conf, - 'task parameters': { - 'templates': 'foo' - } - }) - with pytest.raises(WorkflowConfigError) as exc_ctx: - validate(reg) - assert '[templates] is a section' in str(exc_ctx.value) - - @pytest.mark.parametrize( 'section', [ 'external-trigger', From 62f5ef556a8c6d558041dace332130803634f0b8 Mon Sep 17 00:00:00 2001 From: Tim Pillinger <26465611+wxtim@users.noreply.github.com> Date: Fri, 21 Apr 2023 10:00:49 +0100 Subject: [PATCH 53/57] Fix.platform is regex remote tidy fail (#5445) platforms: fix issue with remote_tidy and platforms with hosts defined by regex patterns Closes #5429 --- CHANGES.md | 3 + cylc/flow/exceptions.py | 25 ++- cylc/flow/platforms.py | 50 ++---- cylc/flow/task_job_mgr.py | 2 +- cylc/flow/task_remote_mgr.py | 111 ++++++++---- .../01-periodic-clear-badhosts.t | 1 + tests/integration/test_task_remote_mgr.py | 167 ++++++++++++++++++ tests/unit/test_platforms.py | 54 +----- tests/unit/test_task_remote_mgr.py | 111 +++++++++++- 9 files changed, 407 insertions(+), 117 deletions(-) create mode 100644 tests/integration/test_task_remote_mgr.py diff --git a/CHANGES.md b/CHANGES.md index 014dc887841..7644721d777 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -14,6 +14,9 @@ ones in. --> ### Fixes +[5445](https://github.com/cylc/cylc-flow/pull/5445) - Fix remote tidy + bug where install target is not explicit in platform definition. + [5398](https://github.com/cylc/cylc-flow/pull/5398) - Fix platform from group selection order bug. diff --git a/cylc/flow/exceptions.py b/cylc/flow/exceptions.py index 073c8261b29..dd59fb9eec8 100644 --- a/cylc/flow/exceptions.py +++ b/cylc/flow/exceptions.py @@ -436,12 +436,29 @@ def __str__(self): class NoPlatformsError(PlatformLookupError): - """None of the platforms of a given group were reachable.""" - def __init__(self, platform_group): - self.platform_group = platform_group + """None of the platforms of a given set were reachable. + + Args: + identity: The name of the platform group or install target + set_type: Whether the set of platforms is a platform group or an + install target + place: Where the attempt to get the platform failed. + """ + def __init__( + self, identity: str, set_type: str = 'group', place: str = '' + ): + self.identity = identity + self.type = set_type + if place: + self.place = f' during {place}.' + else: + self.place = '.' def __str__(self): - return f'Unable to find a platform from group {self.platform_group}.' + return ( + f'Unable to find a platform from {self.type} {self.identity}' + f'{self.place}' + ) class CylcVersionError(CylcError): diff --git a/cylc/flow/platforms.py b/cylc/flow/platforms.py index 86806bd26b7..d4fc812187c 100644 --- a/cylc/flow/platforms.py +++ b/cylc/flow/platforms.py @@ -20,7 +20,8 @@ import re from copy import deepcopy from typing import ( - TYPE_CHECKING, Any, Dict, Iterable, List, Optional, Set, Union, overload + TYPE_CHECKING, Any, Dict, Iterable, + List, Optional, Set, Union, overload ) from cylc.flow import LOG @@ -630,18 +631,29 @@ def get_install_target_from_platform(platform: Dict[str, Any]) -> str: def get_install_target_to_platforms_map( - platform_names: Iterable[str] + platform_names: Iterable[str], + quiet: bool = False ) -> Dict[str, List[Dict[str, Any]]]: """Get a dictionary of unique install targets and the platforms which use them. Args: platform_names: List of platform names to look up in the global config. + quiet: Supress PlatformNotFound Errors Return {install_target_1: [platform_1_dict, platform_2_dict, ...], ...} """ platform_names = set(platform_names) - platforms = [platform_from_name(p_name) for p_name in platform_names] + platforms: List[Dict[str, Any]] = [] + for p_name in platform_names: + try: + platform = platform_from_name(p_name) + except PlatformLookupError as exc: + if not quiet: + raise exc + else: + platforms.append(platform) + install_targets = { get_install_target_from_platform(platform) for platform in platforms @@ -667,38 +679,6 @@ def is_platform_with_target_in_list( ) -def get_all_platforms_for_install_target( - install_target: str -) -> List[Dict[str, Any]]: - """Return list of platform dictionaries for given install target.""" - platforms: List[Dict[str, Any]] = [] - all_platforms = glbl_cfg(cached=True).get(['platforms'], sparse=False) - for k, v in all_platforms.iteritems(): # noqa: B301 (iteritems valid here) - if (v.get('install target', k) == install_target): - v_copy = deepcopy(v) - v_copy['name'] = k - platforms.append(v_copy) - return platforms - - -def get_random_platform_for_install_target( - install_target: str -) -> Dict[str, Any]: - """Return a randomly selected platform (dict) for given install target. - - Raises: - PlatformLookupError: We can't get a platform for this install target. - """ - platforms = get_all_platforms_for_install_target(install_target) - try: - return random.choice(platforms) # nosec (not crypto related) - except IndexError: - # No platforms to choose from - raise PlatformLookupError( - f'Could not select platform for install target: {install_target}' - ) - - def get_localhost_install_target() -> str: """Returns the install target of localhost platform""" localhost = get_platform() diff --git a/cylc/flow/task_job_mgr.py b/cylc/flow/task_job_mgr.py index 6c940d48842..358f3586847 100644 --- a/cylc/flow/task_job_mgr.py +++ b/cylc/flow/task_job_mgr.py @@ -155,7 +155,7 @@ def __init__(self, workflow, proc_pool, workflow_db_mgr, self.bad_hosts = bad_hosts self.bad_hosts_to_clear = set() self.task_remote_mgr = TaskRemoteMgr( - workflow, proc_pool, self.bad_hosts) + workflow, proc_pool, self.bad_hosts, self.workflow_db_mgr) def check_task_jobs(self, workflow, task_pool): """Check submission and execution timeout and polling timers. diff --git a/cylc/flow/task_remote_mgr.py b/cylc/flow/task_remote_mgr.py index c38c2ce1e4c..56a51fe8677 100644 --- a/cylc/flow/task_remote_mgr.py +++ b/cylc/flow/task_remote_mgr.py @@ -34,11 +34,13 @@ from time import sleep, time from typing import ( Any, Deque, Dict, TYPE_CHECKING, List, - NamedTuple, Optional, Tuple + NamedTuple, Optional, Set, Tuple ) from cylc.flow import LOG -from cylc.flow.exceptions import PlatformError +from cylc.flow.exceptions import ( + PlatformError, PlatformLookupError, NoHostsError, NoPlatformsError +) import cylc.flow.flags from cylc.flow.network.client_factory import CommsMeth from cylc.flow.pathutil import ( @@ -50,12 +52,10 @@ from cylc.flow.platforms import ( HOST_REC_COMMAND, PLATFORM_REC_COMMAND, - NoHostsError, - PlatformLookupError, get_host_from_platform, get_install_target_from_platform, + get_install_target_to_platforms_map, get_localhost_install_target, - get_random_platform_for_install_target, log_platform_event, ) from cylc.flow.remote import construct_rsync_over_ssh_cmd, construct_ssh_cmd @@ -96,7 +96,7 @@ class RemoteTidyQueueTuple(NamedTuple): class TaskRemoteMgr: """Manage task remote initialisation, tidy, selection.""" - def __init__(self, workflow, proc_pool, bad_hosts): + def __init__(self, workflow, proc_pool, bad_hosts, db_mgr): self.workflow = workflow self.proc_pool = proc_pool # self.remote_command_map = {command: host|PlatformError|None} @@ -110,6 +110,7 @@ def __init__(self, workflow, proc_pool, bad_hosts): self.bad_hosts = bad_hosts self.is_reload = False self.is_restart = False + self.db_mgr = db_mgr def _subshell_eval( self, eval_str: str, command_pattern: re.Pattern @@ -300,6 +301,41 @@ def construct_remote_tidy_ssh_cmd( cmd = construct_ssh_cmd(cmd, platform, host, timeout='10s') return cmd, host + @staticmethod + def _get_remote_tidy_targets( + platform_names: List[str], + install_targets: Set[str] + ) -> Dict[str, List[Dict[str, Any]]]: + """Finds valid platforms for install targets, warns about in invalid + install targets. + + logs: + A list of install targets where no platform can be found. + + returns: + A mapping of install targets to valid platforms only where + platforms are available. + """ + if install_targets and not platform_names: + install_targets_map: Dict[str, List[Dict[str, Any]]] = { + t: [] for t in install_targets} + unreachable_targets = install_targets + else: + install_targets_map = get_install_target_to_platforms_map( + platform_names, quiet=True) + # If we couldn't find a platform for a target, we cannot tidy it - + # raise an Error: + unreachable_targets = install_targets.difference( + install_targets_map) + + if unreachable_targets: + msg = 'No platforms available to remote tidy install targets:' + for unreachable_target in unreachable_targets: + msg += f'\n * {unreachable_target}' + LOG.error(msg) + + return install_targets_map + def remote_tidy(self) -> None: """Remove workflow contact files and keys from initialised remotes. @@ -307,37 +343,50 @@ def remote_tidy(self) -> None: This method is called on workflow shutdown, so we want nothing to hang. Timeout any incomplete commands after 10 seconds. """ + # Get a list of all platforms used from workflow database: + platforms_used = ( + self.db_mgr.get_pri_dao().select_task_job_platforms()) + # For each install target compile a list of platforms: + install_targets = { + target for target, msg + in self.remote_init_map.items() + if msg == REMOTE_FILE_INSTALL_DONE + } + install_targets_map = self._get_remote_tidy_targets( + platforms_used, install_targets) + # Issue all SSH commands in parallel queue: Deque[RemoteTidyQueueTuple] = deque() - for install_target, message in self.remote_init_map.items(): - if message != REMOTE_FILE_INSTALL_DONE: - continue + for install_target, platforms in install_targets_map.items(): if install_target == get_localhost_install_target(): continue - try: - platform = get_random_platform_for_install_target( - install_target - ) - cmd, host = self.construct_remote_tidy_ssh_cmd(platform) - except (NoHostsError, PlatformLookupError) as exc: - LOG.warning( - PlatformError( - f'{PlatformError.MSG_TIDY}\n{exc}', - platform['name'], + for platform in platforms: + try: + cmd, host = self.construct_remote_tidy_ssh_cmd(platform) + except NoHostsError as exc: + LOG.warning( + PlatformError( + f'{PlatformError.MSG_TIDY}\n{exc}', + platform['name'], + ) ) - ) - else: - log_platform_event('remote tidy', platform, host) - queue.append( - RemoteTidyQueueTuple( - platform, - host, - Popen( # nosec - cmd, stdout=PIPE, stderr=PIPE, stdin=DEVNULL, - text=True - ) # * command constructed by internal interface + else: + log_platform_event('remote tidy', platform, host) + queue.append( + RemoteTidyQueueTuple( + platform, + host, + Popen( # nosec + cmd, stdout=PIPE, stderr=PIPE, stdin=DEVNULL, + text=True + ) # * command constructed by internal interface + ) ) - ) + break + else: + LOG.error( + NoPlatformsError( + install_target, 'install target', 'remote tidy')) # Wait for commands to complete for a max of 10 seconds timeout = time() + 10.0 while queue and time() < timeout: diff --git a/tests/functional/intelligent-host-selection/01-periodic-clear-badhosts.t b/tests/functional/intelligent-host-selection/01-periodic-clear-badhosts.t index 07dc1c4b487..fef25c44fe0 100644 --- a/tests/functional/intelligent-host-selection/01-periodic-clear-badhosts.t +++ b/tests/functional/intelligent-host-selection/01-periodic-clear-badhosts.t @@ -76,6 +76,7 @@ platform: fake-platform - initialisation did not complete platform: fake-platform - remote init (on localhost) platform: fake-platform - remote file install (on localhost) platform: fake-platform - initialisation did not complete +platform: fake-platform - remote tidy (on localhost) __HERE__ purge diff --git a/tests/integration/test_task_remote_mgr.py b/tests/integration/test_task_remote_mgr.py new file mode 100644 index 00000000000..0fd462115f7 --- /dev/null +++ b/tests/integration/test_task_remote_mgr.py @@ -0,0 +1,167 @@ +# THIS FILE IS PART OF THE CYLC WORKFLOW ENGINE. +# Copyright (C) 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 . + +import cylc +from cylc.flow.task_remote_mgr import ( + REMOTE_FILE_INSTALL_DONE, + REMOTE_FILE_INSTALL_FAILED +) + + +async def test_remote_tidy( + flow, + scheduler, + start, + mock_glbl_cfg, + one_conf, + monkeypatch +): + """Remote tidy gets platforms for install targets. + + In particular, referencing https://github.com/cylc/cylc-flow/issues/5429, + ensure that install targets defined implicitly by platform name are found. + + Mock remote init map: + - Include an install target (quiz) with + message != REMOTE_FILE_INSTALL_DONE to ensure that + this is picked out. + - Install targets where we can get a platform + - foo - Install target is implicitly the platfrom name. + - bar9 - The install target is implicitly the plaform name, + and the platform name matches a platform regex. + - baz - Install target is set explicitly. + - An install target (qux) where we cannot get a platform: Ensure + that we get the desired error. + + Test that platforms with no good hosts (no host not in bad hosts). + """ + # Monkeypatch away subprocess.Popen calls - prevent any interaction with + # remotes actually happening: + class MockProc: + def __init__(self, *args, **kwargs): + self.poll = lambda: True + if ( + 'baum' in args[0] + or 'bay' in args[0] + ): + self.returncode = 255 + else: + self.returncode = 0 + self.communicate = lambda: ('out', 'err') + + monkeypatch.setattr( + cylc.flow.task_remote_mgr, + 'Popen', + lambda *args, **kwargs: MockProc(*args, **kwargs) + ) + + # Monkeypath function to add a sort order which we don't need in the + # real code but rely to prevent the test becoming flaky: + def mock_get_install_target_platforms_map(*args, **kwargs): + """Add sort to original function to ensure test consistency""" + from cylc.flow.platforms import get_install_target_to_platforms_map + result = get_install_target_to_platforms_map(*args, **kwargs) + sorted_result = {} + for key in sorted(result): + sorted_result[key] = sorted( + result[key], key=lambda x: x['name'], reverse=True) + return sorted_result + + monkeypatch.setattr( + cylc.flow.task_remote_mgr, + 'get_install_target_to_platforms_map', + mock_get_install_target_platforms_map + ) + + # Set up global config + mock_glbl_cfg( + 'cylc.flow.platforms.glbl_cfg', + ''' + [platforms] + [[foo]] + # install target = foo (implicit) + # hosts = foo (implicit) + [[bar.]] + # install target = bar1 to bar9 (implicit) + # hosts = bar1 to bar9 (implicit) + [[baz]] + install target = baz + # baum and bay should be uncontactable: + hosts = baum, bay, baz + [[[selection]]] + method = definition order + [[notthisone]] + install target = baz + hosts = baum, bay + [[bay]] + ''', + ) + + # Get a scheduler: + id_ = flow(one_conf) + schd = scheduler(id_) + async with start(schd) as log: + # Write database with 6 tasks using 3 platforms: + platforms = ['baz', 'bar9', 'foo', 'notthisone', 'bay'] + line = r"('', '', {}, 0, 1, '', '', 0,'', '', '', 0, '', '{}', 4, '')" + stmt = r"INSERT INTO task_jobs VALUES" + r','.join([ + line.format(i, platform) for i, platform in enumerate(platforms) + ]) + schd.workflow_db_mgr.pri_dao.connect().execute(stmt) + schd.workflow_db_mgr.pri_dao.connect().commit() + + # Mock a remote init map. + schd.task_job_mgr.task_remote_mgr.remote_init_map = { + 'baz': REMOTE_FILE_INSTALL_DONE, # Should match platform baz + 'bar9': REMOTE_FILE_INSTALL_DONE, # Should match platform bar. + 'foo': REMOTE_FILE_INSTALL_DONE, # Should match plaform foo + 'qux': REMOTE_FILE_INSTALL_DONE, # Should not match a plaform + 'quiz': REMOTE_FILE_INSTALL_FAILED, # Should not be considered + 'bay': REMOTE_FILE_INSTALL_DONE, # Should return NoPlatforms + } + + # Clear the log, run the test: + log.clear() + schd.task_job_mgr.task_remote_mgr.bad_hosts.update(['baum', 'bay']) + schd.task_job_mgr.task_remote_mgr.remote_tidy() + pass + + records = [str(r.msg) for r in log.records] + + # We can't get qux, no defined platform has a matching install target: + qux_msg = 'No platforms available to remote tidy install targets:\n * qux' + assert qux_msg in records + + # We can get foo bar baz, and we try to remote tidy them. + # (This will ultimately fail, but past the point we are testing). + for target in ['foo', 'bar9', 'baz']: + msg = f'platform: {target} - remote tidy (on {target})' + assert msg in records + + # We haven't done anything with Quiz because we're only looking + # at cases where platform == REMOTE_FILE_INSTALL_DONE + assert not [r for r in records if 'quiz' in r] + + notthisone_msg = ( + 'platform: notthisone - clean up did not complete' + '\nUnable to find valid host for notthisone' + ) + assert notthisone_msg in records + + bay_msg = ( + 'Unable to find a platform from install target' + ' bay during remote tidy.') + assert bay_msg in records diff --git a/tests/unit/test_platforms.py b/tests/unit/test_platforms.py index 8bbe19ebbd4..68d08cb8d16 100644 --- a/tests/unit/test_platforms.py +++ b/tests/unit/test_platforms.py @@ -21,10 +21,9 @@ from cylc.flow.parsec.OrderedDict import OrderedDictWithDefaults from cylc.flow.platforms import ( - get_all_platforms_for_install_target, get_platform, get_platform_deprecated_settings, - get_random_platform_for_install_target, is_platform_definition_subshell, + is_platform_definition_subshell, platform_from_name, platform_name_from_job_info, get_install_target_from_platform, get_install_target_to_platforms_map, @@ -414,6 +413,7 @@ def test_get_install_target_from_platform(platform, expected): assert get_install_target_from_platform(platform) == expected +@pytest.mark.parametrize('quiet', [True, False]) @pytest.mark.parametrize( 'platform_names, expected_map, expected_err', [ @@ -450,14 +450,19 @@ def test_get_install_target_to_platforms_map( platform_names: List[str], expected_map: Dict[str, Any], expected_err: Type[Exception], - monkeypatch: pytest.MonkeyPatch): + quiet: bool, + monkeypatch: pytest.MonkeyPatch +): """Test that get_install_target_to_platforms_map works as expected.""" monkeypatch.setattr('cylc.flow.platforms.platform_from_name', lambda x: platform_from_name(x, PLATFORMS_TREK)) - if expected_err: + if expected_err and not quiet: with pytest.raises(expected_err): get_install_target_to_platforms_map(platform_names) + elif expected_err and quiet: + # No error should be raised in quiet mode. + assert get_install_target_to_platforms_map(platform_names, quiet=quiet) else: result = get_install_target_to_platforms_map(platform_names) # Sort the maps: @@ -511,47 +516,6 @@ def test_generic_items_match(platform, job, remote, expect): assert generic_items_match(platform, job, remote) == expect -def test_get_all_platforms_for_install_target(mock_glbl_cfg): - mock_glbl_cfg( - 'cylc.flow.platforms.glbl_cfg', - ''' - [platforms] - [[localhost]] - hosts = localhost - install target = localhost - [[olaf]] - hosts = snow, ice, sparkles - install target = arendelle - [[snow white]] - hosts = happy, sleepy, dopey - install target = forest - [[kristoff]] - hosts = anna, elsa, hans - install target = arendelle - [[belle]] - hosts = beast, maurice - install target = france - [[bambi]] - hosts = thumper, faline, flower - install target = forest - [[merida]] - hosts = angus, fergus - install target = forest - [[forest]] - hosts = fir, oak, elm - ''' - ) - actual = get_all_platforms_for_install_target('forest') - expected = ['snow white', 'bambi', 'merida', 'forest'] - for platform in actual: - assert platform['name'] in expected - arendelle_platforms = ['kristoff', 'olaf'] - assert get_random_platform_for_install_target( - 'arendelle')['name'] in arendelle_platforms - assert get_random_platform_for_install_target( - 'forest')['name'] not in arendelle_platforms - - @pytest.mark.parametrize( 'task_conf, expected', [ diff --git a/tests/unit/test_task_remote_mgr.py b/tests/unit/test_task_remote_mgr.py index bb7b8cea085..539349da3fe 100644 --- a/tests/unit/test_task_remote_mgr.py +++ b/tests/unit/test_task_remote_mgr.py @@ -112,7 +112,7 @@ def test_get_log_file_name(tmp_path: Path, install_target: str, load_type: Optional[str], expected: str): - task_remote_mgr = TaskRemoteMgr('some_workflow', None, None) + task_remote_mgr = TaskRemoteMgr('some_workflow', None, None, None) if load_type == 'restart': task_remote_mgr.is_restart = True elif load_type == 'reload': @@ -127,3 +127,112 @@ def test_get_log_file_name(tmp_path: Path, log_name = task_remote_mgr.get_log_file_name( install_target, install_log_dir=log_dir) assert log_name == expected + + +@pytest.mark.parametrize( + 'platform_names, install_targets, glblcfg, expect', + [ + pytest.param( + # Two platforms share an install target. Both are reachable. + ['sir_handel', 'peter_sam'], + ['mountain_railway'], + ''' + [platforms] + [[peter_sam, sir_handel]] + install target = mountain_railway + ''', + { + 'targets': {'mountain_railway': ['peter_sam', 'sir_handel']}, + 'unreachable': set() + }, + id='basic' + ), + pytest.param( + # Two platforms share an install target. Both are unreachable. + set(), + ['mountain_railway'], + ''' + [platforms] + [[peter_sam, sir_handel]] + install target = mountain_railway + ''', + { + 'targets': {'mountain_railway': []}, + 'unreachable': {'mountain_railway'} + }, + id='platform_unreachable' + ), + pytest.param( + # One of our install targets matches one of our platforms, + # but only implicitly; i.e. the platform name is the same as the + # install target name. + ['sir_handel'], + ['sir_handel'], + ''' + [platforms] + [[sir_handel]] + ''', + { + 'targets': {'sir_handel': ['sir_handel']}, + 'unreachable': set() + }, + id='implicit-target' + ), + pytest.param( + # One of our install targets matches one of our platforms, + # but only implicitly, and the platform name is defined using a + # regex. + ['sir_handel42'], + ['sir_handel42'], + ''' + [platforms] + [[sir_handel..]] + ''', + { + 'targets': {'sir_handel42': ['sir_handel42']}, + 'unreachable': set() + }, + id='implicit-target-regex' + ), + pytest.param( + # One of our install targets (rusty) has no defined platforms + # causing a PlatformLookupError. + ['duncan', 'rusty'], + ['mountain_railway', 'rusty'], + ''' + [platforms] + [[duncan]] + install target = mountain_railway + ''', + { + 'targets': {'mountain_railway': ['duncan']}, + 'unreachable': {'rusty'} + }, + id='PlatformLookupError' + ) + ] +) +def test_map_platforms_used_for_install_targets( + mock_glbl_cfg, + platform_names, install_targets, glblcfg, expect, caplog +): + def flatten_install_targets_map(itm): + result = {} + for target, platforms in itm.items(): + result[target] = sorted([p['name'] for p in platforms]) + return result + + mock_glbl_cfg('cylc.flow.platforms.glbl_cfg', glblcfg) + + install_targets_map = TaskRemoteMgr._get_remote_tidy_targets( + set(platform_names), set(install_targets)) + + assert ( + expect['targets'] == flatten_install_targets_map(install_targets_map)) + + if expect['unreachable']: + for unreachable in expect["unreachable"]: + assert ( + unreachable in caplog.records[0].msg) + else: + assert not caplog.records From 79d68f0c0928f820d056f73ef296c5777b8fe9cd Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Fri, 21 Apr 2023 11:54:38 +0100 Subject: [PATCH 54/57] Address review --- conda-environment.yml | 5 ++--- cylc/flow/scripts/cylc.py | 5 ++++- setup.cfg | 2 +- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/conda-environment.yml b/conda-environment.yml index 3169c6ddc22..e128173868a 100644 --- a/conda-environment.yml +++ b/conda-environment.yml @@ -17,10 +17,9 @@ dependencies: - python - pyzmq >=22,<23 - setuptools >=49, <67 - - importlib_metadata + - importlib_metadata # [py<3.8] - urwid >=2,<3 - # Add # [py<3.11] for tomli once Python 3.11 Released - - tomli >=2 + - tomli >=2 # [py<3.11] # optional dependencies #- empy >=3.3,<3.4 diff --git a/cylc/flow/scripts/cylc.py b/cylc/flow/scripts/cylc.py index b524d681a0f..661f7045c1a 100644 --- a/cylc/flow/scripts/cylc.py +++ b/cylc/flow/scripts/cylc.py @@ -23,7 +23,6 @@ from typing import Iterator, NoReturn, Optional, Tuple from ansimarkup import parse as cparse -from importlib_metadata import files import pkg_resources from cylc.flow import __version__, iter_entry_points @@ -393,6 +392,10 @@ def print_id_help(): def print_license() -> None: + try: + from importlib.metadata import files + except ImportError: + from importlib_metadata import files license_file = next(filter( lambda f: f.name == 'COPYING', files('cylc-flow') )) diff --git a/setup.cfg b/setup.cfg index 087ad0f31ee..3b6743802a8 100644 --- a/setup.cfg +++ b/setup.cfg @@ -74,7 +74,7 @@ install_requires = pyzmq==22.* # https://github.com/pypa/setuptools/issues/3802 setuptools>=49, <67 - importlib_metadata + importlib_metadata; python_version < "3.8" urwid==2.* # unpinned transient dependencies used for type checking rx From f9b2742f8d1519dea1fae667a3ff4c1a1ced7970 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Fri, 21 Apr 2023 15:33:22 +0100 Subject: [PATCH 55/57] Update changelog --- CHANGES.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/CHANGES.md b/CHANGES.md index 014dc887841..37c5fc5b20f 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -23,6 +23,9 @@ shuts down if all hosts for all platforms in a platform group are unreachable. [#5384](https://github.com/cylc/cylc-flow/pull/5384) - Fixes `cylc set-verbosity`. +[#5479](https://github.com/cylc/cylc-flow/pull/5479) - +Fixes `cylc help license` + [#5394](https://github.com/cylc/cylc-flow/pull/5394) - Fixes a possible scheduler traceback observed with remote task polling. From 769602381347436d5b335ecfcc28bc683acae681 Mon Sep 17 00:00:00 2001 From: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com> Date: Fri, 21 Apr 2023 17:15:10 +0100 Subject: [PATCH 56/57] manylinux tests: update to ubuntu-20.04 ubuntu-18.04 no longer available --- .github/workflows/test_manylinux.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/test_manylinux.yml b/.github/workflows/test_manylinux.yml index 3a22636f59e..12d9e64025a 100644 --- a/.github/workflows/test_manylinux.yml +++ b/.github/workflows/test_manylinux.yml @@ -33,7 +33,7 @@ jobs: fail-fast: false matrix: manylinux: ['1'] - os: ['ubuntu-18.04'] # run on the oldest linux we have access to + os: ['ubuntu-20.04'] # run on the oldest linux we have access to python-version: ['3.7', '3.8', '3.9', '3.10'] steps: @@ -73,7 +73,7 @@ jobs: >> _manylinux.py - name: Install - timeout-minutes: 25 + timeout-minutes: 35 run: | PYTHONPATH="$PWD:$PYTHONPATH" pip install ."[all]" @@ -84,6 +84,6 @@ jobs: import cylc.flow.scheduler - name: Test - timeout-minutes: 15 + timeout-minutes: 5 run: | pytest -n 5 From d34ae5328fcf3f6741ec58a07f12c0836450cc97 Mon Sep 17 00:00:00 2001 From: Oliver Sanders Date: Tue, 25 Apr 2023 11:47:50 +0100 Subject: [PATCH 57/57] cat-log: don't list directories which don't exist (#5488) --- cylc/flow/scripts/cat_log.py | 2 ++ tests/functional/cylc-cat-log/00-local.t | 6 +++++- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/cylc/flow/scripts/cat_log.py b/cylc/flow/scripts/cat_log.py index 5b2223bacb0..a98f2de520d 100755 --- a/cylc/flow/scripts/cat_log.py +++ b/cylc/flow/scripts/cat_log.py @@ -454,6 +454,8 @@ def main( # set of log/ directories to scan for files in Path(log_dir, _file_name).parent for _, _file_name in WORKFLOW_LOG_OPTS.values() + # don't try to list directories which aren't there + if Path(log_dir, _file_name).parent.exists() } for path in dirpath.iterdir() # strip out file aliases such as scheduler/log diff --git a/tests/functional/cylc-cat-log/00-local.t b/tests/functional/cylc-cat-log/00-local.t index bc0e97f29c4..cf5c6d067e3 100755 --- a/tests/functional/cylc-cat-log/00-local.t +++ b/tests/functional/cylc-cat-log/00-local.t @@ -18,7 +18,7 @@ # Test "cylc cat-log" on the workflow host. . "$(dirname "$0")/test_header" #------------------------------------------------------------------------------- -set_test_number 40 +set_test_number 41 install_workflow "${TEST_NAME_BASE}" "${TEST_NAME_BASE}" #------------------------------------------------------------------------------- TEST_NAME="${TEST_NAME_BASE}-validate" @@ -142,5 +142,9 @@ run_ok "${TEST_NAME}-get-path" cylc cat-log -m p "${WORKFLOW_NAME}//1/a-task" run_ok "${TEST_NAME}" cylc cat-log --prepend-path "${WORKFLOW_NAME}//1/a-task" grep_ok "$(cat "#.*${TEST_NAME}-get-path.stdout")" "${TEST_NAME}.stdout" #------------------------------------------------------------------------------- +TEST_NAME=${TEST_NAME_BASE}-list-no-install-dir +rm -r "${WORKFLOW_RUN_DIR}/log/install" +run_ok "${TEST_NAME}-get-path" cylc cat-log -m l "${WORKFLOW_NAME}" +#------------------------------------------------------------------------------- purge exit