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

Turn log timestamps back on before running Cylc Play in VIP & VR #5524

Merged
merged 1 commit into from
May 11, 2023
Merged

Turn log timestamps back on before running Cylc Play in VIP & VR #5524

merged 1 commit into from
May 11, 2023

Conversation

wxtim
Copy link
Member

@wxtim wxtim commented May 5, 2023 •
edited
Loading

Closes #5505

Cylc Validate disables the use of timestamps in logging. I've added a command to allow it to be restarted before these commands play a workflow.

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 entry not req'd
  • If this is a bug fix, PR should be raised against the relevant ?.?.x branch.

@wxtim wxtim force-pushed the fix.timestamps_in_VIP_&_VR branch from 5501407 to be6cbc1 Compare May 5, 2023 20:31
@wxtim wxtim requested review from MetRonnie and hjoliver May 5, 2023 20:31
@wxtim wxtim self-assigned this May 5, 2023
@wxtim wxtim added this to the cylc-8.1.5 milestone May 5, 2023
hjoliver
hjoliver requested changes May 8, 2023
View reviewed changes
Copy link
Member

@hjoliver hjoliver 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. One minor issue, it would be slightly cleaner to have a single toggle_timestamps function?

@wxtim wxtim requested a review from hjoliver May 9, 2023 09:31
@wxtim
Copy link
Member Author

wxtim commented May 9, 2023 •
edited
Loading

Looks good. One minor issue, it would be slightly cleaner to have a single toggle_timestamps function?

Sort of - my only concern is that for me the word "toggle" means "get the state and change it to the other state". Whilst this function could do this, I think it's better if it specifies the desired state to avoid ambiguity - I think in the places it is used it's more to do "set the state to this, whatever it was before". To this end I've made the enable argument compulsory, because I think defaulting to True is misleading.

@MetRonnie MetRonnie added the could be better Not exactly a bug, but not ideal. label May 9, 2023
MetRonnie
MetRonnie reviewed May 9, 2023
View reviewed changes
cylc/flow/loggingutil.py Outdated Show resolved Hide resolved
cylc/flow/scripts/validate_install_play.py Outdated Show resolved Hide resolved
cylc/flow/scripts/validate_reinstall.py Outdated Show resolved Hide resolved
@hjoliver
Copy link
Member

Sort of - my only concern is that for me the word "toggle" means "get the state and change

Yes, badly chosen word sorry. I really meant something like set_timestamps(state=bool) - i.e. one function but you do have to specify the desired state, not just "toggle" to the other state whatever that is.

@wxtim wxtim force-pushed the fix.timestamps_in_VIP_&_VR branch 2 times, most recently from ff065e7 to f0f8a34 Compare May 11, 2023 07:33
@wxtim wxtim requested a review from MetRonnie May 11, 2023 07:33
@wxtim @MetRonnie
Turn log timestamps back on before running Cylc Play in VIP & VR
d4d3447 
(if that is what user has asked for).

- Replace enable or disable with a single toggle_timestamps fn
- Made enable an arg not a keyword arg because I'd expect
  kwarg with function name "toggle" to change state
  to the other state if unspecified.

Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
@wxtim wxtim force-pushed the fix.timestamps_in_VIP_&_VR branch from f0f8a34 to d4d3447 Compare May 11, 2023 08:41
MetRonnie
MetRonnie approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@MetRonnie MetRonnie left a comment

Choose a reason for hiding this comment

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

Tested out

hjoliver
hjoliver approved these changes May 11, 2023
View reviewed changes
Copy link
Member

@hjoliver hjoliver left a comment

Choose a reason for hiding this comment

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

👍

@hjoliver hjoliver merged commit ce6b7bf into cylc:8.1.x May 11, 2023
@wxtim wxtim deleted the fix.timestamps_in_VIP_&_VR branch May 12, 2023 09:35
@wxtim wxtim mentioned this pull request May 12, 2023
Closed
@hjoliver hjoliver modified the milestones: cylc-8.1.5, cylc-8.2.0 Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MetRonnie MetRonnie MetRonnie approved these changes

@hjoliver hjoliver hjoliver approved these changes

Assignees

@wxtim wxtim

Labels
could be better Not exactly a bug, but not ideal.
Projects
None yet
Milestone
cylc-8.2.0
Development

Successfully merging this pull request may close these issues.
3 participants
@wxtim @hjoliver @MetRonnie