Make `--stopcp=reload` use `[scheduling]stop after cycle point` instead of the final cycle point #4543

MetRonnie · 2021-12-03T15:58:12Z

These changes partially address #4062

cylc play --stopcp=reload now does what it says - reload the value from [scheduling]stop after cycle point. (Previously it would use the value of the final cycle point)

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/updated (unit and functional).
Appropriate change log entry included.
(master branch) I have opened a documentation PR at Add docs for cycle point options in restart cylc-doc#362

Including remove unnecessary tests that cover the same functionality as other tests

It now takes value from `[scheduling]stop after cycle point` instead of the final cycle point

MetRonnie · 2021-12-03T16:00:16Z

tests/functional/cylc-play/06-warnif-scp-after-fcp.t

@@ -53,35 +51,3 @@ for SCP in 1 2 3; do
 done

 purge
-
-# Gregorian Cycling


No need to check this for different cycling types; it should be the responsibility of the tests/unit/cycling and tests/functional/cyclers tests to ensure cycle point comparison works

MetRonnie · 2021-12-03T16:02:11Z

tests/functional/restart/44-stop-point.t

-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-#-------------------------------------------------------------------------------
-# Test restart with stop point


This covered the same stuff as tests/functional/restart/08-stop-after-cycle-point.t

MetRonnie · 2021-12-03T18:20:49Z

cylc/flow/scheduler.py

-            if self.config.start_point is None:
-                # No start cycle point at which to load cycling tasks.
-                continue


Pretty sure this condition is no longer possible. And L691 showing as never hit in code coverage: https://app.codecov.io/gh/cylc/cylc-flow/blob/master/cylc/flow/scheduler.py

Checked in practice. Confirmed I can't get it to be true.

MetRonnie · 2021-12-03T18:22:21Z

cylc/flow/scheduler.py

-        if self.options.startcp:
-            self.config.start_point = TaskID.get_standardised_point(
-                self.options.startcp)


This seems to be unnecessary as duplication of

cylc-flow/cylc/flow/config.py

Lines 669 to 687 in b366e13

def process_start_cycle_point(self) -> None:

"""Set the start cycle point from options.

Sets:

self.options.startcp

self.start_point

"""

startcp = getattr(self.options, 'startcp', None)

starttask = getattr(self.options, 'starttask', None)

if startcp is not None and starttask is not None:

raise UserInputError(

"--start-cycle-point and --start-task are mutually exclusive"

)

if startcp:

# Start from a point later than initial point.

if self.options.startcp == 'now':

self.options.startcp = get_current_time_string()

self.start_point = get_point(self.options.startcp).standardise()

Confirm looks reasonable.

MetRonnie · 2021-12-03T18:23:20Z

cylc/flow/scheduler.py

-        if not self.config.initial_point and not self.is_restart:
-            LOG.warning('No initial cycle point provided - no cycling tasks '
-                        'will be loaded.')
-


Again pretty sure this condition is no longer possible (config.initial_point should be set by this point)

MetRonnie · 2021-12-03T18:27:20Z

cylc/flow/scheduler.py

-            if self.is_restart and self.options.icp == 'reload':
-                LOG.debug(f"- initial point = {value} (ignored)")
-            elif self.options.icp is None:
-                self.options.icp = value
-                LOG.info(f"+ initial point = {value}")
+            self.options.icp = value
+            LOG.info(f"+ initial point = {value}")
        elif key in self.workflow_db_mgr.KEY_START_CYCLE_POINT_COMPATS:


Since #4313, initial and start cycle points cannot change on restart. But still need to set self.options.icp/startcp from DB as they are used later to set self.config.initial_point/start_point.

cylc/flow/scheduler.py

wxtim

Read, looks reasonable.
Checked to see if I can trigger conditions removed as surplus to reqs.

Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>

hjoliver

Thanks @MetRonnie - good to have this confusing mess cleared up!

MetRonnie added 3 commits December 3, 2021 11:27

Tidy

32ed7d6

Including remove unnecessary tests that cover the same functionality as other tests

Fix behaviour of --stopcp=reload

f4b2ab4

It now takes value from `[scheduling]stop after cycle point` instead of the final cycle point

Type annotations

6c27190

MetRonnie added the could be better Not exactly a bug, but not ideal. label Dec 3, 2021

MetRonnie added this to the cylc-8.0rc1 milestone Dec 3, 2021

MetRonnie self-assigned this Dec 3, 2021

MetRonnie commented Dec 3, 2021

View reviewed changes

MetRonnie requested review from wxtim and hjoliver December 3, 2021 16:04

MetRonnie mentioned this pull request Dec 3, 2021

Re-evaluate the --initial/final/start/stop-cycle-point options for restarts #4062

Closed

5 tasks

MetRonnie added 2 commits December 3, 2021 18:03

Changelog

986ae87

Tidy

b366e13

MetRonnie commented Dec 3, 2021

View reviewed changes

wxtim reviewed Dec 6, 2021

View reviewed changes

cylc/flow/scheduler.py Outdated Show resolved Hide resolved

wxtim reviewed Dec 6, 2021

View reviewed changes

cylc/flow/scheduler.py Show resolved Hide resolved

wxtim approved these changes Dec 6, 2021

View reviewed changes

Update CLI help for --fcp & --stopcp

cb851c4

MetRonnie mentioned this pull request Dec 6, 2021

Add docs for cycle point options in restart cylc/cylc-doc#362

Merged

Update cylc/flow/scheduler.py

433072d

Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>

hjoliver approved these changes Dec 7, 2021

View reviewed changes

hjoliver merged commit 9a3d5b6 into cylc:master Dec 7, 2021

MetRonnie deleted the stopcp branch December 7, 2021 10:21

MetRonnie linked an issue Dec 8, 2021 that may be closed by this pull request

Re-evaluate the --initial/final/start/stop-cycle-point options for restarts #4062

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make `--stopcp=reload` use `[scheduling]stop after cycle point` instead of the final cycle point #4543

Make `--stopcp=reload` use `[scheduling]stop after cycle point` instead of the final cycle point #4543

MetRonnie commented Dec 3, 2021 •

edited

Loading

MetRonnie Dec 3, 2021

MetRonnie Dec 3, 2021

MetRonnie Dec 3, 2021

wxtim Dec 6, 2021

MetRonnie Dec 3, 2021

wxtim Dec 6, 2021

MetRonnie Dec 3, 2021

wxtim Dec 6, 2021

MetRonnie Dec 3, 2021 •

edited

Loading

wxtim left a comment

hjoliver left a comment

	def process_start_cycle_point(self) -> None:
	"""Set the start cycle point from options.

	Sets:
	self.options.startcp
	self.start_point
	"""
	startcp = getattr(self.options, 'startcp', None)
	starttask = getattr(self.options, 'starttask', None)

	if startcp is not None and starttask is not None:
	raise UserInputError(
	"--start-cycle-point and --start-task are mutually exclusive"
	)
	if startcp:
	# Start from a point later than initial point.
	if self.options.startcp == 'now':
	self.options.startcp = get_current_time_string()
	self.start_point = get_point(self.options.startcp).standardise()

Make --stopcp=reload use [scheduling]stop after cycle point instead of the final cycle point #4543

Make --stopcp=reload use [scheduling]stop after cycle point instead of the final cycle point #4543

Conversation

MetRonnie commented Dec 3, 2021 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MetRonnie Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

wxtim left a comment

Choose a reason for hiding this comment

hjoliver left a comment

Choose a reason for hiding this comment

Make `--stopcp=reload` use `[scheduling]stop after cycle point` instead of the final cycle point #4543

Make `--stopcp=reload` use `[scheduling]stop after cycle point` instead of the final cycle point #4543

MetRonnie commented Dec 3, 2021 •

edited

Loading

MetRonnie Dec 3, 2021 •

edited

Loading