Prevent cylc clean from removing workflow base dirs containing run dirs/named runs #4289

MetRonnie · 2021-07-02T16:03:12Z

These changes close #4163 (assuming we're happy not having an --all option and will instead wait until universal ID is implemented, allowing cylc clean foo/*)

Explanation

If you have

~/cylc-run
`-- foo
    `-- run1
        |-- flow.cylc
            etc

Up til now, doing cylc clean foo would just delete ~/cylc-run/foo, and if you had any symlink dirs or remote installations, they wouldn't get cleaned.

With this PR, it now raises an error if you try to clean a dir containing multiple (>1) runs - you have to explicitly type out the run dir. However:

If the dir contains 1 run dir, it will clean it (cleans foo/run1 followed by deleting foo)
There is now the option --force to allow possibly incomplete cleaning of the specified dir (e.g. it will just delete the foo dir)

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).
Applied any dependency changes to both setup.py and
conda-environment.yml.
Appropriate tests are included (unit).
Appropriate change log entry included.
Documentation will be handled by Document cylc clean cylc-doc#256

… a workflow All workflow run dirs contain a log/workflow/log dir, which was getting erroneously identified as a workflow. Solution was to exit from the generator if the scan dir itself is a workflow run dir.

cylc/flow/workflow_files.py

MetRonnie · 2021-07-02T16:08:42Z

tests/integration/test_scan.py

-@pytest.fixture(scope='session')
-def badly_messed_up_run_dir():
-    tmp_path = Path(TemporaryDirectory().name)
-    tmp_path.mkdir()
+@pytest.fixture


This fixture only used once so I've made it function-scoped to allow monkeypatching

wxtim

Read, all makes sense.
Tried it out → Working.

cylc/flow/network/scan.py

cylc/flow/workflow_files.py

oliver-sanders · 2021-07-05T15:54:58Z

cylc/flow/workflow_files.py

-            f"Cannot remove running workflow.\n\n{exc}")
+        raise ServiceFileError(f"Cannot remove running workflow.\n\n{exc}")
+    # Check dir does not contain other workflows:
+    contained_workflows = asyncio.run(get_contained_workflows(run_dir))


I think this is ok (if it wasn't I would expect all the tests to fail), however, could cause bugs later on.

The convenience function asyncio.run function creates a new event loop and closes it, I think that can cause issues when other event loops are kicking about.

This function cannot be called when another asyncio event loop is running in the same thread.

If debug is True, the event loop will be run in debug mode.

This function always creates a new event loop and closes it at the end. It should be used as a main entry point for asyncio programs, and should ideally only be called once.

-- https://docs.python.org/3/library/asyncio-task.html

I think the alternative here would be the totally user-friendly asyncio.get_event_loop().run_until_complete(coro())

I was a bit worried about that

oliver-sanders · 2021-07-28T09:58:17Z

Should cylc clean ignore the runN directory or will that come with #4285 (or future work)?

$ cylc install two
$ cylc clean two
...
contains the following workflow(s):
    - two/run1
    - two/runN

MetRonnie · 2021-07-28T13:29:54Z

Marking as draft while investigating the runN symlink being detected as a separate workflow.

(As discussed on Teams) cylc clean will not use the inferred run number functionality. Edit: however, if running cylc clean foo and foo only has 1 run, I expect we should clean foo/run1 and remove the foo directory too

- Increase scan depth for cylc clean when it checks that the dir doesn't contain other workflows - Try to safen up use of asyncio

cylc/flow/pathutil.py

wxtim · 2021-08-10T10:07:05Z

cylc/flow/workflow_files.py

+            f"{bullet}{bullet.join(contained_workflows)}"
+        )
+        if not opts.force:
+            raise WorkflowFilesError(f"Cannot clean - {msg}")


Might be worth adding a note to the end of the message to the effect that the user can use --force if that is what they genuinely want to do...

cylc/flow/workflow_files.py

wxtim

Scan read changes since last time.
Played with result branch, run some examples.
Had an in-depth play with test_init_clean__multiple_runs. Assured myself that Codecov is wrong in stating some logic not tested.
Had an in-depth play with test_remove_empty_parents

MetRonnie · 2021-08-12T12:05:16Z

Merging as 2 approvals

MetRonnie added 4 commits July 2, 2021 13:54

Prevent network.scan.scan() from thinking a workflow run dir contains…

773706b

… a workflow All workflow run dirs contain a log/workflow/log dir, which was getting erroneously identified as a workflow. Solution was to exit from the generator if the scan dir itself is a workflow run dir.

Type annotations

fc8fce7

cylc clean: prevent cleaning of workflow dirs containing named runs

b8f5bac

cylc clean: add --force option to clean workflow base dirs

19f7ce8

MetRonnie added this to the cylc-8.0b2 milestone Jul 2, 2021

MetRonnie requested review from oliver-sanders and wxtim July 2, 2021 16:03

MetRonnie self-assigned this Jul 2, 2021

This comment has been minimized.

Sign in to view

MetRonnie commented Jul 2, 2021

View reviewed changes

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

MetRonnie commented Jul 2, 2021

View reviewed changes

wxtim approved these changes Jul 5, 2021

View reviewed changes

Merge branch 'master' into cylc-clean-base-dirs

a26c274

oliver-sanders reviewed Jul 21, 2021

View reviewed changes

oliver-sanders modified the milestones: cylc-8.0b2, cylc-8.0b3 Jul 28, 2021

MetRonnie marked this pull request as draft July 28, 2021 13:27

Address code review

22775b4

- Increase scan depth for cylc clean when it checks that the dir doesn't contain other workflows - Try to safen up use of asyncio

MetRonnie force-pushed the cylc-clean-base-dirs branch from e8ac170 to 9e20d8d Compare August 4, 2021 16:11

MetRonnie added 5 commits August 4, 2021 17:12

Merge branch 'master' into cylc-clean-base-dirs

3ebde57

Update changelog

a1632ea

Stop network.scan.scan() from showing runN in first listing

164726f

Tidy

edcf529

Allow cylc clean to proceed if dir contains 1 run dir

8060cc2

MetRonnie force-pushed the cylc-clean-base-dirs branch from 9e20d8d to 8060cc2 Compare August 4, 2021 16:17

MetRonnie marked this pull request as ready for review August 4, 2021 16:19

MetRonnie requested review from wxtim and oliver-sanders August 4, 2021 16:19

oliver-sanders approved these changes Aug 10, 2021

View reviewed changes

cylc/flow/pathutil.py Show resolved Hide resolved

wxtim reviewed Aug 10, 2021

View reviewed changes

cylc/flow/workflow_files.py Show resolved Hide resolved

MetRonnie mentioned this pull request Aug 10, 2021

Infer run number & centralised reg parse #4285

Merged

7 tasks

wxtim reviewed Aug 10, 2021

View reviewed changes

MetRonnie merged commit 4686ba6 into cylc:master Aug 12, 2021

MetRonnie deleted the cylc-clean-base-dirs branch August 12, 2021 12:05

MetRonnie mentioned this pull request Aug 18, 2021

Attempt to fix unit test flakiness on Python 3.7 #4364

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent cylc clean from removing workflow base dirs containing run dirs/named runs #4289

Prevent cylc clean from removing workflow base dirs containing run dirs/named runs #4289

MetRonnie commented Jul 2, 2021 •

edited

Loading

This comment has been minimized.

MetRonnie Jul 2, 2021 •

edited

Loading

wxtim left a comment

oliver-sanders Jul 5, 2021

MetRonnie Jul 21, 2021

oliver-sanders commented Jul 28, 2021

MetRonnie commented Jul 28, 2021 •

edited

Loading

wxtim Aug 10, 2021

wxtim left a comment •

edited

Loading

MetRonnie commented Aug 12, 2021

Prevent cylc clean from removing workflow base dirs containing run dirs/named runs #4289

Prevent cylc clean from removing workflow base dirs containing run dirs/named runs #4289

Conversation

MetRonnie commented Jul 2, 2021 • edited Loading

This comment has been minimized.

MetRonnie Jul 2, 2021 • edited Loading

Choose a reason for hiding this comment

wxtim left a comment

Choose a reason for hiding this comment

oliver-sanders Jul 5, 2021

Choose a reason for hiding this comment

MetRonnie Jul 21, 2021

Choose a reason for hiding this comment

oliver-sanders commented Jul 28, 2021

MetRonnie commented Jul 28, 2021 • edited Loading

wxtim Aug 10, 2021

Choose a reason for hiding this comment

wxtim left a comment • edited Loading

Choose a reason for hiding this comment

MetRonnie commented Aug 12, 2021

MetRonnie commented Jul 2, 2021 •

edited

Loading

MetRonnie Jul 2, 2021 •

edited

Loading

MetRonnie commented Jul 28, 2021 •

edited

Loading

wxtim left a comment •

edited

Loading