Support concurrent execution of goals in a single run

[stuhood]

Currently ./pants test lint $target runs sequentially, and does not start any of the work involved with lint until after test has finished. This is because goals are currently implemented as sequential requests from outside the engine, which allows us to avoid needing to worry about how their outputs might be interleaved.

But because we have carefully controlled how goal rules trigger sideeffects (either via using the Workspace object to write outputs to disk, or the Console object to write stdio to the user), we might be able to use finer-grained locking to allow goals to run concurrently/in-parallel until they need access to the Workspace or Console, at which point they would be allowed to acquire those in the order specified on the commandline.

"Waiting to acquire" in the goal might mean adding an async API to acquire access to the Console/Workspace though, which will take some thought. One other area that might be challenging is fmt, which uses the Workspace to create sideeffects that other goals would like to consume: running ./pants fmt lint should never fail in lint due to errors that have been fixed during fmt, and with naive concurrency, that might not be the case.

The above is sufficient for the 80% of usecases where the goals all wanted to operate on the same set of targets. For the other 20% where your CI environment wants to skip some targets/tags for some goals, something like the Root Selection Overhaul proposal might be necessary.

[Eric-Arellano]

How does InteractiveRunner work?

[stuhood]

How does InteractiveRunner work?

This would need to be adjusted to use a similar "waiting to acquire" API, or the "run" method would just be made asynchronous itself.

---

I've added this to the Plugin API Stability project, because it's highly desirable, but will require some minor changes to @goal_rules.

[stuhood]

The potential API changes here to await acquiring resources relate to #11329.

[stuhood]

I think that this could be accomplished by treating use of the Workspace/etc as critical sections, and applying some constraints:

1. until/unless a @goal_rule enters one of these critical sections, it is interruptible, but after it has, it is no longer interruptible for the rest of its run
2. a @goal_rule may not enter a critical section until the goals listed before it on the command line have completed
3. there is an implicit critical section as the final statement of a @goal_rule (so that running a goal which has no sideeffect still waits for goals listed before it on the command line to complete before calling itself complete).

Together these constraints would mean that @goal_rules are able to run concurrently, but would still be restarted to observe sideeffects caused by other goals.

[stuhood]

I thought that #9462 would not require us to change the public API of @side_effecting types, and it mostly doesn't... except that InteractiveProcess needs to actually become async in order to be interruptible. Currently it cheats by using block_in_place_and_wait to block a thread:

pants/src/rust/engine/src/externs/interface.rs

Lines 1937 to 1953 in 3d45c04

	///
	/// Calling `wait()` in the context of a @goal_rule blocks a thread that is owned by the tokio
	/// runtime. To do that safely, we need to relinquish it.
	/// see https://github.com/pantsbuild/pants/issues/9476
	///
	/// TODO: The alternative to blocking the runtime would be to have the Python code `await` special
	/// methods for things like `write_digest` and etc.
	///
	fn block_in_place_and_wait<T, E, F>(py: Python, f: impl FnOnce() -> F + Sync + Send) -> Result<T, E>
	where
	F: Future<Output = Result<T, E>>,
	{
	py.allow_threads(|| {
	let future = f();
	tokio::task::block_in_place(|| futures::executor::block_on(future))
	})
	}

---

So, that means that to resolve #9462, we need to choose an async API for InteractiveProcess, and that will likely set the convention that we will use for other APIs that will need to become async to resolve this ticket (Console, Workspace).

A few potential APIs:

# have the side-effecting method return an Effect type that can be awaited similar to Get?
process_result = await interactive_runner.run(InteractiveProcess(..))

# make side-effecting classes magic?
process_result = await InteractiveProcess(..)

# new keyword?
process_result = await Do(interactive_runner.run(InteractiveProcess(..))

# etc?

[Eric-Arellano]

How about something like this?

process_result = await SideEffect(InteractiveProcessResult, InteractiveProcess)

I agree with not await interactive_runner.run(InteractiveProcess(..)) - plugin authors are already confused enough thinking they can use await anywhere like normal Python programs.

[kaos]

+1 for option 3, with a dedicated keyword, I also believe it will be easier to check the pre conditions etc, and also being clearer what it does. Also helps the documentation case I think, by having a standard "entry point" to reason about, rather than vaguely referring to "any side effecting method..".

[benjyw]

I also like the new keyword option.

[stuhood]

#13178 added this as await Effect(InteractiveProcessResult, InteractiveProcess(..)). The difference between a "side" effect and an effect is nebulous here, since in @goal_rules the Effects are the whole point. Thanks for the feedback!

The rest of this ticket involves porting the other SideEffecting types to async Effects, and then introducing some synchronization at the Session level to ensure that goals wait to apply their Effects until the previous goals have completed (so that they can be restarted if their inputs change). See #10542 (comment).

[thejcannon]

I was thinking about the UI side of this.

We can either decide which goals should be run together and make it just work for the user.

Or we can introduce new syntax so we don't have to make assumptions.

I lean towards 2. Something like fmt+lint test.

[Eric-Arellano]

@thejcannon when would you not want to run in parallel, when it would have been safe to do?

Also I fear that introduces a new error mode. fmt+lint is illegal. Pants knows that, so can wait to finish fmt first. But now we have to decide if we do that silently, vs eagerly error to the user that it's illegal.

[thejcannon]

I'm thinking if I have lint/check and test, I don't think it's worthwhile to run tests on bad code. But then again maybe it's harmless 🤔

[stuhood]

I'm thinking if I have lint/check and test, I don't think it's worthwhile to run tests on bad code. But then again maybe it's harmless 🤔

Racing check against test will frequently be worthwhile in my experience... mypy will show you more errors, but pytest can sometimes show the first error more quickly.