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

Minor post-platforms fix. #3981

Merged
merged 2 commits into from
Dec 13, 2020
Merged

Minor post-platforms fix. #3981

merged 2 commits into from
Dec 13, 2020

Conversation

hjoliver
Copy link
Member

@hjoliver hjoliver commented Dec 3, 2020 •
edited
Loading

This is a small change with no associated Issue.

Found some problems while looking at #3706

  • TaskEventsManager has a method get_host_conf that used to extract config items from the deprecated task remote and job sections, defaulting to global config. It's broken because some deprecated job items get auto-upgraded to top level task settings (so they no longer have a secondary dict key), but remote items don't because they've moved to platforms (so in deprecated form they still have a secondary dict key). So I've split these into two separate functions.
  • TaskJobManager also makes a call to the above event manager method to extract batch systems config (err tailer etc.) which is now in platforms. And, the batch_sys_config dict is not actually used anywhere, so I've removed it.

@wxtim - could you cast your "platforms eye" over this to see I'm on the right track?

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).
  • No change log entry required (invisible to users).
  • No documentation update required.
  • No dependency changes.

@hjoliver hjoliver added the bug Something is wrong :( label Dec 3, 2020
@hjoliver hjoliver added this to the cylc-8.0b0 milestone Dec 3, 2020
@hjoliver hjoliver self-assigned this Dec 3, 2020
@hjoliver hjoliver mentioned this pull request Dec 3, 2020
Closed
@hjoliver hjoliver requested a review from wxtim December 4, 2020 00:22
@hjoliver
Copy link
Member Author

hjoliver commented Dec 4, 2020

(Draft until I think about tests)

@wxtim
Copy link
Member

wxtim commented Dec 4, 2020

It looks reasonable - Sorry that I missed doing this.
these look like nicely testable units, but I guess that you probably want to check that over-all functionality hasn't changed in an unexpected way so you want a functional test?

hjoliver
hjoliver commented Dec 11, 2020
View reviewed changes
cylc/flow/task_events_mgr.py
else:
try:
ret = itask.platform[key]
except (KeyError, ItemNotFoundError):
Copy link
Member Author

Choose a reason for hiding this comment

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

There's no way to get ItemNotFoundError here. And KeyError can only occur if an illegal config item is requested.

@hjoliver hjoliver requested a review from kinow December 11, 2020 00:14
@hjoliver hjoliver marked this pull request as ready for review December 11, 2020 00:14
kinow
kinow approved these changes Dec 11, 2020
View reviewed changes
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.

Tests look good to me! LGTM

tests/unit/test_task_events_mgr_2.py Show resolved Hide resolved
@kinow
Copy link
Member

kinow commented Dec 11, 2020

One functional test job failing. Strange. Could it be similar issue to that from my PR fixing unused imports??

@hjoliver
Copy link
Member Author

Damn, it passed before my final trivial commit.

@hjoliver
Copy link
Member Author

It is a flaky one. I've restarted the tests.

@kinow
Copy link
Member

kinow commented Dec 11, 2020 •
edited
Loading

It is a flaky one. I've restarted the tests.

Two jobs failing now 😄 (I think I had seen only one before)

wxtim
wxtim approved these changes Dec 11, 2020
View reviewed changes
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.

Oh dear - tests look flaky this morning

@hjoliver
Copy link
Member Author

The test failure here appears to be genuine, but is also happening on master (local testing):

tests/f/cylc-poll/04-poll-multi-hosts.t (Wstat: 0 Tests: 3 Failed: 2)
  Failed tests:  2-3

So I'll merge this and try to diagnose the problem on master.

@hjoliver hjoliver merged commit 22a19a6 into cylc:master Dec 13, 2020
@hjoliver hjoliver deleted the get-host-conf-fix branch December 13, 2020 22:26
@oliver-sanders oliver-sanders modified the milestones: cylc-8.0b0, cylc-8.0a3 Dec 14, 2020
@hjoliver hjoliver modified the milestones: cylc-8.0a3, cylc-8.0b0 Feb 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxtim wxtim wxtim approved these changes

@kinow kinow kinow approved these changes

Assignees

@hjoliver hjoliver

Labels
bug Something is wrong :(
Projects
None yet
Milestone
cylc-8.0b0
Development

Successfully merging this pull request may close these issues.
4 participants
@hjoliver @wxtim @kinow @oliver-sanders