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

Add support for InteractiveProcess consuming inputs while run_in_workspace=True #16093

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Jul 8, 2022

As described in #16105, blocking code in @rule bodies can trigger a shutdown race condition when pantsd is disabled and Ctrl+C is sent.

Longer term solutions are discussed on that issue, but in the short term, we can avoid using blocking code for InteractiveProcess, such that run uses the same sandbox creation, async teardown, and relativizing code as Process does.

Fixes #13852, fixes #14386, fixes #16120, and fixes #15771.

@stuhood stuhood added the category:internal CI, fixes for not-yet-released features, etc. label Jul 8, 2022
@stuhood stuhood force-pushed the stuhood/interactive-process-uses-prepare-workdir branch 2 times, most recently from bd92f4b to 530bd42 Compare July 8, 2022 20:24
@stuhood stuhood force-pushed the stuhood/interactive-process-uses-prepare-workdir branch from 530bd42 to 7906af5 Compare July 8, 2022 20:28
@stuhood stuhood changed the title InteractiveProcess uses prepare_workdir Add support for InteractiveProcess consuming inputs while run_in_workspace=True Jul 8, 2022
@stuhood stuhood added category:bugfix Bug fixes for released features and removed category:internal CI, fixes for not-yet-released features, etc. labels Jul 8, 2022
@stuhood stuhood force-pushed the stuhood/interactive-process-uses-prepare-workdir branch from 7906af5 to a93fc66 Compare July 8, 2022 21:39
@stuhood stuhood added this to the 2.13.x milestone Jul 8, 2022
@stuhood stuhood marked this pull request as ready for review July 8, 2022 21:40
@stuhood
Copy link
Sponsor Member Author

stuhood commented Jul 8, 2022

Commits are useful to review independently.

@thejcannon
Copy link
Member

I can confirm this fixes #15771

[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
…hroots.

[ci skip-rust]
[ci skip-build-wheels]
@stuhood stuhood force-pushed the stuhood/interactive-process-uses-prepare-workdir branch from a93fc66 to 8557cd0 Compare July 10, 2022 21:51
Copy link
Sponsor Contributor

@benjyw benjyw 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 to me! Although on the rust side I can only review the general shape, and not the nuances.

@stuhood stuhood merged commit 756a448 into pantsbuild:main Jul 12, 2022
@stuhood stuhood deleted the stuhood/interactive-process-uses-prepare-workdir branch July 12, 2022 16:06
stuhood added a commit to stuhood/pants that referenced this pull request Jul 12, 2022
…orkspace=True` (pantsbuild#16093)

As described in pantsbuild#16105, blocking code in `@rule` bodies can trigger a shutdown race condition when `pantsd` is disabled and `Ctrl+C` is sent.

Longer term solutions are discussed on that issue, but in the short term, we can avoid using blocking code for `InteractiveProcess`, such that `run` uses the same sandbox creation, async teardown, and relativizing code as `Process` does.

Fixes pantsbuild#13852, fixes pantsbuild#14386, fixes pantsbuild#16120, and fixes pantsbuild#15771.

[ci skip-build-wheels]
stuhood added a commit that referenced this pull request Jul 12, 2022
Rust tests were accidentally skipped in the top commit of #16093.

[ci skip-build-wheels]
stuhood added a commit that referenced this pull request Jul 12, 2022
…orkspace=True` (Cherry-pick of #16093) (#16148)

As described in #16105, blocking code in `@rule` bodies can trigger a shutdown race condition when `pantsd` is disabled and `Ctrl+C` is sent.

Longer term solutions are discussed on that issue, but in the short term, we can avoid using blocking code for `InteractiveProcess`, such that `run` uses the same sandbox creation, async teardown, and relativizing code as `Process` does.

Fixes #13852, fixes #14386, fixes #16120, and fixes #15771.
chrisjrn pushed a commit that referenced this pull request Jul 22, 2022
…16273)

This replaces the hacky `__RuntimeJvm` construct with `InteractiveProcess`' support for `immutable_inputs` and `append_only_caches` that was added in #16093. This required adding passthrough args to `RunRequest`.

Closes #16104 and removes a Terrible Terrible Hack.
@stuhood stuhood mentioned this pull request Jul 22, 2022
jyggen pushed a commit to jyggen/pants that referenced this pull request Jul 27, 2022
…antsbuild#16273)

This replaces the hacky `__RuntimeJvm` construct with `InteractiveProcess`' support for `immutable_inputs` and `append_only_caches` that was added in pantsbuild#16093. This required adding passthrough args to `RunRequest`.

Closes pantsbuild#16104 and removes a Terrible Terrible Hack.
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
stuhood pushed a commit to stuhood/pants that referenced this pull request Aug 2, 2022
…antsbuild#16273)

This replaces the hacky `__RuntimeJvm` construct with `InteractiveProcess`' support for `immutable_inputs` and `append_only_caches` that was added in pantsbuild#16093. This required adding passthrough args to `RunRequest`.

Closes pantsbuild#16104 and removes a Terrible Terrible Hack.
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
stuhood pushed a commit to stuhood/pants that referenced this pull request Aug 2, 2022
…antsbuild#16273)

This replaces the hacky `__RuntimeJvm` construct with `InteractiveProcess`' support for `immutable_inputs` and `append_only_caches` that was added in pantsbuild#16093. This required adding passthrough args to `RunRequest`.

Closes pantsbuild#16104 and removes a Terrible Terrible Hack.
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
stuhood added a commit that referenced this pull request Aug 2, 2022
…herry-pick of #16273) (#16370)

This replaces the hacky `__RuntimeJvm` construct with `InteractiveProcess`' support for `immutable_inputs` and `append_only_caches` that was added in #16093. This required adding passthrough args to `RunRequest`.

Closes #16104 and removes a Terrible Terrible Hack.

[ci skip-rust]
[ci skip-build-wheels]

Co-authored-by: Christopher Neugebauer <chrisjrn@toolchain.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:bugfix Bug fixes for released features
Projects
None yet
3 participants