fix CPF wall_clock offset precision constraint #6431

dwsutherland · 2024-10-17T23:19:22Z

[scheduler]
    cycle point format = CCYYMMDDThh
    allow implicit tasks = True
[scheduling]
    initial cycle point = 20241017T22
    [[xtriggers]]
        clock_PT25M31S = wall_clock(offset=PT25M31S):PT20S
    [[graph]]
        T-00 = """
@clock_PT25M31S => a
"""
[runtime]
    [[root]]
        script = sleep $((1 + $RANDOM % 10))

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 if present).
Tests added.
Changelog entry included if this is a change that can affect users
If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

hjoliver · 2024-10-17T23:24:55Z

[x] Existing tests cover.

Presumably not, as we've been living with the bug?

dwsutherland · 2024-10-17T23:30:29Z

[x] Existing tests cover.

Presumably not, as we've been living with the bug?

Well coverage is 100%, and trigger offsets are tested ... But I could add a test that has CPF at lower resolution than offset

hjoliver · 2024-10-17T23:31:52Z

Yeah worth doing that I think.

dwsutherland · 2024-10-18T05:52:50Z

Yeah worth doing that I think.

Good thing I tried.. Found a way to break my solution, and have been working on various solutions all afternoon..
Will have it up by this evening.

dwsutherland · 2024-10-18T07:07:15Z

Tests added..

The solution had to ensure TimePoint + Duration were added together to make sure years and months were properly calculated before converting to seconds since unix epoc.

oliver-sanders · 2024-10-18T10:28:20Z

changes.d/6431.fix.md

@@ -0,0 +1 @@
+The `cycle point format` was imposing an undesirable constraint on `wall_clock` offsets, this has been fixed.


Suggested change

The `cycle point format` was imposing an undesirable constraint on `wall_clock` offsets, this has been fixed.

Allowed `wall_clock` offsets to use smaller precision than the `cycle point format` (e.g. `wall_clock(offset="PT1M")` in combination with `cycle point format = CCYY`).

Well technically they were capable of it before (the last 10 years in Cylc7 at least), but some change broke it causing this constraint.

Approved, but I prefer @oliver-sanders wording here, in terms of clarity for users.

MetRonnie

Tested 👍 Can actually be even simpler

MetRonnie · 2024-10-18T16:42:24Z

cylc/flow/task_proxy.py

+            point_time = point_parse(str(point))
            if offset_str == 'P0Y':
-                trigger_time = point
+                trigger_time = point_time
            else:
-                trigger_time = point + ISO8601Interval(offset_str)
-
-            offset = int(
-                point_parse(str(trigger_time)).seconds_since_unix_epoch)
-            self.clock_trigger_times[offset_str] = offset
+                trigger_time = (
+                    point_parse(
+                        point_time.__str__(strftime_format='%Y%m%dT%H%M%S'),
+                        '%Y%m%dT%H%M%S'
+                    )
+                    + interval_parse(offset_str)
+                )


There is no need to mess with the strftime_format:

Suggested change

point_time = point_parse(str(point))

if offset_str == 'P0Y':

trigger_time = point

trigger_time = point_time

else:

trigger_time = point + ISO8601Interval(offset_str)

offset = int(

point_parse(str(trigger_time)).seconds_since_unix_epoch)

self.clock_trigger_times[offset_str] = offset

trigger_time = (

point_parse(

point_time.__str__(strftime_format='%Y%m%dT%H%M%S'),

'%Y%m%dT%H%M%S'

)

+ interval_parse(offset_str)

)

# Convert ISO8601Point into metomi-isodatetime TimePoint at full

# second precision (N.B. it still dumps at the same precision

# as workflow cycle point format):

point_time = point_parse(str(point))

if offset_str == 'P0Y':

trigger_time = point_time

else:

trigger_time = point_time + interval_parse(offset_str)

No way! shocking .. Just tested this and it worked..

Ah I think I know where my confusion was ... I was originally converting these objects back to ISO8601Point and ISO8601Interval adding them together then parsing them again (with the string of the result) but you get a dump format and epoc seconds the same as the CPF (trimmed to that precision)...

The TimePoint object also has dump format I assumed they behaved the same and/or this was inherited. Which the dump format was the same, but obviously your suggestion shows the epoc seconds aren't truncated... Doh!

Reverting things to this.. Thanks

(p.s. sorry, should have merged yours in first then rebased)

Yeah I stuck a breakpoint in there and got confused by the dump format precision for a while 😄 Hopefully the comment I included makes it clear for people debugging in the future

MetRonnie · 2024-10-18T16:44:23Z

cylc/flow/cycling/iso8601.py

There is no need to change this file actually, see above comment

Noted above! thanks again

tests added

dwsutherland added this to the 8.3.6 milestone Oct 17, 2024

dwsutherland requested review from hjoliver and MetRonnie October 17, 2024 23:19

dwsutherland self-assigned this Oct 17, 2024

dwsutherland changed the base branch from master to 8.3.x October 17, 2024 23:19

dwsutherland changed the title ~~Wclock offset cpformat~~ fix CPF wall_clock offset precision constraint Oct 17, 2024

dwsutherland force-pushed the wclock-offset-cpformat branch from 07ba5de to c89c34f Compare October 18, 2024 07:01

dwsutherland force-pushed the wclock-offset-cpformat branch 2 times, most recently from 806fe1f to 3f009c1 Compare October 18, 2024 07:17

oliver-sanders added bug Something is wrong :( priority-1 A top pririty issue, e.g. a severe bug that affects multiple users labels Oct 18, 2024

oliver-sanders reviewed Oct 18, 2024

View reviewed changes

MetRonnie requested changes Oct 18, 2024

View reviewed changes

dwsutherland force-pushed the wclock-offset-cpformat branch from 3f009c1 to a63553f Compare October 19, 2024 01:10

dwsutherland requested a review from MetRonnie October 19, 2024 01:11

dwsutherland added 2 commits October 19, 2024 19:22

fix CPF wall_clock offset precision constraint

ea300ab

tests added

changelog entry added

01d15b5

dwsutherland force-pushed the wclock-offset-cpformat branch from a63553f to 01d15b5 Compare October 19, 2024 06:22

hjoliver approved these changes Oct 21, 2024

View reviewed changes

MetRonnie approved these changes Oct 21, 2024

View reviewed changes

MetRonnie merged commit 0a88f7a into cylc:8.3.x Oct 21, 2024
27 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix CPF wall_clock offset precision constraint #6431

fix CPF wall_clock offset precision constraint #6431

dwsutherland commented Oct 17, 2024 •

edited

Loading

hjoliver commented Oct 17, 2024

dwsutherland commented Oct 17, 2024

hjoliver commented Oct 17, 2024

dwsutherland commented Oct 18, 2024

dwsutherland commented Oct 18, 2024

oliver-sanders Oct 18, 2024

dwsutherland Oct 18, 2024 •

edited

Loading

hjoliver Oct 21, 2024

MetRonnie left a comment

MetRonnie Oct 18, 2024

dwsutherland Oct 19, 2024 •

edited

Loading

dwsutherland Oct 19, 2024

MetRonnie Oct 21, 2024

MetRonnie Oct 18, 2024

dwsutherland Oct 19, 2024 •

edited

Loading

		@@ -0,0 +1 @@
		The `cycle point format` was imposing an undesirable constraint on `wall_clock` offsets, this has been fixed.

	The `cycle point format` was imposing an undesirable constraint on `wall_clock` offsets, this has been fixed.
	Allowed `wall_clock` offsets to use smaller precision than the `cycle point format` (e.g. `wall_clock(offset="PT1M")` in combination with `cycle point format = CCYY`).

fix CPF wall_clock offset precision constraint #6431

fix CPF wall_clock offset precision constraint #6431

Conversation

dwsutherland commented Oct 17, 2024 • edited Loading

hjoliver commented Oct 17, 2024

dwsutherland commented Oct 17, 2024

hjoliver commented Oct 17, 2024

dwsutherland commented Oct 18, 2024

dwsutherland commented Oct 18, 2024

oliver-sanders Oct 18, 2024

Choose a reason for hiding this comment

dwsutherland Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

hjoliver Oct 21, 2024

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

MetRonnie Oct 18, 2024

Choose a reason for hiding this comment

dwsutherland Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

dwsutherland Oct 19, 2024

Choose a reason for hiding this comment

MetRonnie Oct 21, 2024

Choose a reason for hiding this comment

MetRonnie Oct 18, 2024

Choose a reason for hiding this comment

dwsutherland Oct 19, 2024 • edited Loading

Choose a reason for hiding this comment

dwsutherland commented Oct 17, 2024 •

edited

Loading

dwsutherland Oct 18, 2024 •

edited

Loading

dwsutherland Oct 19, 2024 •

edited

Loading

dwsutherland Oct 19, 2024 •

edited

Loading