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

Queue warning #3539

Merged
merged 2 commits into from
Mar 30, 2020
Merged

Queue warning #3539

merged 2 commits into from
Mar 30, 2020

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Mar 29, 2020

These changes partially address #3470 (close after PR port to 7.9.x and master as well)

Requirements 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).
  • Appropriate tests are included (unit and/or functional).

  • Appropriate change log entry included.

  • No documentation update required. (No significant change to users, just fewer warnings)

@hjoliver hjoliver added the bug Something is wrong :( label Mar 29, 2020
@hjoliver hjoliver added this to the cylc-7.8.5 milestone Mar 29, 2020
@hjoliver hjoliver self-assigned this Mar 29, 2020
@hjoliver
Copy link
Member Author

[scheduling]
   [[queues]]
       [[[q1]]]
           members = A, B
       [[[q2]]]
           members = x
   [[dependencies]]
       graph = "x => y"
[runtime]
   [[A]]
   [[B]]
   [[x]]
       inherit = A, B
   [[y]]

On 7.8.x:

cylc-flow-7.8.x$ cylc val foo
WARNING - Queue configuration warnings:
        + q1: ignoring x from B (already assigned to a queue)  <--- SPURIOUS
        + q2: ignoring x (already assigned to a queue)  <--- CORRECT
Valid for cylc-7.8.4-21-gb766b-dirty

On this PR branch:

cylc-flow-7.8.x$ cylc val foo
WARNING - Queue configuration warnings:
        + q2: ignoring x (already assigned to a queue)  <--- CORRECT
Valid for cylc-7.8.4-22-g9db758

@hjoliver hjoliver force-pushed the queue-warning branch 2 times, most recently from 78b541e to e5d0d98 Compare March 30, 2020 00:11
@kinow
Copy link
Member

kinow commented Mar 30, 2020

Conflicts in CHANGES.md @hjoliver

@hjoliver
Copy link
Member Author

Conflicts in CHANGES.md @hjoliver

(just rebased)

@hjoliver
Copy link
Member Author

I'll replace the new functional test with a unit test, on the master version of this (once this is approved).

@hjoliver hjoliver requested review from kinow and wxtim March 30, 2020 00:19
@hjoliver
Copy link
Member Author

hjoliver commented Mar 30, 2020

Note as a follow-on to this change I could change the existing behaviour (as marked <---- CORRECT in the example above) to arguably more standard override instead of warn and ignore. i.e. if a task gets assigned to two queues, the second assignment sticks, not the first. However, I'm unsure if anyone is relying on the current behaviour - what do you think @oliver-sanders ?

Note this is discussed in #2701 along with the possibility of allowing tasks to belong to multiple queues at once, but I don't think we figured out a sensible way of doing that.

Copy link
Member

@kinow kinow left a comment

Choose a reason for hiding this comment

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

Code looks good. Functional tests passed 2 out of 4 jobs. Approving assuming the other two (and flakytests) pass (with or without some kicking 🥋 )

for msg in warnings:
err_msg += "\n+ %s" % msg
LOG.warning(err_msg)

Copy link
Member Author

@hjoliver hjoliver Mar 30, 2020

Choose a reason for hiding this comment

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

(This block was wrongly inside the outermost loop, resulting in repeated warnings in some cases)

This was referenced Mar 30, 2020
@oliver-sanders oliver-sanders merged commit 9d86192 into cylc:7.8.x Mar 30, 2020
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.

3 participants