Add Cylc Clean Support #323

wxtim · 2022-02-24T16:50:12Z

These changes close #299

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).
Appropriate change log entry included.
No documentation update required.
No dependency changes.

oliver-sanders · 2022-02-25T10:43:50Z

For the pool to be able to manage all operations centrally it should be a property of the uiserver (i.e. self.proc_pool = ProcessPoolExecutor(max_workers=None)) rather than created for each command (with ProcessPoolExecutor...).

There's a stop_extension method in the UIServer which can be used to close the pool on shutdown.

wxtim · 2022-02-25T11:15:28Z

For the pool to be able to manage all operations centrally it should be a property of the uiserver (i.e. self.proc_pool = ProcessPoolExecutor(max_workers=None)) rather than created for each command (with ProcessPoolExecutor...).

There's a stop_extension method in the UIServer which can be used to close the pool on shutdown.

That makes more sense than what I was trying to do.

oliver-sanders · 2022-02-28T15:56:57Z

Executor is looking good.

codecov-commenter · 2022-03-02T15:29:20Z

Codecov Report

Merging #323 (3299e00) into master (dcb91cc) will increase coverage by 0.59%.
The diff coverage is 66.26%.

@@            Coverage Diff             @@
##           master     #323      +/-   ##
==========================================
+ Coverage   77.76%   78.36%   +0.59%     
==========================================
  Files          10       11       +1     
  Lines        1048     1137      +89     
  Branches      202      217      +15     
==========================================
+ Hits          815      891      +76     
- Misses        196      207      +11     
- Partials       37       39       +2

Impacted Files	Coverage Δ
cylc/uiserver/resolvers.py	`54.10% <59.09%> (+12.35%)`	⬆️
cylc/uiserver/app.py	`84.48% <75.00%> (-0.34%)`	⬇️
cylc/uiserver/schema.py	`85.91% <100.00%> (+3.15%)`	⬆️
cylc/uiserver/workflows_mgr.py	`81.76% <0.00%> (-0.59%)`	⬇️
cylc/uiserver/authorise.py	`88.64% <0.00%> (-0.05%)`	⬇️
cylc/uiserver/utils.py	`100.00% <0.00%> (ø)`
cylc/uiserver/data_store_mgr.py	`65.00% <0.00%> (+3.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcb91cc...3299e00. Read the comment docs.

datamel

I have manually tested this and am getting an error "'types.SimpleNamespace' object has no attribute 'remote_timeout'".

cylc/uiserver/resolvers.py

datamel

I've had this branch running today, using it to clean both remote and local workflows. I have read the code and run the tests. Working smoothly for me. Thanks @wxtim

datamel · 2022-03-14T13:46:43Z

cylc/uiserver/resolvers.py

+        except Exception as exc:
+            return cls._error(exc)
+        # Hard set remote timeout.
+        opts.remote_timeout = "600"


Very minor point but this defaults to 120 in cylc flow. Wonder if it would be worth making them consistent, for documentation purposes mainly.
I think 600 may seem like the better timeout than 120, since nfs and file deletions can perhaps take a while?

In the CLI 120 is the default, but the user can over-ride it.
In the UI I haven't exposed it to the user, so I think it's reasonable to set it longer.

Is there a reason not to expose it to the user? (Happy to leave this as is for now, but perhaps there should be a question issue for how long the timeouts should be on the CLI vs UI and whether to expose it to user on the UI)

I can't remember the reason, but I think @oliver-sanders and I agreed not to - I think it ought to be documented for discussion...

cylc/uiserver/resolvers.py

oliver-sanders

Coming back to this post RC2 release!

One small conflict.

cylc/uiserver/schema.py

cylc/uiserver/resolvers.py

wxtim · 2022-05-25T09:17:07Z

@MetRonnie Tagging you because Oliver is away.

MetRonnie · 2022-05-30T09:50:38Z

cylc/uiserver/resolvers.py

+        except Exception as exc:
+            return cls._error(exc)
+        # Hard set remote timeout.
+        opts.remote_timeout = "600"


Is there a reason not to expose it to the user? (Happy to leave this as is for now, but perhaps there should be a question issue for how long the timeouts should be on the CLI vs UI and whether to expose it to user on the UI)

cylc/uiserver/resolvers.py

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

MetRonnie · 2022-06-01T10:15:57Z

cylc/uiserver/schema.py

+
+    class Arguments:
+        workflows = graphene.List(WorkflowID, required=True)
+        rm = graphene.String(


Tested it out with rm and it failed. This should be a list of strings rather than a single string, I think

Yes - according to the documentation it can be a colon-separated list of strings. I've added something to deal.

cylc/uiserver/resolvers.py

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

stale

…o add_clean_to_ui2 * 'add_clean_to_ui2' of github.com:wxtim/cylc-uiserver: Update cylc/uiserver/resolvers.py Update cylc/uiserver/resolvers.py

oliver-sanders · 2022-06-17T10:33:40Z

LGTM: Merging with two approvals.

wxtim self-assigned this Feb 24, 2022

wxtim added this to the 1.1.0 milestone Feb 24, 2022

wxtim added the enhancement label Feb 24, 2022

wxtim requested review from oliver-sanders and datamel February 24, 2022 16:52

wxtim marked this pull request as draft February 24, 2022 16:52

wxtim marked this pull request as ready for review February 25, 2022 20:41

wxtim mentioned this pull request Mar 1, 2022

2022 Cylc Meetings cylc/cylc-admin#143

Closed

datamel reviewed Mar 3, 2022

View reviewed changes

cylc/uiserver/resolvers.py Outdated Show resolved Hide resolved

cylc/uiserver/resolvers.py Outdated Show resolved Hide resolved

wxtim requested a review from datamel March 14, 2022 09:36

datamel approved these changes Mar 14, 2022

View reviewed changes

oliver-sanders reviewed Mar 30, 2022

View reviewed changes

cylc/uiserver/schema.py Outdated Show resolved Hide resolved

cylc/uiserver/resolvers.py Outdated Show resolved Hide resolved

cylc/uiserver/resolvers.py Outdated Show resolved Hide resolved

wxtim added 14 commits March 31, 2022 14:37

WIP making cylc clean work on UI

b5b9425

refactor to common logic from play and clean into one place.

5fbcfd1

just a note

cca15e3

implement cylc clean in a blocking way

670fcd1

run clean in subprocpool

17b2b0b

added testing

9cb564d

put play function back

4023a37

keep the _build_cmd refactor

5baddf2

newline

185edf0

flake8

60355d0

Fix mypy bug

bdbc5b2

Made ProcPoolExecutor a whole UIS object

cc7ccb6

fix style

1accec8

fix tests which check for mutations

ff85529

oliver-sanders mentioned this pull request May 9, 2022

Job logs #347

Closed

12 tasks

wxtim marked this pull request as draft May 10, 2022 15:39

wxtim marked this pull request as ready for review May 11, 2022 14:28

wxtim requested a review from MetRonnie May 25, 2022 09:16

MetRonnie reviewed May 30, 2022

View reviewed changes

wxtim and others added 3 commits May 31, 2022 14:03

Update cylc/uiserver/resolvers.py

1145ede

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

Update cylc/uiserver/resolvers.py

5ae1703

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

response to review

875f2a8

wxtim requested a review from MetRonnie June 1, 2022 09:14

MetRonnie requested changes Jun 1, 2022

View reviewed changes

fix rm bug

3299e00

wxtim requested a review from MetRonnie June 1, 2022 13:18

MetRonnie reviewed Jun 1, 2022

View reviewed changes

cylc/uiserver/resolvers.py Outdated Show resolved Hide resolved

MetRonnie reviewed Jun 1, 2022

View reviewed changes

cylc/uiserver/resolvers.py Outdated Show resolved Hide resolved

wxtim and others added 2 commits June 7, 2022 09:09

Update cylc/uiserver/resolvers.py

aca76f9

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

Update cylc/uiserver/resolvers.py

d610994

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

wxtim requested a review from MetRonnie June 7, 2022 08:09

oliver-sanders removed their request for review June 13, 2022 09:20

wxtim added 3 commits June 14, 2022 15:23

merge

2c3aafd

Merge branch 'add_clean_to_ui2' of github.com:wxtim/cylc-uiserver int…

b698883

…o add_clean_to_ui2 * 'add_clean_to_ui2' of github.com:wxtim/cylc-uiserver: Update cylc/uiserver/resolvers.py Update cylc/uiserver/resolvers.py

fix change in tests

f1d3a6c

wxtim requested a review from oliver-sanders June 17, 2022 09:33

MetRonnie approved these changes Jun 17, 2022

View reviewed changes

oliver-sanders merged commit 9f011ae into cylc:master Jun 17, 2022

MetRonnie mentioned this pull request Jun 17, 2022

Clean fails silently #366

Closed

wxtim deleted the add_clean_to_ui2 branch June 20, 2022 07:45

MetRonnie mentioned this pull request Jun 22, 2022

Fix cylc clean & tidy implementation #369

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Cylc Clean Support #323

Add Cylc Clean Support #323

wxtim commented Feb 24, 2022 •

edited

Loading

oliver-sanders commented Feb 25, 2022

wxtim commented Feb 25, 2022

oliver-sanders commented Feb 28, 2022

codecov-commenter commented Mar 2, 2022 •

edited

Loading

datamel left a comment

datamel left a comment

datamel Mar 14, 2022

wxtim Mar 14, 2022

MetRonnie May 30, 2022

wxtim May 31, 2022 •

edited

Loading

oliver-sanders left a comment

wxtim commented May 25, 2022

MetRonnie May 30, 2022

MetRonnie Jun 1, 2022 •

edited

Loading

wxtim Jun 1, 2022

oliver-sanders commented Jun 17, 2022

Add Cylc Clean Support #323

Add Cylc Clean Support #323

Conversation

wxtim commented Feb 24, 2022 • edited Loading

oliver-sanders commented Feb 25, 2022

wxtim commented Feb 25, 2022

oliver-sanders commented Feb 28, 2022

codecov-commenter commented Mar 2, 2022 • edited Loading

Codecov Report

datamel left a comment

Choose a reason for hiding this comment

datamel left a comment

Choose a reason for hiding this comment

datamel Mar 14, 2022

Choose a reason for hiding this comment

wxtim Mar 14, 2022

Choose a reason for hiding this comment

MetRonnie May 30, 2022

Choose a reason for hiding this comment

wxtim May 31, 2022 • edited Loading

Choose a reason for hiding this comment

oliver-sanders left a comment

Choose a reason for hiding this comment

wxtim commented May 25, 2022

MetRonnie May 30, 2022

Choose a reason for hiding this comment

MetRonnie Jun 1, 2022 • edited Loading

Choose a reason for hiding this comment

wxtim Jun 1, 2022

Choose a reason for hiding this comment

oliver-sanders commented Jun 17, 2022

wxtim commented Feb 24, 2022 •

edited

Loading

codecov-commenter commented Mar 2, 2022 •

edited

Loading

wxtim May 31, 2022 •

edited

Loading

MetRonnie Jun 1, 2022 •

edited

Loading