Move processing of stop cycle point to config.py #4827

MetRonnie · 2022-04-19T12:28:49Z

These changes close:

Requirements check-list

I have read CONTRIBUTING.md and added my name as a Code Contributor.
Contains logically grouped changes (else tidy your branch by rebase).
Does not contain off-topic changes (use other PRs for other changes).
Applied any dependency changes to both setup.cfg and conda-environment.yml.
Appropriate tests are included (unit, integration and functional!)
Appropriate change log entry included: c140476
Docs: Update docs for --fcp/--stopcp cylc-doc#459

MetRonnie · 2022-04-19T15:10:32Z

tests/functional/cylc-play/09-invalid-cp-opt.t

+# -----------------------------------------------------------------------------
+
+# Test that an invalid cycle point option does not cause an empty DB to be
+# created - https://github.com/cylc/cylc-flow/issues/4637


This test is mostly nabbed from #4680

MetRonnie · 2022-04-19T16:38:44Z

cylc/flow/scheduler_cli.py

@@ -147,7 +147,6 @@ def get_option_parser(add_std_opts=False):
            "Set the final cycle point. "
            "This command line option overrides the workflow "
            "config option '[scheduling]final cycle point'. "
-            "Use a value of 'reload' to reload from flow.cylc in a restart."


Removed this as it implies changes to flow.cylc will not be picked up normally. The true behaviour is too long-winded to explain here, see cylc/cylc-doc#459

wxtim

Read
Manually tested locally

There are a couple of Codecov holes. They aren't too bad IMO, but it would be nice if you could fix them.

oliver-sanders

Looks good, yet to manually test start/restart/reload'ing the stop cycle manully but looks like you have done that extensively in the automated tests.

oliver-sanders · 2022-04-21T09:22:14Z

cylc/flow/cfgspec/workflow.py

+
+            .. versionchanged:: 8.0.0
+
+               The deprecated ``[scheduling]max active cycle points`` setting


Unrelated, from user support yesterday this caused confusion making it look like "max active cycle points" is still deprecated.

Suggested change

The deprecated ``[scheduling]max active cycle points`` setting

The ``[scheduling]max active cycle points`` setting

But it is deprecated?

cylc-flow/cylc/flow/cfgspec/workflow.py

Lines 1709 to 1714 in 9ed6035

u.deprecate(

'8.0.0',

['scheduling', 'max active cycle points'],

['scheduling', 'runahead limit'],

cvtr=converter(lambda x: f'P{x}' if x != '' else '', '"n" -> "Pn"')

)

The setting is deprecated, however, the functionality is not.

# deprecated max active cycle points = 4 # fine runahead limit = P4

Because we had previously marked runahead limit as deprecated this has caused some confusion.

I've tried to improve the wording of that notice

oliver-sanders · 2022-04-25T09:40:38Z

cylc/flow/config.py

-            self.cfg['scheduling']['final cycle point'] is not None and
-            not self.cfg['scheduling']['final cycle point'].strip()
-        ):
+        if self.cfg['scheduling']['final cycle point'] == '':


The .strip() may or may not have been helpful, if you're totally sure it wasn't needed that's fine, otherwise perhaps:

Suggested change

if self.cfg['scheduling']['final cycle point'] == '':

if not self.cfg['scheduling']['final cycle point'].strip():

I tested out

final cycle point = " "

and put a breakpoint at this line; the value of self.cfg['scheduling']['final cycle point'] was just ''. Presumably parsec handles stripping (as I would hope so)

oliver-sanders · 2022-04-25T09:46:57Z

cylc/flow/cycling/iso8601.py



 @lru_cache(10000)
-def _point_parse(point_string):
+def _point_parse(point_string, _dump_fmt):


_dump_fmt arg does nothing?

It is needed for the caching to be valid, seeing as we rely on the WorkflowSpecifics.DUMP_FORMAT inside the function. This is how it is done in isodatetime too: https://github.com/metomi/isodatetime/blob/abec7c8ff05ab19d06f5eda790422df54ca72a8b/metomi/isodatetime/data.py#L2174-L2184

But this argument is not used within the function which makes it look like you can set this arg, which you cant.

Dunno if _dump_fmt=WorkflowSpecifics.DUMP_FORMAT would work with this mutable attribute?

If my understanding is correct, dump_fmt=WorkflowSpecifics.DUMP_FORMAT gets evaluated at time of function definition (when it is still None), not at time of call, so that would not be helpful.

IMHO, it's a "private" (leading underscore) argument in a "private" function, so it's an acceptable solution for the time being

oliver-sanders · 2022-04-25T09:55:37Z

tests/unit/test_config.py

+            # Need to check this as fcp treated as string by parsec, unlike
+            # other cycle point settings


"Need to check this as" == "This test is needed because"

datamel

Checked out and tested. No problems spotted with the implementation. Thanks @MetRonnie.

hjoliver · 2022-04-20T00:21:46Z

cylc/flow/config.py

-        ):
+        if self.cfg['scheduling']['final cycle point'] == '':
+            # (Unlike other cycle point settings in config, fcp is treated as
+            # a string by parsec)


Worth saying why (here, as well in the cfgspec file):

# NOTE: final cycle point is not a V_CYCLE_POINT to allow expressions # such as '+P1Y' (relative to initial cycle point)

- remove potentially misleading line from cylc-play usage - clear up versionchanged notice for runahead limit/max active cycle points setting

MetRonnie added the bug Something is wrong :( label Apr 19, 2022

MetRonnie added this to the cylc-8.0rc4 milestone Apr 19, 2022

MetRonnie self-assigned this Apr 19, 2022

MetRonnie added 5 commits April 19, 2022 14:56

Move processing of stop cycle point to config.py

d577a19

Tidy

9ed6035

Testing: add dump_format arg to set_cycling_type fixture

16cf303

Fix point_parse cache issue

79a38a7

Add test for empty DB when --stopcp is bad

30167e0

MetRonnie force-pushed the stopcp branch from 24dd2dd to 30167e0 Compare April 19, 2022 15:08

MetRonnie commented Apr 19, 2022

View reviewed changes

MetRonnie requested review from oliver-sanders, wxtim and datamel April 19, 2022 16:07

MetRonnie mentioned this pull request Apr 19, 2022

Update docs for --fcp/--stopcp cylc/cylc-doc#459

Merged

1 task

MetRonnie commented Apr 19, 2022

View reviewed changes

This was linked to issues Apr 20, 2022

Giving an invalid --stopcp on start "corrupts" database #4637

Closed

Fix stop after cycle point inconsistency with other cycle point options #4709

Closed

wxtim approved these changes Apr 25, 2022

View reviewed changes

oliver-sanders approved these changes Apr 25, 2022

View reviewed changes

MetRonnie force-pushed the stopcp branch 2 times, most recently from 31692eb to 66c913c Compare April 26, 2022 12:27

datamel approved these changes Apr 29, 2022

View reviewed changes

hjoliver approved these changes May 2, 2022

View reviewed changes

Improve docs

f2ba87b

- remove potentially misleading line from cylc-play usage - clear up versionchanged notice for runahead limit/max active cycle points setting

MetRonnie force-pushed the stopcp branch from 66c913c to f2ba87b Compare May 3, 2022 12:51

MetRonnie requested a review from oliver-sanders May 3, 2022 12:52

oliver-sanders merged commit 36dfa25 into cylc:master May 3, 2022

MetRonnie modified the milestones: cylc-8.0rc4, cylc-8.0rc3 May 3, 2022

MetRonnie deleted the stopcp branch May 3, 2022 15:00

MetRonnie mentioned this pull request May 4, 2022

2022 Cylc Meetings cylc/cylc-admin#143

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move processing of stop cycle point to config.py #4827

Move processing of stop cycle point to config.py #4827

MetRonnie commented Apr 19, 2022 •

edited

Loading

MetRonnie Apr 19, 2022

MetRonnie Apr 19, 2022 •

edited

Loading

wxtim left a comment

oliver-sanders left a comment

oliver-sanders Apr 21, 2022

MetRonnie Apr 26, 2022

oliver-sanders Apr 26, 2022

MetRonnie Apr 26, 2022

oliver-sanders Apr 25, 2022

MetRonnie Apr 26, 2022

oliver-sanders Apr 25, 2022

MetRonnie Apr 26, 2022

oliver-sanders Apr 26, 2022 •

edited

Loading

oliver-sanders Apr 26, 2022

MetRonnie Apr 26, 2022

oliver-sanders Apr 25, 2022

MetRonnie Apr 26, 2022

datamel left a comment

hjoliver Apr 20, 2022

MetRonnie May 3, 2022


		.. versionchanged:: 8.0.0

		The deprecated ``[scheduling]max active cycle points`` setting

	The deprecated ``[scheduling]max active cycle points`` setting
	The ``[scheduling]max active cycle points`` setting

	u.deprecate(
	'8.0.0',
	['scheduling', 'max active cycle points'],
	['scheduling', 'runahead limit'],
	cvtr=converter(lambda x: f'P{x}' if x != '' else '', '"n" -> "Pn"')
	)

	if self.cfg['scheduling']['final cycle point'] == '':
	if not self.cfg['scheduling']['final cycle point'].strip():

		# Need to check this as fcp treated as string by parsec, unlike
		# other cycle point settings

Move processing of stop cycle point to config.py #4827

Move processing of stop cycle point to config.py #4827

Conversation

MetRonnie commented Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

MetRonnie Apr 19, 2022 • edited Loading

Choose a reason for hiding this comment

wxtim left a comment

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliver-sanders Apr 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datamel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MetRonnie commented Apr 19, 2022 •

edited

Loading

MetRonnie Apr 19, 2022 •

edited

Loading

oliver-sanders Apr 26, 2022 •

edited

Loading