Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix evaluation of conditional expressions at negative cycle points #6590

Draft
wants to merge 3 commits into
base: 8.4.x
Choose a base branch
from

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Feb 5, 2025

Integer offsets [-P2] or bigger aren't getting replaced with Python code in prerequisite conditional expressions because they generate negative cycle points when evaluated at initial cycle point 1. (There's no problem with [-P1] - that generates a cycle point of 0).

Close #6588

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).
  • Changelog 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 hjoliver added the bug Something is wrong :( label Feb 5, 2025
@hjoliver hjoliver requested a review from MetRonnie February 5, 2025 00:30
@hjoliver hjoliver self-assigned this Feb 5, 2025
@hjoliver hjoliver added this to the 8.4.x milestone Feb 5, 2025
Comment on lines -214 to +224
fr"\b{re.escape(self.MESSAGE_TEMPLATE % t_output)}\b",
fr"{re.escape(self.MESSAGE_TEMPLATE % t_output)}\b",
Copy link
Member Author

@hjoliver hjoliver Feb 5, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: consider the preceding comment - might need a slightly more sophisticated fix?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Tests all pass, but maybe the commented issue only tested the other word boundary for task name sub-strings)

Copy link
Member

@MetRonnie MetRonnie Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test added in #4426 is no longer valid since universal ID was implemented.

This is needed to make the test correct

diff --git a/cylc/flow/prerequisite.py b/cylc/flow/prerequisite.py
index 717276affc..0ad3ef7eee 100644
--- a/cylc/flow/prerequisite.py
+++ b/cylc/flow/prerequisite.py
@@ -193,14 +193,13 @@ class Prerequisite:
         Examples:
             # GH #3644 construct conditional expression when one task name
-            # is a substring of another: foo | xfoo => bar.
-            # Add 'foo' to the 'satisfied' dict before 'xfoo'.
+            # is a substring of another: 11/foo | 1/foo => bar.
             >>> preq = Prerequisite(1)
             >>> preq[(1, 'foo', 'succeeded')] = False
-            >>> preq[(1, 'xfoo', 'succeeded')] = False
-            >>> preq.set_conditional_expr("1/foo succeeded|1/xfoo succeeded")
+            >>> preq[(11, 'foo', 'succeeded')] = False
+            >>> preq.set_conditional_expr("11/foo succeeded|1/foo succeeded")
             >>> expr = preq.conditional_expression
             >>> expr.split('|')  # doctest: +NORMALIZE_WHITESPACE
-            ['bool(self._satisfied[("1", "foo", "succeeded")])',
-            'bool(self._satisfied[("1", "xfoo", "succeeded")])']
+            ['bool(self._satisfied[("11", "foo", "succeeded")])',
+            'bool(self._satisfied[("1", "foo", "succeeded")])']

@MetRonnie MetRonnie modified the milestones: 8.4.x, 8.4.1 Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is wrong :(
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants