Fix.platform is regex remote tidy fail #5445

wxtim · 2023-03-31T08:46:51Z

Closes #5429

@dpmatthews tagged as reviewer as reporter of original issue.

To test manually see #5429 .

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.

tests/integration/test_task_remote_mgr.py

cylc/flow/platforms.py

cylc/flow/task_remote_mgr.py

tests/integration/test_task_remote_mgr.py

oliver-sanders

LGTM, couple of minor points.

cylc/flow/task_remote_mgr.py

oliver-sanders · 2023-04-03T10:54:53Z

Gotta small lint failure.

…ote tidy Remove broken unit test tests for map_platforms_used_for_install_targets unit test select_platforms_used Test remote_tidy Changelog f

Co-authored-by: Oliver Sanders <[email protected]>

and replaced it by adding a "quiet" mode to get_install_targets_platform_map. Added test for the new quiet option. Factored some logic from remote_tidy to a staticmethod of TaskRemoteMgr to allow unit testing. Added unit test. monkeypatch test to avoid actual subproc calls

cylc/flow/exceptions.py

cylc/flow/task_remote_mgr.py

oliver-sanders

Tested with this workflow:

[scheduling] 
    [[graph]]           
        R1 = x => y     
        
[runtime]               
    [[x]]               
        platform = myhost01       
        script = cylc stop "${CYLC_WORKFLOW_ID}" --now --now    
    [[y]]

and this global config:

[platforms]
  [[myhost0.]]

Where "myhost01" exists on my network (obviously edit this to match an actual host).

Before:

INFO - Workflow shutting down - REQUEST(NOW-NOW)
WARNING - Orphaned tasks:
    * 1/x (running)
ERROR - Error during shutdown
ERROR - local variable 'platform' referenced before assignment
    Traceback (most recent call last):
      File "~/cylc-flow/cylc/flow/scheduler.py", line 1756, in shutdown
        await self._shutdown(reason)
      File "~/cylc-flow/cylc/flow/scheduler.py", line 1834, in _shutdown
        self.task_job_mgr.task_remote_mgr.remote_tidy()
      File "~/cylc-flow/cylc/flow/task_remote_mgr.py", line 312, in remote_tidy
        platform['name'],
    UnboundLocalError: local variable 'platform' referenced before assignment
INFO - DONE

After:

INFO - Workflow shutting down - REQUEST(NOW-NOW)
WARNING - Orphaned tasks:
    * 1/x (running)
INFO - platform: exvcylcdev01 - remote tidy (on exvcylcdev01)
INFO - DONE

cylc/flow/exceptions.py

cylc/flow/rundb.py

cylc/flow/task_remote_mgr.py

MetRonnie · 2023-04-13T15:17:55Z

cylc/flow/task_remote_mgr.py

+        platforms_used = (
+            self.db_mgr.get_pri_dao().select_platforms_used())


I think we should ensure the DB is closed

Suggested change

platforms_used = (

self.db_mgr.get_pri_dao().select_platforms_used())

with self.db_mgr.get_pri_dao() as dao:

platforms_used = dao.select_platforms_used()

tests/unit/test_platforms.py

tests/unit/test_rundb.py

tests/unit/test_task_remote_mgr.py

Co-authored-by: Oliver Sanders <[email protected]>

Co-authored-by: Ronnie Dutta <[email protected]>

…gex_remote_tidy_fail * upstream/8.1.x: Update cylc/flow/scripts/message.py Upload coverage to Codecov in separate job (cylc#5459) upgrade cylc message internal help with details of severity levels Update tests/functional/platforms/10-do-not-host-check-platforms.t Fix flake8-comprehensions C419 Don't use any([i for i in iterable]) use any(i for i in iterable). It's more efficient because we don't have to expand the entire thing. Improve readability of host/platform eval code (#53) small changlog error fix update comment on localhost check and add test for case localhost4.localhost42 undo mistake clarification of nomenclature Avoid running host check on platform names - this doesn't make any sense.

cylc/flow/task_remote_mgr.py

MetRonnie · 2023-04-17T16:03:27Z

cylc/flow/task_remote_mgr.py

+        platforms_used = (
+            self.db_mgr.get_pri_dao().select_task_job_platforms())


Suggested change

platforms_used = (

self.db_mgr.get_pri_dao().select_task_job_platforms())

with self.db_mgr.get_pri_dao() as dao:

platforms_used = dao.select_task_job_platforms())

cylc/flow/task_remote_mgr.py

MetRonnie

Tested out using a regex pattern platform section in global.cylc

wxtim · 2023-04-19T08:17:43Z

@oliver-sanders - Ronnie's had me make some changes since last you looked.. mind giving it another quick look?

oliver-sanders · 2023-04-19T12:12:42Z

@wxtim, some minor conflicts with the other branch.

oliver-sanders · 2023-04-19T13:22:52Z

Gotta couple of test failures :(

wxtim · 2023-04-19T14:50:48Z

Gotta couple of test failures :(

I think that the one in tests/f/rnd is a random flake, but the other one exists on 8.1.x, so it's probably a mistake in one of my other PR's :(

oliver-sanders · 2023-04-20T13:32:11Z

I've re-run the f/rnd test and it's failed again (so it's failed four times in a row due to retries) so it looks like it's genuine.

cylc/flow/exceptions.py

cylc/flow/task_job_mgr.py

Co-authored-by: Ronnie Dutta <[email protected]>

wxtim · 2023-04-20T15:01:14Z

I've re-run the f/rnd test and it's failed again (so it's failed four times in a row due to retries) so it looks like it's genuine.

Have you looked at what's in it?

oliver-sanders · 2023-04-21T08:59:58Z

Merging with an unrelated test failure fixed in #5479

…too_soon * upstream/8.1.x: Fix.platform is regex remote tidy fail (cylc#5445) parsec: better error messages for section/setting mixups cat-log: change interface for scheduler logs to include the dirname cat-log: support other workflow files Fix workflow shutdown on no platforms error (cylc#5395) actions: add build test

…too_soon * upstream/8.1.x: (46 commits) Bump dev version (cylc#5518) Prepare release 8.1.4 cat-log: fix issues with file listing Update changelog Fix `eval_host()` localhost bug & plug testing gap Bump dev version (cylc#5501) Prepare release 8.1.3 Ignore off-sequence parents, for datastore. (cylc#5495) Avoid duplicate prerequisites from multiple recurrences. (cylc#5466) tests/f: fix events/11 swarm: convert cylc-dev Docker image to run on ubuntu:latest fix changelog Apply prerequisite changes to spawned tasks, on reload & restart (cylc#5334) cat-log: don't list directories which don't exist (cylc#5488) task_job_mgr: move compute out of open call manylinux tests: update to ubuntu-20.04 Update changelog Address review Fix.platform is regex remote tidy fail (cylc#5445) parsec: better error messages for section/setting mixups ...

wxtim requested review from dpmatthews and oliver-sanders March 31, 2023 08:47

wxtim self-assigned this Mar 31, 2023

wxtim added this to the cylc-8.1.3 milestone Mar 31, 2023

wxtim force-pushed the fix.platform_is_regex_remote_tidy_fail branch 3 times, most recently from adfa520 to 0aee582 Compare March 31, 2023 08:58

wxtim commented Mar 31, 2023

View reviewed changes

tests/integration/test_task_remote_mgr.py Show resolved Hide resolved

oliver-sanders reviewed Mar 31, 2023

View reviewed changes

oliver-sanders reviewed Apr 3, 2023

View reviewed changes

wxtim and others added 4 commits April 11, 2023 11:04

Create a mapping between platforms and install_targets for use by rem…

53a10ed

…ote tidy Remove broken unit test tests for map_platforms_used_for_install_targets unit test select_platforms_used Test remote_tidy Changelog f

Update tests/integration/test_task_remote_mgr.py

af7d22e

Co-authored-by: Oliver Sanders <[email protected]>

Response to review

dd2a373

wxtim force-pushed the fix.platform_is_regex_remote_tidy_fail branch from d56e9dd to dd2a373 Compare April 11, 2023 10:53

wxtim requested review from oliver-sanders and MetRonnie and removed request for dpmatthews April 11, 2023 10:54

fix test

75a08f3

oliver-sanders reviewed Apr 11, 2023

View reviewed changes

cylc/flow/exceptions.py Outdated Show resolved Hide resolved

oliver-sanders reviewed Apr 11, 2023

View reviewed changes

cylc/flow/task_remote_mgr.py Outdated Show resolved Hide resolved

oliver-sanders approved these changes Apr 11, 2023

View reviewed changes

This was linked to issues Apr 12, 2023

Remote Tidy: Cannot get platform from install target if platform config is regex #5429

Closed

remote-tidy: don't block shutdown if a host cannot be contacted #5181

Closed

MetRonnie reviewed Apr 13, 2023

View reviewed changes

wxtim and others added 3 commits April 17, 2023 11:38

response to review

488bc58

Update cylc/flow/exceptions.py

35efbca

Co-authored-by: Oliver Sanders <[email protected]>

Apply suggestions from code review

7a9da9f

Co-authored-by: Ronnie Dutta <[email protected]>

wxtim force-pushed the fix.platform_is_regex_remote_tidy_fail branch from d1ff32e to 7a9da9f Compare April 17, 2023 10:41

wxtim added 2 commits April 17, 2023 11:44

f

7629ea6

wxtim requested a review from MetRonnie April 17, 2023 10:54

fixed some logic

812da58

MetRonnie reviewed Apr 17, 2023

View reviewed changes

review response

94a61da

wxtim force-pushed the fix.platform_is_regex_remote_tidy_fail branch from e43c13f to 94a61da Compare April 18, 2023 08:55

wxtim requested a review from MetRonnie April 18, 2023 08:55

MetRonnie reviewed Apr 18, 2023

View reviewed changes

cylc/flow/task_remote_mgr.py Outdated Show resolved Hide resolved

MetRonnie approved these changes Apr 18, 2023

View reviewed changes

wxtim added 2 commits April 19, 2023 13:21

Merge branch '8.1.x' into fix.platform_is_regex_remote_tidy_fail

d99d688

Fix flake 8 failure

ec9a531

fixup test logic

7e674d0

MetRonnie reviewed Apr 20, 2023

View reviewed changes

cylc/flow/exceptions.py Outdated Show resolved Hide resolved

cylc/flow/task_job_mgr.py Outdated Show resolved Hide resolved

wxtim and others added 2 commits April 20, 2023 16:00

Update cylc/flow/exceptions.py

4a86033

Co-authored-by: Ronnie Dutta <[email protected]>

Update cylc/flow/task_job_mgr.py

cda7a57

Co-authored-by: Ronnie Dutta <[email protected]>

oliver-sanders merged commit 62f5ef5 into cylc:8.1.x Apr 21, 2023

oliver-sanders mentioned this pull request Apr 21, 2023

Remote Tidy: Cannot get platform from install target if platform config is regex #5429

Closed

wxtim deleted the fix.platform_is_regex_remote_tidy_fail branch April 21, 2023 09:03

oliver-sanders mentioned this pull request Apr 24, 2023

remote-tidy: don't block shutdown if a host cannot be contacted #5181

Closed

MetRonnie mentioned this pull request May 3, 2023

actions: test up to python 3.11 #5497

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix.platform is regex remote tidy fail #5445

Fix.platform is regex remote tidy fail #5445

wxtim commented Mar 31, 2023 •

edited

Loading

oliver-sanders left a comment

oliver-sanders commented Apr 3, 2023

oliver-sanders left a comment

MetRonnie Apr 13, 2023

MetRonnie Apr 17, 2023

MetRonnie left a comment

wxtim commented Apr 19, 2023

oliver-sanders commented Apr 19, 2023

oliver-sanders commented Apr 19, 2023

wxtim commented Apr 19, 2023

oliver-sanders commented Apr 20, 2023

wxtim commented Apr 20, 2023

oliver-sanders commented Apr 21, 2023

		platforms_used = (
		self.db_mgr.get_pri_dao().select_platforms_used())

		platforms_used = (
		self.db_mgr.get_pri_dao().select_task_job_platforms())

Fix.platform is regex remote tidy fail #5445

Fix.platform is regex remote tidy fail #5445

Conversation

wxtim commented Mar 31, 2023 • edited Loading

oliver-sanders left a comment

Choose a reason for hiding this comment

oliver-sanders commented Apr 3, 2023

oliver-sanders left a comment

Choose a reason for hiding this comment

MetRonnie Apr 13, 2023

Choose a reason for hiding this comment

MetRonnie Apr 17, 2023

Choose a reason for hiding this comment

MetRonnie left a comment

Choose a reason for hiding this comment

wxtim commented Apr 19, 2023

oliver-sanders commented Apr 19, 2023

oliver-sanders commented Apr 19, 2023

wxtim commented Apr 19, 2023

oliver-sanders commented Apr 20, 2023

wxtim commented Apr 20, 2023

oliver-sanders commented Apr 21, 2023

wxtim commented Mar 31, 2023 •

edited

Loading