Optionally execute processes exclusively in the foreground #8974
Conversation
|
This is a working draft for discussion, with a few TODOs: Commits are useful to review independently. |
|
I'm concerned that this PR is too closely tied to what would've been useful for making specifically #8827 easier to implement, especially now that that change is done with and merged. I do recognize that we will probably want to add --debug flags for non-Python tests in the future though.
We definitely need streaming stdin to work for at least two subprocess usecases - The test
I think this PR only makes sense if #6598 is implemented first. Without it, we can't guarantee that the engine will actually run an EPR with the I basically see this proposal + #6598 as a way of getting rid of the need for side-effecting types like I don't think it makes sense to have two separate ways of solving the problem of running a subprocess, so we should aim either to make IPR the right way to run a foregrounded subprocess, or implement this change to EPR and remove IPR. I'm concerned that right now this PR doesn't let us replicate everything IPR does with modifications to EPR; and also that even if it did, this would necessarily involve making large chunks of the rule graph selectively uncached by flipping a boolean in an EPR data structure somewhere. It might desirable to force rule-writers to structure their types such that they can only do side-effecting things in a |
To be clear: it already works here. I'm suggesting that in some cases folks might want streaming stdout/stderr without stdin, and so we might not want |
Yes: all of them. Which means that all test runners will have this duplication. But additionally, there has been a desire to (conditionally) stream output from processes: you could imagine the
I agree that it would be unfortunate to need two ways to do things, but I think I agree that having sideeffecting operations (and to be clear: the sideeffecting part here is not stdin: rather, the fact that the process runs in the buildroot, rather than in a chroot) deeper in the graph is a no-go. Is it possible that there is a middle ground where IPR can be made significantly simpler, and essentially targeted at only |
### 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`).
…temporarily relinquish stdio access.
…dio, and so we panic and deadlock on the logger.
stuhood
force-pushed
the
stuhood/process-execute-foreground
branch
from
0e03fc1 to
10720dc
Compare
|
Still a working draft, but now based on #9015, so it has proper cache behavior. |
| @@ -188,25 +187,15 @@ def get_packages_to_cover( | |||
| description=f'Run Pytest for {target_with_origin.adaptor.address.reference()}', | |||
| timeout_seconds=test_setup.timeout_seconds if test_setup.timeout_seconds is not None else 9999, | |||
| env=env, | |||
| foreground=test_options.values.debug, | |||
There was a problem hiding this comment.
This flag replaces the debug-specific test runner code.
|
Is this stale? I think you mentioned wanting to land this last week in a thread on |
|
This is stale, but the idea isn't dead. Can close it if it would clean things up to do so. I'd still like to unify the |
It'd be great to close, if you don't mind. I think it's helpful for the # of PRs to remain low and only be work that we have an intent to merge in the near future. Helps us to stay on track with things like users submitting PRs. |
|
Have updated #8923 with the new information. Closing for now. |
Problem
As #8923 explains: streaming, foreground access to processes run by Pants is a useful tool for implementing debugging facilities. But it is currently only possible inside of a
@goal_ruleviaInteractiveProcessRequest(rather thanExecuteProcessRequest), and this makes it more challenging to implement toggling of debug facilities without parallel codepaths that deal with the independent types and APIs.Solution
Add support for a
foregroundflag onExecuteProcessRequestthat forces the process to run:Result
./pants test --debugis implemented using this facility.