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 output consistency check when there are offsets. #6526

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Dec 16, 2024

Close #6523

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 Dec 16, 2024
@hjoliver hjoliver added this to the 8.4.0 milestone Dec 16, 2024
@hjoliver hjoliver requested a review from wxtim December 16, 2024 03:05
@hjoliver hjoliver self-assigned this Dec 16, 2024
@hjoliver hjoliver force-pushed the fix-graph-offset-opt branch 3 times, most recently from 0cb877a to 811699f Compare December 16, 2024 03:35
Comment on lines -951 to +982
# Don't check the left hand side if this has a non-terminal RHS:
param((('a &', 'b[-P42M]'), {}), None),
# Check the left hand side if this has a non-terminal RHS:
param((('a &', 'b[-P42M]'), {}), 'Null task name in graph'),
Copy link
Member Author

Choose a reason for hiding this comment

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

a & => b[-P25] => b

On master this validates OK and the test is confirming that? 🤯

Copy link
Member

@wxtim wxtim Dec 16, 2024

Choose a reason for hiding this comment

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

Fixed against master with this change hjoliver#63: I've failed to come up with other bad scenarios, but this is bad enough.

@hjoliver hjoliver force-pushed the fix-graph-offset-opt branch from 811699f to 691f6f5 Compare December 16, 2024 03:41
@hjoliver hjoliver force-pushed the fix-graph-offset-opt branch from 691f6f5 to b0ec142 Compare December 16, 2024 04:32
┆ (
✓ ┆ started
⨯ ┆ and succeeded
┆ )
Copy link
Member Author

Choose a reason for hiding this comment

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

This is now consistent with the completion condition if no offset is present.

@hjoliver hjoliver force-pushed the fix-graph-offset-opt branch from f3592ef to 697f923 Compare December 16, 2024 04:51
# The word "optional" is common to both. Not ideal!
'optional',
id='succeed-or-failed-implies-success-optional'
),
Copy link
Member Author

Choose a reason for hiding this comment

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

Took your test cases with attribution (via commit message) @wxtim

@hjoliver hjoliver marked this pull request as ready for review December 16, 2024 05:13
Copy link
Member

@wxtim wxtim left a comment

Choose a reason for hiding this comment

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

I think we need to double check the test I've queried, but otherwise this seems to fix the bug in question.

I have a related issue where some code I did early in the year hasn't caused a bug, but I'm unclear whether it's correct. 😢

@@ -102,8 +102,8 @@ async def test_over_bracketed(flow, scheduler, reftest):
'final cycle point': '2013-12-25T12:00Z',
'graph': {
'T12': '''
(a[-P1D]:fail | b[-P1D]:fail | c[-P1D]:fail) => d
a & b & c # Implied by implicit cycling now...
(a[-P1D]:fail? | b[-P1D]:fail? | c[-P1D]:fail?) => d
Copy link
Member

Choose a reason for hiding this comment

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

This is absolutely reasobable in the case of recurring cycling, but is it reasonable in general?

i.e. I think that this should fail validation

[graph]
    R1/1 = a
    R1/2 = a[-P1]:fail

but I'm not sure that this should, because a at -P1 is not the same task as a.

R1 = a[-P1]:fail => a

I think I may be happy to gloss over this, because the latter recurrance isn't very meaningful.

suicide = (suicide_char == self.__class__.SUICIDE)
optional = (opt_char == self.__class__.OPTIONAL)
if output:
output = output.strip(self.__class__.QUALIFIER)

if name in self.family_map:
fam = True
mems = self.family_map[name]
rhs_mems = self.family_map[name]
Copy link
Member

Choose a reason for hiding this comment

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

rhs_members for easier reading please?

),
param(
"foo[-P1]:succeeded? | foo[-P1]:failed? => foo",
# Order of processing can result in different error messages
Copy link
Member

Choose a reason for hiding this comment

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

I fixed this in my version of the pr hjoliver#62

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.

Graph: Inter-cycle dependencies not correctly checked
2 participants