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

Fixed unhelpful upgrade message for [scheduling][dependencies]graph #5874

Merged
merged 2 commits into from
Mar 14, 2024

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Dec 13, 2023

… foo=>bar

Closes #5810

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 improves documentation, doesn't change functioning.
  • 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.

@wxtim wxtim self-assigned this Dec 13, 2023
@wxtim wxtim marked this pull request as draft January 2, 2024 10:01
@wxtim wxtim marked this pull request as ready for review January 2, 2024 15:53
@wxtim wxtim requested a review from MetRonnie January 15, 2024 15:58
@wxtim wxtim force-pushed the fix.unhelpful_upgrade_message branch from a0b0772 to 1f58728 Compare January 17, 2024 10:47
@MetRonnie
Copy link
Member

Something's gone wrong git-wise

@wxtim wxtim force-pushed the fix.unhelpful_upgrade_message branch from cb78b5f to 9d13c1c Compare February 20, 2024 15:02
@wxtim wxtim force-pushed the fix.unhelpful_upgrade_message branch from 9d13c1c to 06700e7 Compare February 20, 2024 15:03
@wxtim
Copy link
Member Author

wxtim commented Feb 20, 2024

Something's gone wrong git-wise

Rebased ... poorly

indiana-jones-he-chose-poorly

Copy link
Member

Choose a reason for hiding this comment

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

These validation tests are getting quite fragmented. We now have

  • tests/integration/test_config.py
  • tests/integration/scripts/test_validate_integration.py
  • tests/integration/validate/test_outputs.py
  • tests/integration/test_upgrade_warning.py

All of which do pretty much the same kind of tests

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll have a look and see if I can do any rationalization in this PR, but I think that there is definately scope for keeping tight reigns on the integration tests, lest they become a sprawing mess.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've moved my new tests so that they aren't at least creating a new test file, and only modifying one test file. I think further rationalization is out of scope.

Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

[scheduling]
    initial cycle point = 2012
    [[dependencies]]
        graph = foo
        [[[T00]]]
            graph = bar
[runtime]
    [[foo,bar]]
$ cylc validate cylc7-simple/
WARNING - deprecated graph items were automatically upgraded in "workflow definition":
     * (8.0.0) [scheduling][dependencies]graph -> [scheduling][graph]R1

It's missing the T00 upgrade from the warning

@MetRonnie MetRonnie changed the title Fixed unhelpful upgrade message for [scheduling][dependencies]graph =… Fixed unhelpful upgrade message for [scheduling][dependencies]graph Feb 20, 2024
@wxtim wxtim modified the milestones: cylc-8.2.5, cylc-8.3.0 Feb 21, 2024
@wxtim
Copy link
Member Author

wxtim commented Feb 21, 2024

[scheduling]
    initial cycle point = 2012
    [[dependencies]]
        graph = foo
        [[[T00]]]
            graph = bar
[runtime]
    [[foo,bar]]
$ cylc validate cylc7-simple/
WARNING - deprecated graph items were automatically upgraded in "workflow definition":
     * (8.0.0) [scheduling][dependencies]graph -> [scheduling][graph]R1

It's missing the T00 upgrade from the warning

Somewhat surprised that's valid at all!

Fixed with 2d868cf

Deal with scenario where
```
[scheduling]
  graph = foo
  [[P1D]]
    graph = bar
```
Copy link
Member

@MetRonnie MetRonnie 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 this will do

@wxtim
Copy link
Member Author

wxtim commented Feb 22, 2024

I think this will do

Don't get me wrong, it's wierd and complex now. I'm not thrilled. I also cannot justify spending too much time on code which I want to delete before the end of 2026.

@hjoliver hjoliver modified the milestones: cylc-8.3.0, cylc-8.4.0 Mar 13, 2024
@MetRonnie MetRonnie modified the milestones: cylc-8.4.0, cylc-8.3.0 Mar 14, 2024
@MetRonnie
Copy link
Member

Kicking tests

@MetRonnie MetRonnie closed this Mar 14, 2024
@MetRonnie MetRonnie reopened this Mar 14, 2024
@MetRonnie MetRonnie merged commit ae5709d into cylc:master Mar 14, 2024
45 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unhelpful Upgrade Warning
4 participants