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

Stop searching platform groups after first match. #5398

Merged
merged 2 commits into from
Mar 15, 2023

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented Mar 7, 2023

Platform selection from a platform group was missing a break in the for loop, leading to the possibility of the wrong platform being selected.

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 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 marked this pull request as draft March 7, 2023 13:51
@MetRonnie
Copy link
Member

Kicking tests

@MetRonnie MetRonnie closed this Mar 7, 2023
@MetRonnie MetRonnie reopened this Mar 7, 2023
Comment on lines 249 to 257
[[aleph, bet, alpha, beta]]

[platform groups]
[[hebrew_letters]]
platforms = alpha, beta
[[[selection]]]
method = definition order
[[aleph]]
platforms = alpha
Copy link
Member

Choose a reason for hiding this comment

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

This aleph platform group is a little confusing to me

Copy link
Member Author

Choose a reason for hiding this comment

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

Without the break the loop runs for .*_letters, aleph and hebrew_letters in that order.

When it runs over .*_letters it matches to hebrew_letters (the platform_name we inputted into the function. It then sets platform_name = aleph from .*letters list of platforms.

If we put the break in the iteration then stops. But without it the loop runs for aleph and sets platform_name=alpha.

Finally on the third iteration platform_name = alpha which does not match hebrew letters (this is OK behaviour).

The break is only needed if a platform_name set by an earlier platform group matches a later platform group. Otherwise it's just inefficient, because it keeps searching.

(The original test is checking that despite the name hebrew_letters matches .*_letters and not hebrew_letters)

@MetRonnie MetRonnie changed the base branch from master to 8.1.x March 7, 2023 17:33
@MetRonnie MetRonnie added this to the cylc-8.1.x milestone Mar 7, 2023
@MetRonnie
Copy link
Member

Ah, wrong base branch. Need to rebase onto 8.1.x

@wxtim wxtim force-pushed the fix.platform_from_group_selection_order branch from d3ce313 to b5e851a Compare March 9, 2023 09:03
@wxtim wxtim requested a review from MetRonnie March 9, 2023 09:13
@wxtim wxtim force-pushed the fix.platform_from_group_selection_order branch from b5e851a to 18d1e4b Compare March 9, 2023 09:28
@wxtim wxtim marked this pull request as ready for review March 9, 2023 09:28
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 have suggested a comment in light of your explanation, to clarify the test

Can you add a description of the bug to the PR body?

tests/unit/test_platforms_get_platform.py Show resolved Hide resolved
@MetRonnie MetRonnie added the bug Something is wrong :( label Mar 9, 2023
Comment on lines +256 to +259
[[aleph]]
# Group with same name as platform to try and
# trip up the platform selection logic after it
# has processed [[.*_letters]] below
Copy link
Member Author

@wxtim wxtim Mar 9, 2023

Choose a reason for hiding this comment

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

Yes. That's a good idea, this test is wierd and non obvious
Feel free to add the following if you like.

Suggested change
[[aleph]]
# Group with same name as platform to try and
# trip up the platform selection logic after it
# has processed [[.*_letters]] below
[[aleph]]
# Group with same name as platform to try and
# trip up the platform selection logic after it
# has processed [[.*_letters]] below
# https://github.com/cylc/cylc-flow/pull/5398#discussion_r1130692633

or

Suggested change
[[aleph]]
# Group with same name as platform to try and
# trip up the platform selection logic after it
# has processed [[.*_letters]] below
[[aleph]]
# Group with same name as platform to try and
# trip up the platform selection logic after it
# has processed [[.*_letters]] below
# https://github.com/cylc/cylc-flow/pull/5398

Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

👍

@hjoliver hjoliver merged commit fe1c8ae into cylc:8.1.x Mar 15, 2023
@MetRonnie MetRonnie modified the milestones: cylc-8.1.x, cylc-8.1.3 Mar 16, 2023
@wxtim wxtim deleted the fix.platform_from_group_selection_order branch March 16, 2023 10:43
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