Improve and refactor checking of initial, start, stop and final cycle points

Author: wxtim

changes.d/6874.fix.md

@@ -0,0 +1 @@
+Improved warnings for suspect start and stop cycle points.

cylc/flow/config.py

@@ -471,6 +471,10 @@ def __init__(
         self.process_start_cycle_point()
         self.process_final_cycle_point()
         self.process_stop_cycle_point()
+        if self.start_point:
+            self.cycle_point_warning('start', 'before', 'initial')
+            self.cycle_point_warning('start', 'after', 'final')
+            self.cycle_point_warning('stop', 'before', 'start')
 
         # Parse special task cycle point offsets, and replace family names.
         LOG.debug("Parsing [special tasks]")
@@ -797,6 +801,7 @@ def process_start_cycle_point(self) -> None:
             if self.options.startcp == 'now':
                 self.options.startcp = get_current_time_string()
             self.start_point = get_point(self.options.startcp).standardise()
+
         elif starttask:
             # Start from designated task(s).
             # Select the earliest start point for use in pre-initial ignore.
@@ -815,6 +820,25 @@ def process_start_cycle_point(self) -> None:
             # Start from the initial point.
             self.start_point = self.initial_point
 
+    def cycle_point_warning(self, label1, order, label2):
+        """Centralized logic for warning that start, stop, initial and
+        final cycle points are not sensibly ordered.
+        """
+        point1 = getattr(self, label1 + '_point')
+        point2 = getattr(self, label2 + '_point')
+        if (
+            order == 'before' and point1 < point2
+            or order == 'after' and point1 > point2
+        ):
+            msg = (
+                f"{label1} cycle point '{point1}' will have no effect as"
+                f" it is {order} the {label2} cycle point '{point2}'."
+            )
+            # Capitalize() makes the ISO strings look very odd:
+            LOG.warning(msg[0].upper() + msg[1:])
+            return True
+        return False
+
     def process_final_cycle_point(self) -> None:
         """Validate and set the final cycle point from flow.cylc or options.
 
@@ -895,15 +919,15 @@ def process_stop_cycle_point(self) -> None:
                 self.initial_point,
             ).standardise()
             if (
-                self.final_point is not None
-                and self.stop_point is not None
-                and self.stop_point > self.final_point
-            ):
-                LOG.warning(
-                    f"Stop cycle point '{self.stop_point}' will have no "
-                    "effect as it is after the final cycle "
-                    f"point '{self.final_point}'."
+                self.stop_point is not None
+                and (
+                    (
+                        self.final_point is not None
+                        and self.cycle_point_warning('stop', 'after', 'final')
+                    )
+                    or self.cycle_point_warning('stop', 'before', 'initial')
                 )
+            ):
                 self.stop_point = None
             stopcp_str = str(self.stop_point) if self.stop_point else None
             self.cfg['scheduling']['stop after cycle point'] = stopcp_str

tests/integration/test_config.py

@@ -670,3 +670,119 @@ async def test_invalid_starttask(one_conf, flow, scheduler, start):
     with pytest.raises(InputError, match='a///b'):
         async with start(schd):
             pass
+
+
+@pytest.mark.parametrize(
+    'a, b, c, d, validation_fail, err',
+    (
+        ('initial', 'start', 'stop', 'final', True, False),
+        (
+            'initial', 'start', 'final', 'stop', True,
+            "Stop cycle point '20030101T0000Z' will have no effect as it"
+            " is after the final cycle point '20020101T0000Z'."
+        ),
+        (
+            'initial', 'stop', 'start', 'final', False,
+            "Stop cycle point '20010101T0000Z' will have no effect as it"
+            " is before the start cycle point '20020101T0000Z'."
+        ),
+        (
+            'initial', 'stop', 'final', 'start', False,
+            "Start cycle point '20030101T0000Z' will have no effect as it"
+            " is after the final cycle point '20020101T0000Z'."
+        ),
+        (
+            'initial', 'final', 'start', 'stop', True,
+            "Stop cycle point '20030101T0000Z' will have no effect as it"
+            " is after the final cycle point '20010101T0000Z'."
+        ),
+        (
+            'initial', 'final', 'stop', 'start', True,
+            "Stop cycle point '20020101T0000Z' will have no effect as it"
+            " is after the final cycle point '20010101T0000Z'."
+        ),
+        (
+            'start', 'initial', 'stop', 'final', False,
+            "Start cycle point '20000101T0000Z' will have no effect as it"
+            " is before the initial cycle point '20010101T0000Z'."
+        ),
+        (
+            'start', 'initial', 'final', 'stop', True,
+            "Stop cycle point '20030101T0000Z' will have no effect as it"
+            " is after the final cycle point '20020101T0000Z'."
+        ),
+        (
+            'start', 'stop', 'initial', 'final', True,
+            "Stop cycle point '20010101T0000Z' will have no effect as it"
+            " is before the initial cycle point '20020101T0000Z'."
+        ),
+        ('start', 'stop', 'final', 'initial', True, WorkflowConfigError),
+        ('start', 'final', 'initial', 'stop', True, WorkflowConfigError),
+        ('start', 'final', 'stop', 'initial', True, WorkflowConfigError),
+        (
+            'stop', 'initial', 'start', 'final', True,
+            "Stop cycle point '20000101T0000Z' will have no effect as it"
+            " is before the initial cycle point '20010101T0000Z'."
+        ),
+        (
+            'stop', 'initial', 'final', 'start', True,
+            "Stop cycle point '20000101T0000Z' will have no effect as it"
+            " is before the initial cycle point '20010101T0000Z'."
+        ),
+        (
+            'stop', 'start', 'initial', 'final', True,
+            "Stop cycle point '20000101T0000Z' will have no effect as it"
+            " is before the initial cycle point '20020101T0000Z'."
+        ),
+        ('stop', 'start', 'final', 'initial', True, WorkflowConfigError),
+        ('stop', 'final', 'initial', 'start', True, WorkflowConfigError),
+        ('stop', 'final', 'start', 'initial', True, WorkflowConfigError),
+        ('final', 'initial', 'start', 'stop', True, WorkflowConfigError),
+        ('final', 'initial', 'stop', 'start', True, WorkflowConfigError),
+        ('final', 'start', 'initial', 'stop', True, WorkflowConfigError),
+        ('final', 'start', 'stop', 'initial', True, WorkflowConfigError),
+        ('final', 'stop', 'initial', 'start', True, WorkflowConfigError),
+        ('final', 'stop', 'start', 'initial', True, WorkflowConfigError),
+    )
+)
+async def test_milestone_cycle_points(
+    a,
+    b,
+    c,
+    d,
+    validation_fail,
+    err,
+    flow,
+    validate,
+    scheduler,
+    start,
+    log_filter,
+    caplog,
+):
+    """Ensure that all combinations of initial, start, stop and final cycle
+    point return sensible warnings or errors.
+    """
+    order = dict(zip((a, b, c, d), [2000, 2001, 2002, 2003]))
+
+    wid = flow({
+        'scheduling': {
+            'initial cycle point': order['initial'],
+            'stop after cycle point': order['stop'],
+            'final cycle point': order['final'],
+            'graph': {'P1Y': 'foo'}
+        },
+    })
+    if validation_fail:
+        if not err:
+            validate(wid)
+        elif isinstance(err, str):
+            validate(wid)
+            assert err in caplog.messages
+        else:
+            with pytest.raises(err):
+                validate(wid)
+
+    else:
+        schd = scheduler(wid, startcp=str(order['start']))
+        async with start(schd):
+            assert err in caplog.messages

tests/unit/test_config.py

@@ -14,10 +14,12 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
+from functools import partial
+import os
+from optparse import Values
+import pytest
 from contextlib import suppress
 import logging
-from optparse import Values
-import os
 from pathlib import Path
 from textwrap import dedent
 from types import SimpleNamespace
@@ -31,8 +33,6 @@
     Type,
 )
 
-import pytest
-
 from cylc.flow import (
     CYLC_LOG,
     flags,
@@ -710,7 +710,7 @@ def test_process_fcp(
             id="stopcp-beyond-fcp"
         ),
         pytest.param(
-            '+P12Y -P2Y', None, '2000', None, None,
+            '+P12Y -P2Y', None, '1010', None, None,
             id="stopcp-relative-to-icp"
         ),
     ]
@@ -742,11 +742,13 @@ def test_process_stop_cycle_point(
                 'stop after cycle point': cfg_stopcp
             }
         },
-        initial_point=ISO8601Point('1990'),
+        initial_point=ISO8601Point('1000'),
         final_point=fcp,
         stop_point=None,
         options=RunOptions(stopcp=options_stopcp),
     )
+    mock_config.cycle_point_warning = partial(
+        WorkflowConfig.cycle_point_warning, mock_config)
 
     WorkflowConfig.process_stop_cycle_point(mock_config)
     assert str(mock_config.stop_point) == str(expected_value)