Support optionally restarting interactive processes when input files change #13178

stuhood · 2021-10-08T21:55:35Z

Adds pex_binary(.., restartable=True) and repl --restartable to optionally allow user processes to be restarted when files change. See run_integration_test.py for an example.

InteractiveProcess is used for any process that needs to run in the foreground, including those for run, and repl. To support restarting those processes, we make invoking an InteractiveProcess async, which allows it to (optionally) be restarted when files change.

To make InteractiveProcess async, we convert it into a rust-side Intrinsic, and introduce a new Effect awaitable type, which is used in place of Get when a type is SideEffecting. This also prepares the other SideEffecting param types to be converted to Effects as well (in order to accomplish #10542).

Fixes #9462.

stuhood · 2021-10-10T23:33:32Z

~~This is atop #13199 currently, but is reviewable.~~

The commits are somewhat useful to review independently, but the second and third overlap one another quite a bit.

benjyw · 2021-10-11T07:20:27Z

src/python/pants/engine/rules.py

+        )
+        if signature is not None:
+            product_str, subject_str, effect = signature
+            get = AwaitableConstraints(


Are get/gets still appropriate var names here? (and in validate_requirements()?) One might also be an effect, no?

There is going to be a long tail of updates required, for sure. I'm not really sure what the best abstract name for the new concept is (used Awaitable in selectors, but mostly because that was what it was used for in that file). But I'll change the local variable names nearby Awaitable* at least.

kaos

Cool stuff.
Think there's only a few outstanding nits and bits for error feedback, could be done as follow up too..

src/python/pants/engine/internals/selectors.py

src/python/pants/engine/process.py

src/python/pants/engine/rules.py

src/python/pants/core/goals/run.py

Eric-Arellano

🤘

src/python/pants/engine/internals/selectors.py

src/python/pants/core/goals/run.py

Eric-Arellano · 2021-10-11T13:20:50Z

src/python/pants/core/goals/run_integration_test.py

+
+
+@ensure_daemon
+def test_run_then_edit(use_pantsd: bool) -> None:


Eric-Arellano · 2021-10-11T13:22:57Z

src/python/pants/core/goals/test.py

@@ -490,7 +491,7 @@ async def run_tests(
                OpenFiles, OpenFilesRequest(coverage_report_files, error_if_open_not_found=False)
            )
            for process in open_files.processes:
-                interactive_runner.run(process)
+                _ = await Effect(InteractiveProcessResult, InteractiveProcess, process)


No need for _ =

When discarding something, I like to make it clear that it's intentional.

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

… support configuring whether to restart. [ci skip-build-wheels] # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

[ci skip-build-wheels]

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

# 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]

Eric-Arellano · 2021-10-11T16:39:37Z

src/python/pants/core/goals/run.py

+        "If your application can safely be restarted while it is running, you can pass "
+        "`restartable=True` on your binary target (for supported types), and the `run` goal "


Imo pass => CLI arg, set => change a BUILD file.

Suggested change

"If your application can safely be restarted while it is running, you can pass "

"`restartable=True` on your binary target (for supported types), and the `run` goal "

"If your application can safely be restarted while it is running, you can set "

"`restartable=True` on your binary target (for supported types), and the `run` goal "

Eric-Arellano · 2021-10-11T16:39:48Z

src/python/pants/core/goals/run.py

+        "If your application can safely be restarted while it is running, you can pass "
+        "`restartable=True` on your binary target (for supported types), and the `run` goal "
+        "will automatically restart them as all relevant files change. This can be particularly "
+        "useful for server applications."


like Django and Flask.

…t spurious restarts. (#13385) Unlike `run_rule_with_mocks`, `RuleRunner.run_goal_rule` does actually run a `Goal` via the engine, and is thus subject to being restarted when its inputs have changed (see #13178). Consequently, the `Console` and `Workspace` types _do_ need to track side-effects in that context, in order to prevent the `@rule` from being restarted after its first side-effect. This was exhibiting as test flakiness for a goal that re-writes files in the workspace (see #13383), because after writing the updated lockfile to disk, the `@rule` has changed its own inputs and is subject to restart: if the restart arrived before any `Console` output was written, the second attempt to run the rule would not find anything to do, and would thus not render any `Console` output. Fixes #13383. [ci skip-rust] [ci skip-build-wheels]

kaos · 2022-01-04T20:24:41Z

src/python/pants/engine/rules.py

-                    f"A `@rule` that was not a @goal_rule ({func_id}) has a `SideEffecting` parameter: {ty}"
-                )
+    if not cacheable:
+        return


Just realized now, that this early return cancels all the Effect checks further down for @goal_rules. cc @stuhood

Thanks! #14069.

#13178 added additional validation of use of `Effect`s in `@goal_rules` but it was accidentally unreachable. [ci skip-rust] [ci skip-build-wheels]

stuhood force-pushed the stuhood/sideeffect-based-restartability branch 5 times, most recently from c847d4c to 0b12c42 Compare October 10, 2021 23:12

stuhood changed the title ~~Rules are restartable unless they have had sideeffects~~ Support optionally restarting interactive processes when input files change Oct 10, 2021

stuhood requested review from kaos, Eric-Arellano and benjyw October 10, 2021 23:31

stuhood marked this pull request as ready for review October 10, 2021 23:31

stuhood force-pushed the stuhood/sideeffect-based-restartability branch 2 times, most recently from 52a9915 to f5c289b Compare October 11, 2021 04:16

benjyw approved these changes Oct 11, 2021

View reviewed changes

kaos approved these changes Oct 11, 2021

View reviewed changes

Eric-Arellano approved these changes Oct 11, 2021

View reviewed changes

stuhood added 4 commits October 11, 2021 08:40

Add Effect for side-effecting types.

3d5150c

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

InteractiveProcess is conditionally restartable, and repl and run…

e4d7630

… support configuring whether to restart. [ci skip-build-wheels] # Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust]

InteractiveProcess is async.

9722581

[ci skip-build-wheels]

Review feedback.

1e82cda

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

stuhood force-pushed the stuhood/sideeffect-based-restartability branch from f5c289b to 1e82cda Compare October 11, 2021 15:58

stuhood enabled auto-merge (squash) October 11, 2021 15:58

Eric-Arellano approved these changes Oct 11, 2021

View reviewed changes

Add explanation to run and add RestartableField in more places.

f0c75a6

# 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]

Eric-Arellano reviewed Oct 11, 2021

View reviewed changes

stuhood mentioned this pull request Oct 11, 2021

Support concurrent execution of goals in a single run #10542

Open

stuhood merged commit a7619cf into pantsbuild:main Oct 11, 2021

stuhood deleted the stuhood/sideeffect-based-restartability branch October 11, 2021 17:14

stuhood mentioned this pull request Oct 11, 2021

[internal] Split restartable from cacheable for graph Nodes, and lift SideEffecting to an abstract class #13199

Merged

stuhood mentioned this pull request Oct 20, 2021

Create a mock InteractiveRunner #9108

Closed

stuhood mentioned this pull request Oct 27, 2021

[internal] Track side-effects in RuleRunner.run_goal_rule to prevent spurious restarts #13385

Merged

kaos reviewed Jan 4, 2022

View reviewed changes

stuhood mentioned this pull request Jan 4, 2022

Fix @rule Effect validation for not cacheable types. #14069

Merged

stuhood added a commit that referenced this pull request Jan 4, 2022

Fix @rule effect validation for not cacheable types. (#14069)

920591e

#13178 added additional validation of use of `Effect`s in `@goal_rules` but it was accidentally unreachable. [ci skip-rust] [ci skip-build-wheels]

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support optionally restarting interactive processes when input files change #13178

Support optionally restarting interactive processes when input files change #13178

stuhood commented Oct 8, 2021 •

edited

stuhood commented Oct 10, 2021 •

edited

benjyw Oct 11, 2021

stuhood Oct 11, 2021

kaos left a comment

Eric-Arellano left a comment

Eric-Arellano Oct 11, 2021

Eric-Arellano Oct 11, 2021

stuhood Oct 11, 2021

Eric-Arellano Oct 11, 2021

Eric-Arellano Oct 11, 2021

kaos Jan 4, 2022

stuhood Jan 4, 2022



		@ensure_daemon
		def test_run_then_edit(use_pantsd: bool) -> None:

		"If your application can safely be restarted while it is running, you can pass "
		"`restartable=True` on your binary target (for supported types), and the `run` goal "

Support optionally restarting interactive processes when input files change #13178

Support optionally restarting interactive processes when input files change #13178

Conversation

stuhood commented Oct 8, 2021 • edited

stuhood commented Oct 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaos left a comment

Choose a reason for hiding this comment

Eric-Arellano left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood commented Oct 8, 2021 •

edited

stuhood commented Oct 10, 2021 •

edited