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

cylc clean improvements #4440

Merged
merged 11 commits into from
Oct 6, 2021
Merged

cylc clean improvements #4440

merged 11 commits into from
Oct 6, 2021

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Sep 29, 2021

This is a series of changes with no associated Issue.

  • Fix traceback during remote clean when doing cylc clean myflow --remote where ~/cylc-run/myflow contains exactly 1 run dir
  • Improve logging to terminal
    • Log what's being deleted at INFO level
    • For cylc clean only: Make everything below logging level WARNING go to stdout instead of stderr; this allows stdout to be logged during remote clean so the above 2 bullet points can work for remote clean
    • Only log traceback in --debug/-vv mode, not -v (fixes possible typo in 44802cb#diff-e64f339efa9420b39fef7ef5bdee04a49751d7ed6b2446aa048ca1b8a7fa5e4b)
    • Make -v mode log DEBUG level stuff (no traceback), because otherwise it's pretty pointless
  • Allow cylc clean myflow/runN to infer the run number
  • Prevent cylc clean myflow --rm something (where ~/cylc-run/myflow contains exactly 1 run dir) from trying to delete ~/cylc-run/myflow/something (i.e. it will only delete ~/cylc-run/myflow/run1/something)
  • Prevent cylc clean myflow --rm '..' from deleting the ~/cylc-run directory!

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.
  • (master branch) I have opened a documentation PR at Update cylc clean docs slightly cylc-doc#301.

Traceback occurred when using --remote option and reg was parent of 1 run dir
- Use segregated streams (stdout and stderr) instead of just stderr, allowing more useful info logged during remote clean
- Log which files are being deleted in single `-v` (verbose) mode
- Tidy
@MetRonnie MetRonnie added the bug Something is wrong :( label Sep 29, 2021
@MetRonnie MetRonnie added this to the cylc-8.0b3 milestone Sep 29, 2021
@MetRonnie MetRonnie self-assigned this Sep 29, 2021
Do not try to clean in parent dir for `cylc clean foo --rm something` where foo contains 1 run dir (i.e. only clean in the run dir)
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.

  • Read
  • Played with - working as promised.

@MetRonnie MetRonnie requested review from oliver-sanders and removed request for hjoliver October 5, 2021 09:24
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Looks good, yet to test.

I'm not convinced by the dynamic log level stuff, is there a reason we have to change the level of the message rather than the level of the handler?

@@ -260,24 +261,24 @@ def remove_dir_and_target(path: Union[Path, str]) -> None:
Args:
path: the absolute path of the directory to delete.
"""
log = LOG.info if cylc.flow.flags.verbosity == 1 else LOG.debug
Copy link
Member

Choose a reason for hiding this comment

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

I think these should stay at LOG.debug, the visibility of the messages being decided by the level of the log handlers.

Is there a reason that approach won't work here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally there would be a log level between INFO and DEBUG for the -v level, I suppose. Or are you saying we can manually set the CYLC_LOG handler level to DEBUG for cylc clean -v, inside the init_clean function?

Copy link
Member

Choose a reason for hiding this comment

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

Ok, got it, DEBUG messages don't show in VERBOSE mode so you made them INFO level to make them visible.

VERBOSE mode seems a bit pointless ATM, I'm trying to remember what the original idea was...

Options:

  1. We could create a custom log level for VERBOSE (logging supports this).
    • However, do we really need it? Would it really be helpful to separate log messages like this?
    • (related) We have talked about creating a TASK log level (at ~ the same level as info) to help separate task and scheduler events in the log.
  2. We could set the log level to DEBUG in verbose mode
    • I've forgotten what the original idea was, but this would make sense.

With (2) the levels would become:

QUIET

Shows WARNING or higher.

INFO

Shows INFO or higher.

VERBOSE

Shows DEBUG or higher.

DEBUG

Shows DEBUG or higher with full traceback for debug purposes.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've decided to make the cylc.flow.pathutil functions for deleting files/dirs/symlinks log at INFO level so that it's clear what's being deleted. This undoes 6066648. I hope this is fair - it only logs the bare minimum of deleted things (if it's removing a directory, it only logs that, not any constituent files; if a glob matches children of a matching parent, it only logs the parent etc). The only places where the logging may get a bit much are when the workflow has run on many different install targets, or when doing --rm 'log/job/*' instead of --rm log/job for a workflow with many cycle points.

cylc/flow/pathutil.py Outdated Show resolved Hide resolved
cylc/flow/pathutil.py Outdated Show resolved Hide resolved
- Make '-v' log at DEBUG level (so only difference with '-vv' is that turns on traceback)
- Make file deletion log at INFO level during cylc clean
Otherwise daemonised scheduler hangs in quiet mode because it tries to parse this info from the log file
@MetRonnie
Copy link
Member Author

Test failures seem to be flakiness only

Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

Small change that should make the remote logs a little easier to read.

cylc/flow/workflow_files.py Outdated Show resolved Hide resolved
cylc/flow/workflow_files.py Outdated Show resolved Hide resolved
Copy link
Member

@oliver-sanders oliver-sanders left a comment

Choose a reason for hiding this comment

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

I think the logging looks good, the output is pretty minimal and it's very clear what cylc clean is doing 👍.

@MetRonnie MetRonnie requested a review from wxtim October 6, 2021 11:38
@wxtim wxtim merged commit 7a45c9b into cylc:master Oct 6, 2021
@MetRonnie MetRonnie deleted the cylc-clean branch October 6, 2021 14:05
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