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.

Sign up for GitHub

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

Jump to bottom

Ensure that platform from group selection checks broadcast manager #6330

Merged
merged 2 commits into from
Sep 10, 2024
Merged

Ensure that platform from group selection checks broadcast manager #6330

merged 2 commits into from
Sep 10, 2024

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Aug 27, 2024 •
edited
Loading

Fixes #6320

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.

@wxtim wxtim requested a review from oliver-sanders August 27, 2024 13:28
@wxtim wxtim added this to the 8.3.4 milestone Aug 27, 2024
@wxtim wxtim self-assigned this Aug 27, 2024
@wxtim wxtim added the bug Something is wrong :( label Aug 27, 2024
wxtim
wxtim commented Aug 27, 2024
View reviewed changes
cylc/flow/subprocpool.py Outdated Show resolved Hide resolved
@wxtim wxtim marked this pull request as draft August 27, 2024 13:50
@wxtim wxtim force-pushed the fix.6320 branch 2 times, most recently from b0261bd to bde2903 Compare August 27, 2024 14:00
@oliver-sanders oliver-sanders removed their request for review August 27, 2024 14:04
@oliver-sanders
Copy link
Member

(unsubscribing for now)

@wxtim
Ensure that platform from group selection checks broadcast manager
2f6caf6 
for updates to task config.
Ensure that 255 callback gives a sensible platform in warning messages.
@wxtim wxtim marked this pull request as ready for review August 29, 2024 08:59
@wxtim wxtim mentioned this pull request Aug 29, 2024
Closed
oliver-sanders
oliver-sanders requested changes Aug 29, 2024
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

LGTM, but there's an edge case that needs handling.

cylc/flow/task_job_mgr.py Outdated Show resolved Hide resolved
@wxtim wxtim requested a review from oliver-sanders August 29, 2024 15:03
@wxtim wxtim requested a review from oliver-sanders August 30, 2024 09:07
oliver-sanders
oliver-sanders approved these changes Sep 3, 2024
View reviewed changes
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

I have tested this with the example in the OP as well as with the Cylc 7 - style variant (#6320 (comment)) to ensure that it works fine with old-style host selection.

All good 👍.

oliver-sanders added a commit to oliver-sanders/cylc-flow that referenced this pull request Sep 3, 2024
@oliver-sanders
task job mgr: use broadcast_mgr.get_updated_rtconfig interface
17e5a6d 
* Spotted whilst reviewing cylc#6330
* Replace duplicated logic with a centralised implementation.
@oliver-sanders oliver-sanders mentioned this pull request Sep 3, 2024
Merged
8 tasks
@wxtim wxtim linked an issue Sep 5, 2024 that may be closed by this pull request
platforms: broadcasted platform ignored after ssh failure #6320
Closed
@MetRonnie MetRonnie merged commit ddddfcd into cylc:8.3.x Sep 10, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MetRonnie MetRonnie MetRonnie approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@wxtim wxtim

Labels
bug Something is wrong :(
Projects
None yet
Milestone
8.3.4
Development

Successfully merging this pull request may close these issues.

platforms: broadcasted platform ignored after ssh failure
3 participants
@wxtim @oliver-sanders @MetRonnie