Fix.all xtriggers on an itask are the same

[wxtim]

Closes #5783

The task proxy object was caching the value of clock trigger time. Only the first xtrigger was checked!

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 are included (or explain why tests are not needed).
• CHANGES.md entry included if this is a change that can affect users
• Cylc-Doc pull request opened if required at cylc/cylc-doc/pull/XXXX.
• If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

[hjoliver]

I guess we had (wrongly) assumed a task would only ever have one clock trigger, so cache to avoid doing the datetime computation every time the trigger is checked.

With multiple clock triggers, it might still be worth caching all the values?

[wxtim]

multiple clock triggers, it might still be worth caching all the values?

I have done this, although I'm not sure how much time the memoization saves.

[oliver-sanders]

LGTM

[MetRonnie]

Could avoid this line by just storing None in the clock_trigger_times dict?

[wxtim]

Yes, although I've never tried using None as a dictionary key before, and was somewhat surprised by it as a thing.

[wxtim]

It's a bit more complex in practice - because if offset_str is in self.clock_trigger_times it makes the following logic invalid and you have to write a separate loop caching the conversion of P0Y into seconds since epoch. I ended up with this diff:

-        # None cannot be used as a dict key:
-        offset_str = offset_str if offset_str else 'P0Y'
-        if offset_str not in self.clock_trigger_times:
-            if offset_str == 'P0Y':
-                trigger_time = point
-            else:
-                trigger_time = point + ISO8601Interval(offset_str)
-
+        trigger_time = None
+        if (
+            offset_str is None
+            and self.clock_trigger_times[offset_str] == 'P0Y'
+        ):
+            trigger_time = point
+        elif offset_str not in self.clock_trigger_times:
+            trigger_time = point + ISO8601Interval(offset_str)
+        if trigger_time:

[MetRonnie]

None is hashable so can be used as a dict key

[wxtim]

None is hashable so can be used as a dict key

Yes. TIL - still doesn't deal with the other problem.
Removed the plain wrong comment about using None as a dictionary key.

[MetRonnie]

It's a bit more complex in practice - because if offset_str is in self.clock_trigger_times it makes the following logic invalid and you have to write a separate loop caching the conversion of P0Y into seconds since epoch. I ended up with this diff:

I'm not sure what you mean by this. What I am suggesting is:

diff --git a/cylc/flow/task_proxy.py b/cylc/flow/task_proxy.py
index ac1ec5c1c..1fcd91b71 100644
--- a/cylc/flow/task_proxy.py
+++ b/cylc/flow/task_proxy.py
@@ -248,5 +248,5 @@ class TaskProxy:
         self.non_unique_events = Counter()  # type: ignore # TODO: figure out
 
-        self.clock_trigger_times: Dict[str, int] = {}
+        self.clock_trigger_times: Dict[Optional[str], int] = {}
         self.expire_time: Optional[float] = None
         self.late_time: Optional[float] = None
@@ -370,15 +370,13 @@ class TaskProxy:
 
         """
-        # None cannot be used as a dict key:
-        offset_str = offset_str if offset_str else 'P0Y'
         if offset_str not in self.clock_trigger_times:
-            if offset_str == 'P0Y':
+            if not offset_str:
                 trigger_time = point
             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
+            self.clock_trigger_times[offset_str] = int(
+                point_parse(str(trigger_time)).seconds_since_unix_epoch
+            )
         return self.clock_trigger_times[offset_str]

[wxtim]

Too late - Oliver's merged it! - And yes, I had misunderstood.

[hjoliver]

LGTM. Leaving umerged while @MetRonnie 's comments are open.