Create infrastructure for running interactive local process #8495

gshuflin · 2019-10-18T01:42:27Z

Draft of what an interactive process runner might look like, along with the stub of a run @console_rule. This isn't complete yet but I wanted to see what other people thought of this design.

jsirois

This LGTM. If the Workspace solution made sense to everyone, this is just the same pattern for another side-effecting io operation. Whatever we were / are going to do to robustify / improve / evolve Workspace surely applies here too.

jsirois · 2019-10-18T01:53:52Z

src/rust/engine/engine_cffi/src/lib.rs

+    }
+
+    // deliberately do-nothing SIGINT handler
+    ctrlc::set_handler(|| {}).map_err(|e| e.to_string())?;


I think this will need to use tokio-net's ctrl-c: Detegr/rust-ctrlc#30

illicitonion

The shape looks reasonable to me, with one comment

illicitonion · 2019-10-18T15:02:50Z

src/rust/engine/engine_cffi/src/lib.rs

+    }
+
+    // deliberately do-nothing SIGINT handler
+    ctrlc::set_handler(|| {}).map_err(|e| e.to_string())?;


We'll want to remove this signal handler (and reset it to whatever it was set to in python) when this function finishes...

Yeah, I'm not happy with this solution either, but I'm not sure what the right way is to save the existing SIGINT handler and restore it after the subprocess execution is done (although I'll check out @jsirois 's link in the above comment).

gshuflin · 2019-10-22T19:35:59Z

Problem

pants run and pants repl (and perhaps other goals) have as their "output" the actual execution of a program, locally and interactively, which the current ExecuteProcessRequest infrastructure isn't designed to handle well.

Solution

This commit introduces a new type InteractiveRunner, which works like Console and Workspace, in that it can be requested only by a @console_rule and offers a blocking method to kick off an interactive subprocess, that takes control of the terminal.

This commit also happens to include the stub of a v2 run @console_rule (called, currently, v2-run), which is a stub that just runs /usr/bin/python and was only included to test the rule - eventually this will become a full-featured v2 run goal.

illicitonion

Generally looks nice - thanks!

illicitonion · 2019-10-23T22:51:27Z

src/python/pants/engine/scheduler.py

        else:
          state = Return(self._from_value(raw_root.handle))
        roots.append(state)
    finally:
      self._native.lib.nodes_destroy(raw_roots)
+
+    if remaining_runtime_exceptions_to_capture:
+      raise ExecutionError('Internal logic error in scheduler: expected elements in '


The error message here appears to be the negation of the if condition?

This is coming from one of the commits @cosmicexplorer had in his currently-open-PR that I rebased off of becuase the bug he fixed was affecting me. I'm not sure if makes sense to try to get that merged and then merge this PR after that, or just merge this one with those commits.

illicitonion · 2019-10-23T22:57:14Z

src/rust/engine/engine_cffi/src/lib.rs

+      Some(TempDir::new().map_err(|err| format!("Error creating tempdir: {}", err))?)
+    };
+
+    maybe_tempdir


Personal style thing: I find:

if let Some(tempdir) = maybe_tempdir { command.current_dir(tempdir.path()) }

more straightforward to read than a side-effecting map of an Option :)

illicitonion · 2019-10-23T23:00:52Z

src/rust/engine/src/externs.rs

+  value: &Value,
+  field: &str,
+) -> Result<BTreeMap<String, String>, String> {
+  let mut map: BTreeMap<String, String> = BTreeMap::new();


This can be implemented kind of cutely as:

Ok(parts.into_iter().tuples<(_, _)>().collect())

Draft of what an interactive process runner might look like

Instead of establishing a rust signal handler, use the ExceptionSink infrastructure that already exists to selectively ignore SIGINT. In the V2 engine, a SIGINT raised when execution is in rust does not get handled by the python signal handler until sometime after the engine is finished executing a goal, which is why the existing `ignoring_sigint` context in `ExceptionSink` does not work (it successfuly increments and then decrements _threads_ignoring_sigint; but the signal is only handled *after* that happens).

Use a bool flag instead to make it easier to pass across FFI.

### Problem The rust level `Node::cacheable` flag is currently only used to mark `@goal_rule`s as uncacheable (because they are allowed to operate on `@sideeffecting` types, such as the `Console` and the `Workspace`). But since the implementation of `cacheable` did not allow it to operate deeply in the Graph, we additionally needed to mark their parent `Select` nodes uncacheable, and could not use the flag in more positions. Via #7350, #8495, #8347, and #8974, it has become clear that we would like to safely allow nodes deeper in the graph to be uncacheable, as this allows for the re-execution of non-deterministic processes, or re-consumption of un-trackable state, such as: 1. a process receiving stdin from a user 2. an intrinsic rule that pokes an un-watched file on the filesystem 3. interacting with a stateful process like git Note that these would all be intrinsic Nodes: it's not clear that we want to expose this facility to `@rule`s directly. ### Solution Finish adding support for uncacheable nodes. Fixes #6598. When an uncacheable node completes, it will now keep the value it completed with (in order to correctly compute a `Generation` value), but it will re-compute the value once per `Session`. The accurate `Generation` value for the uncacheable node allows its dependents to "clean" themselves and not re-run unless the uncacheable node produced a different value than it had before. ### Result The `Node::cacheable` flag may be safely used deeper in the graph, with the semantics that requests for any of an uncacheable node's dependents will cause it to re-run once per `Session`. The dependents will not re-run unless the value of the uncacheable node changes (regardless of the `Session`).

gshuflin requested review from stuhood and jsirois October 18, 2019 01:42

jsirois reviewed Oct 18, 2019

View reviewed changes

illicitonion reviewed Oct 18, 2019

View reviewed changes

gshuflin force-pushed the subprocess-runner branch from ea94ec0 to 1d5ad31 Compare October 22, 2019 19:26

gshuflin changed the title ~~Starting work on interactive local subprocesses~~ Create infrastructure for running interactive local process Oct 22, 2019

gshuflin marked this pull request as ready for review October 22, 2019 19:30

gshuflin force-pushed the subprocess-runner branch 3 times, most recently from c71be9c to d7f50e5 Compare October 23, 2019 19:48

illicitonion reviewed Oct 23, 2019

View reviewed changes

gshuflin force-pushed the subprocess-runner branch 6 times, most recently from 33caec7 to f96d141 Compare October 30, 2019 00:54

cosmicexplorer and others added 10 commits October 30, 2019 12:30

make keyboardinterrupt capturing in rules work

6dc097f

clean up and make code correct

d95a54e

use list .pop(0) and remove deque

d7b1940

Create InteractiveRunner type + run @console_rule

f5700e5

Draft of what an interactive process runner might look like

Get rid of enum for controlling where subprocess executes

3e30b2c

Use a bool flag instead to make it easier to pass across FFI.

Fix BUILD file and dependencies

c5648c3

Respond to PR

5a87acd

use tuples()

48538e1

Need to fix the build file again.

e1bace0

gshuflin force-pushed the subprocess-runner branch from f96d141 to 81a7f66 Compare October 30, 2019 21:14

Fix type error in toggle method

81a7f66

gshuflin merged commit 2bdb223 into pantsbuild:master Oct 31, 2019

gshuflin deleted the subprocess-runner branch October 31, 2019 00:25

stuhood mentioned this pull request Jan 26, 2020

Dirty the dependents of uncacheable nodes #9015

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create infrastructure for running interactive local process #8495

Create infrastructure for running interactive local process #8495

gshuflin commented Oct 18, 2019

jsirois left a comment

jsirois Oct 18, 2019

illicitonion left a comment

illicitonion Oct 18, 2019

gshuflin Oct 18, 2019

gshuflin commented Oct 22, 2019

illicitonion left a comment

illicitonion Oct 23, 2019

gshuflin Oct 23, 2019

illicitonion Oct 23, 2019

illicitonion Oct 23, 2019

Create infrastructure for running interactive local process #8495

Create infrastructure for running interactive local process #8495

Conversation

gshuflin commented Oct 18, 2019

jsirois left a comment

Choose a reason for hiding this comment

jsirois Oct 18, 2019

Choose a reason for hiding this comment

illicitonion left a comment

Choose a reason for hiding this comment

illicitonion Oct 18, 2019

Choose a reason for hiding this comment

gshuflin Oct 18, 2019

Choose a reason for hiding this comment

gshuflin commented Oct 22, 2019

Problem

Solution

illicitonion left a comment

Choose a reason for hiding this comment

illicitonion Oct 23, 2019

Choose a reason for hiding this comment

gshuflin Oct 23, 2019

Choose a reason for hiding this comment

illicitonion Oct 23, 2019

Choose a reason for hiding this comment

illicitonion Oct 23, 2019

Choose a reason for hiding this comment