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

log_vc_info: Redirect diff straight to file to avoid blocking pipe #5139

Merged
merged 2 commits into from
Sep 26, 2022
Merged

log_vc_info: Redirect diff straight to file to avoid blocking pipe #5139

merged 2 commits into from
Sep 26, 2022

Conversation

MetRonnie
Copy link
Member

@MetRonnie MetRonnie commented Sep 16, 2022 •
edited
Loading

Closes #5117. Fixes bug where cylc install would hang if the uncommitted diff of the source dir repo was large.

Instead of using subprocess.PIPE for the output of git/svn diff commands, use the file object itself. This avoids blocking the pipe if the diff is large, and also has the benefit of not loading large amounts of text into memory.

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).
  • No dependency changes
  • Tests updated
  • CHANGES.md entry included if this is a change that can affect users
  • No docs change needed
  • If this is a bug fix, PRs raised to both master and the relevant maintenance branch.

@MetRonnie MetRonnie added the bug Something is wrong :( label Sep 16, 2022
@MetRonnie MetRonnie added this to the cylc-8.0.3 milestone Sep 16, 2022
@MetRonnie MetRonnie self-assigned this Sep 16, 2022
@MetRonnie MetRonnie changed the base branch from master to 8.0.x September 16, 2022 11:50
oliver-sanders
oliver-sanders reviewed Sep 20, 2022
View reviewed changes
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.

LGTM

cylc/flow/install_plugins/log_vc_info.py Show resolved Hide resolved
@wxtim wxtim merged commit a438c69 into cylc:8.0.x Sep 26, 2022
@MetRonnie MetRonnie deleted the log-vc-info branch September 26, 2022 09:39
datamel pushed a commit that referenced this pull request Sep 27, 2022
@oliver-sanders @dwsutherland
@MetRonnie @hjoliver
8.0.x (#5158)
f88f32e 
* fix reversed data-store edge source-target (#5156)

Co-authored-by: David Sutherland <davidwollow@gmail.com>

* `log_vc_info`: Redirect diff straight to file to avoid blocking pipe (#5139)

* log_vc_info: redirect diff straight to file to avoid blocking pipe
* Update changelog

* A no-flow task should not merge and retrigger incomplete children (#5146)

Prevent no-flow merge.

* remote-install: add "ana/" to the default install list (#5137)

* The `ana/` directory is used by `rose_ana` to load comparison modules.
* These are typically run where the data is generated which is often
  remote.
* These directories typically contain a small number of Python files.

* Db store force triggered (#5023)

Store force-triggered flag in the run DB.

* Add a new functional test.
* Remove some redundant DB updates.
* Update change log.

Co-authored-by: David Sutherland <davidwollow@gmail.com>
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
datamel pushed a commit that referenced this pull request Sep 29, 2022
@oliver-sanders @dwsutherland
@MetRonnie @hjoliver
8.0.x (#5170)
27e866d 
* fix reversed data-store edge source-target (#5156)

Co-authored-by: David Sutherland <davidwollow@gmail.com>

* `log_vc_info`: Redirect diff straight to file to avoid blocking pipe (#5139)

* log_vc_info: redirect diff straight to file to avoid blocking pipe
* Update changelog

* A no-flow task should not merge and retrigger incomplete children (#5146)

Prevent no-flow merge.

* remote-install: add "ana/" to the default install list (#5137)

* The `ana/` directory is used by `rose_ana` to load comparison modules.
* These are typically run where the data is generated which is often
  remote.
* These directories typically contain a small number of Python files.

* Auto bump dev version on release

* Run GH Actions tests on push to `8.*.x` branches

* Db store force triggered (#5023)

Store force-triggered flag in the run DB.

* Add a new functional test.
* Remove some redundant DB updates.
* Update change log.

* remote: ensure all remote commands use a platform config (#5152)

Remote interfaces (SSH & rsync) now all require a platform object for configuration purposes (e.g. "ssh command").
Convenience interfaces added for remote calls to Cylc servers which use the localhost platform configuration. These are now used for:
* `cylc play` command re-invocation on a Cylc server.
* Evaluation of host selection rankings (via `cylc psutil`).
* The detect old contact file check, which tests whether a workflow is still running on the server recored in the contact file.

Co-authored-by: David Sutherland <davidwollow@gmail.com>
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
wxtim added a commit to wxtim/cylc that referenced this pull request Oct 3, 2022
@wxtim
Merge remote-tracking branch 'upstream/8.0.x' into fix.rose-re-instal…
6f83653 
…l-failure.bk

* upstream/8.0.x:
  remote: ensure all remote commands use a platform config (cylc#5152)
  Db store force triggered (cylc#5023)
  Run GH Actions tests on push to `8.*.x` branches
  Auto bump dev version on release
  remote-install: add "ana/" to the default install list (cylc#5137)
  A no-flow task should not merge and retrigger incomplete children (cylc#5146)
  `log_vc_info`: Redirect diff straight to file to avoid blocking pipe (cylc#5139)
  fix reversed data-store edge source-target (cylc#5156)
  Fix Jinja2 support if HOME undefined.
  Assume Jinja2 might be used in global-tests.cylc.
  Fix post-reload trigger. (cylc#5104)
  Bump dev version
  Update changelog
  workflow_state xtrigger: infer run num
  Type annotations
  Prepare release 8.0.2
  Remove HOME Env Variable from get_remote_workflow_run_dir (cylc#5115)
wxtim added a commit that referenced this pull request Oct 6, 2022
@oliver-sanders @dwsutherland
@MetRonnie @hjoliver @wxtim
8.0.x.patch (#5178)
b68c98d 
* fix reversed data-store edge source-target (#5156)

Co-authored-by: David Sutherland <davidwollow@gmail.com>

* `log_vc_info`: Redirect diff straight to file to avoid blocking pipe (#5139)

* log_vc_info: redirect diff straight to file to avoid blocking pipe
* Update changelog

* A no-flow task should not merge and retrigger incomplete children (#5146)

Prevent no-flow merge.

* remote-install: add "ana/" to the default install list (#5137)

* The `ana/` directory is used by `rose_ana` to load comparison modules.
* These are typically run where the data is generated which is often
  remote.
* These directories typically contain a small number of Python files.

* Auto bump dev version on release

* Run GH Actions tests on push to `8.*.x` branches

* Db store force triggered (#5023)

Store force-triggered flag in the run DB.

* Add a new functional test.
* Remove some redundant DB updates.
* Update change log.

* remote: ensure all remote commands use a platform config (#5152)

Remote interfaces (SSH & rsync) now all require a platform object for configuration purposes (e.g. "ssh command").
Convenience interfaces added for remote calls to Cylc servers which use the localhost platform configuration. These are now used for:
* `cylc play` command re-invocation on a Cylc server.
* Evaluation of host selection rankings (via `cylc psutil`).
* The detect old contact file check, which tests whether a workflow is still running on the server recored in the contact file.

* host select: allow unary operators in ranking expressions (#5151)

* host select: allow unary operators in ranking expressions

* Whitelist unary operators (required for expressions like `-1 * x`)
  in `[scheduler][run hosts]ranking` expressions.
* Improve error formatting.

* Update tests/unit/test_exceptions.py

Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>

Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>

* reinstall changes to `rose-suite.conf` (#5125)

reinstall: ensure rose-suite.conf changes trigger reinstallation

Co-authored-by: David Sutherland <davidwollow@gmail.com>
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
datamel pushed a commit to datamel/cylc-flow that referenced this pull request Oct 19, 2022
@oliver-sanders @dwsutherland
@MetRonnie @hjoliver @datamel
8.0.x (cylc#5158)
99d75d0 
* fix reversed data-store edge source-target (cylc#5156)

Co-authored-by: David Sutherland <davidwollow@gmail.com>

* `log_vc_info`: Redirect diff straight to file to avoid blocking pipe (cylc#5139)

* log_vc_info: redirect diff straight to file to avoid blocking pipe
* Update changelog

* A no-flow task should not merge and retrigger incomplete children (cylc#5146)

Prevent no-flow merge.

* remote-install: add "ana/" to the default install list (cylc#5137)

* The `ana/` directory is used by `rose_ana` to load comparison modules.
* These are typically run where the data is generated which is often
  remote.
* These directories typically contain a small number of Python files.

* Db store force triggered (cylc#5023)

Store force-triggered flag in the run DB.

* Add a new functional test.
* Remove some redundant DB updates.
* Update change log.

Co-authored-by: David Sutherland <davidwollow@gmail.com>
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
datamel pushed a commit to datamel/cylc-flow that referenced this pull request Oct 19, 2022
@oliver-sanders @dwsutherland
@MetRonnie @hjoliver @wxtim @datamel
8.0.x.patch (cylc#5178)
cb8f583 
* fix reversed data-store edge source-target (cylc#5156)

Co-authored-by: David Sutherland <davidwollow@gmail.com>

* `log_vc_info`: Redirect diff straight to file to avoid blocking pipe (cylc#5139)

* log_vc_info: redirect diff straight to file to avoid blocking pipe
* Update changelog

* A no-flow task should not merge and retrigger incomplete children (cylc#5146)

Prevent no-flow merge.

* remote-install: add "ana/" to the default install list (cylc#5137)

* The `ana/` directory is used by `rose_ana` to load comparison modules.
* These are typically run where the data is generated which is often
  remote.
* These directories typically contain a small number of Python files.

* Auto bump dev version on release

* Run GH Actions tests on push to `8.*.x` branches

* Db store force triggered (cylc#5023)

Store force-triggered flag in the run DB.

* Add a new functional test.
* Remove some redundant DB updates.
* Update change log.

* remote: ensure all remote commands use a platform config (cylc#5152)

Remote interfaces (SSH & rsync) now all require a platform object for configuration purposes (e.g. "ssh command").
Convenience interfaces added for remote calls to Cylc servers which use the localhost platform configuration. These are now used for:
* `cylc play` command re-invocation on a Cylc server.
* Evaluation of host selection rankings (via `cylc psutil`).
* The detect old contact file check, which tests whether a workflow is still running on the server recored in the contact file.

* host select: allow unary operators in ranking expressions (cylc#5151)

* host select: allow unary operators in ranking expressions

* Whitelist unary operators (required for expressions like `-1 * x`)
  in `[scheduler][run hosts]ranking` expressions.
* Improve error formatting.

* Update tests/unit/test_exceptions.py

Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>

Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>

* reinstall changes to `rose-suite.conf` (cylc#5125)

reinstall: ensure rose-suite.conf changes trigger reinstallation

Co-authored-by: David Sutherland <davidwollow@gmail.com>
Co-authored-by: Ronnie Dutta <61982285+MetRonnie@users.noreply.github.com>
Co-authored-by: Hilary James Oliver <hilary.j.oliver@gmail.com>
Co-authored-by: Tim Pillinger <26465611+wxtim@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wxtim wxtim wxtim approved these changes

@oliver-sanders oliver-sanders oliver-sanders approved these changes

Assignees

@MetRonnie MetRonnie

Labels
bug Something is wrong :(
Projects
None yet
Milestone
cylc-8.0.3
Development

Successfully merging this pull request may close these issues.

log_vc_info: support large diffs
3 participants
@MetRonnie @oliver-sanders @wxtim