Handle @rule cacheability correctly

[gshuflin]

Me and @jsirois were talking about engine design concerns today as they pertained to the v2 versions of the ./pants run and ./pants repl goals, and we realized that there are a couple of problems around the cacheable flag on @rules that need to solve.

1. Create a type of @rule that is not a @console_rule but is also not cacheable. For instance, many @console_rules that represent top-level goals (such as run) will actually proxy to language-specific runner rules (such as a specific python-run @rule) - and these are not currently marked as non-cacheable. It's unclear whether we want to introduce a 3rd type of @rule, or whether the distinction between console and non-console @rules should really be just a cacheability distinction (perhaps with a name change).

2. Ensuring cacheable rules don't depend on non-cacheable rules. We should do some kind of checking of the graph to make sure that a pants rule developer doesn't accidentally write a cacheable rule that depends on a non-cacheable one. Further, it's not clear if the right response to this is to fail at rule-graph-checking time, or to convert cacheable rules dependent on non-cacheable ones into non-cacheable rules (which "poisons" the graph and might lead to undesirable slowdown - so if we did this, we'd probably want to emit some kind of warning to the console).

3. Adding cacheable flag on intrinsics. If you look at the Rule datastructure defined in engine/src/tasks.rs, the Task variant (representing rules defined in python) has the cacheable boolean, but this doesn't exist on Intrinsic, representing rust-defined rules. This means that we can't define a rule written in rust as noncacheable.

[jsirois]

Thinking more about point 1, it does seem we might be able to structure things such that the non-cacheability stays confined to the @console_rule. This may be a good forcing function or it may not.

For example, given ./pants run, you could imagine, if non-console @rules were still never allowed to be non-cacheable, something like:

class Run(Goal):
  """Runs a binary."""

  name = 'run'


@dataclass(frozen=True)
class Binary:
  digest: Digest  # The digest of a content tree containing the binary.
  path: str  # The path of the binary within the digested content.


@dataclass(frozen=True)
class AddressAndBinary:
  address: BuildFileAddress
  binary: Optional[Binary]  # If None, target was not a binary target.


@console_rule
def run(console: Console, addresses: BuildFileAddresses, runner: Runner) -> Run:
  addresses_and_binaries = yield [Get(AddressAndBinary, Address, addr.to_address()) for addr in addresses]
  binaries = [ab for ab in addresses_and_binaries if ab.binary]
  if len(binaries) > 1:
    console.print_stderr(console.red('Can only one one binary, presented with ...'))
    yield Run(-1)
  elif len(binaries) == 1:
    exit_code = runner.run(binaries[0].binary, options.args)
    yield Run(exit_code)

[stuhood]

Will try to take a look at this tomorrow morning, but referencing #6598, which has some thought on this.

[stuhood]

Looking again, I wonder whether this was right: #6598 (comment)

"Marking" parameters un-cacheable (or forcing them to be, by preventing an __eq__ implementation other than identity) is not strictly semantically correct when it's actually the process execution that is non-determinstic.

But the advantage of doing this via parameters is that Params already propagate from a caller to a callee, transitively through the graph. So everywhere they travel in the graph they cause non-cachability (and they won't go further than they need to due to rule graph compilation). And that makes non-cachability less error prone.

As a thought exercise: the reason @console_rule is not cacheable is that it might have a side-effect. How can you have a side-effect? Via the Console or the Workspace. So in that case it's definitely the param that is not cacheable: a @console_rule without access to either of those would not be able to cause a side-effect, and so you wouldn't be able to tell the difference.

I haven't really thought about how un-cacheable parameters would work in the middle of the graph though. If you have a @console_rule depending on another @rule that executed yield Get(ExecuteProcessResult, LocalProcessExecuteRequest(..)), the rules it depended on would not be cachable, but it would be itself. But, again: since we're expecting sideeffects via the Console and Workspace, maybe the answer is for LocalProcessExecuteRequest to take the Console and Workspace as fields (preventing it from being cacheable, and preventing any @rule that constructed one from being cacheable, etc)? So LocalProcessExecuteRequest(.., workspace, console).

[gshuflin]

If the right unit of cache-ability is parameters and not rules themselves, then maybe the distinction between @rule and @console_rule doesn't make sense, and the engine should determine cacheability depending on whether or not one of a set of side-effect-able types (including Workspace and Console) is requested.

I don't see a strong difference between requiring LocalProcessExecuteRequest to take Console and Workspace as parameters, and creating a new non-cacheable type LocalProcessExecutor that can be requested by a non-cacheable rule, and that has a method run_local_process that can be called on it. This is similar to the way Workspace and Console work already.

(And then once we have a non-cacheable parameter type, I don't see how it matters whether or not the actual subprocess spawning is done in rust or in python, except with regards to signal-handling - but either way, it wouldn't be an engine intrinsic spawning the new process, it would be the non-cacheable type doing it outside the engine).

[jsirois]

And that makes non-cachability less error prone.

But wouldn't checking this / propagating cacheability where it needs to be statically at graph construction time also be just as non error prone? IE: its done once in one bit of unit tested code.

I haven't really thought about how un-cacheable parameters would work in the middle of the graph though.
...
maybe the answer is for LocalProcessExecuteRequest to take the Console and Workspace as fields (preventing it from being cacheable, and preventing any @rule that constructed one from being cacheable, etc)? So LocalProcessExecuteRequest(.., workspace, console).

This might make sense. If you run a local process that does no IO, there is no point, so a LocalProcessExecuteRequest would need to take one or more of a Workspace, Console, Network, .... That said - this seems manufactured. There will be no way I can see for the implementation that takes in a LocalProcessExecuteRequest to use the Workspace, Console and Network, they'll just be sentinels hacked in the signature to get un-cacheability. The actual implementation in rust or python will just fork/exec and let that use the local io resources instead of hand-intercepting each one somehow.

[stuhood]

The actual implementation in rust or python will just fork/exec and let that use the local io resources instead of hand-intercepting each one somehow.

This isn't quite true. The Console would ideally be the actual source of the file handles that are used: that is important from a concurrency perspective, in order to support multiple concurrent connections with different Consoles: we cannot continue to use global stdio handles.

[jsirois]

This isn't quite true. The Console would ideally be the actual source of the file handles that are use...

In the case on the brain, a local EPR, the type would be ExclusiveConsole and this would be moot. We absolutely do not want concurrency for that set of cases. But in general; ie for logging events of an in progress build, agreed.

[stuhood]

This isn't quite true. The Console would ideally be the actual source of the file handles that are use...

In the case on the brain, a local EPR, the type would be ExclusiveConsole and this would be moot. We absolutely do not want concurrency for that set of cases. But in general; ie for logging events of an in progress build, agreed.

@jsirois : The Console is already intended to be exclusive, and the way that we have attempted to enforce that is via #8336 (ie, only @console_rules should be able to request it). Multiple threads interacting with a Console (or the Workspace) would lead to lots of races.

[jsirois]

I'm confused comparing:

This isn't quite true. The Console would ideally be the actual source of the file handles that are used: that is important from a concurrency perspective, in order to support multiple concurrent connections with different Consoles: we cannot continue to use global stdio handles.

With:

The Console is already intended to be exclusive, and the way that we have attempted to enforce that is via #8336 (ie, only @console_rules should be able to request it). Multiple threads interacting with a Console (or the Workspace) would lead to lots of races.

[stuhood]

@jsirois : Currently, "only @console_rule should interact with Console", and that's what provides the exclusivity. If we were to have more @rules interacting with the Console (which we might want to do if we think that parameters deciding cacheability is the way to go) we would need a mechanism other than #8336: namely, the Engine would need the ability to make the parameter itself exclusive.

So I was agreeing with you, but indicating that I don't think the Console needs a name change... it's always been intended to be exclusive.

---

Having said that, "side-effecting" and "exclusive" probably go hand in hand. It's possible that all side-effecting parameters should be exclusive, non-cacheable, etc, via a marker trait of some sort? The non-cacheability is handled by banning overrides of __eq__. Exclusive would need a bit more work, but probably: before invoking a @rule, the engine could look for any marked parameters and acquire a lock on them (in a well-defined order) (and release them during yields... or make them reentrant... hm)?

[stuhood]

This is now implemented internally (as @_uncacheable_rule), but is not exposed to @rule authors in plugins. The decision to expose it can be made independently as part of the push to stabilize the plugin API if it sounds like consumers have uses for it.