[DavidANeil]

When unused_inputs_list is produced by a prior action and passed as an input to the consuming action, read it during discoverInputs() to trim unused inputs before execution.

Resolves #14292.

Description

This hooks up input discovery for Starlark actions by way of passing in the unused_inputs_list as an input to the action.

Motivation

Input discovery can drastically improve build performance by decreasing the number of inputs needed to be added to an execution sandbox, and by decreasing the entropy of the cache key. C++ actions got this by native include scanning, but other actions had no way to access this feature.

Build API Changes

1. Has this been discussed in a design doc or issue? Vary unused_inputs_list behavior based on which action produces the file #14292
2. Is the change backward compatible? I'm not sure what the policy is here. Technically actions that used to not be able to see certain inputs (that they claimed to not read) now might not have those inputs in their sandbox), so yes a breaking change by implementation, but not a breaking change by spec.
3. If it's a breaking change, what is the migration plan? None yes.

Checklist

• I have added tests for the new use cases (if any).
• I have updated the documentation (if applicable).

Release Notes

RELNOTES[INC]: unused_inputs_list artifacts that are passed as an input to an action (i.e. the file was produced by a previous action) will now prune the action's inputs before the action is executed, instead of only after.

[DavidANeil]

It looks like Windows sandboxes don't prune the inputs. I don't know how sandboxes work on Windows. Am I fine to just exclude that test from running on Windows?

[fmeum]

@lberki Given how simple this is to implement, I'm hugely supportive. It solves a real need in Starlark with essentially no new API, even if it doesn't solve the (pretty unique) C++ include scanning use case.

[fmeum]

This flattens the set of inputs during analysis, which is a potentially costly operation. Could this decision be delayed to input discovery time?

[fmeum]

Could the logic in afterExecute be extracted and reused here? It handles path mapping, doesn't eagerly read the whole file into memory, etc.

[DavidANeil]

Okay, I consolidated and added support (and tests) for path mapping. I was having a hard time trying to get the spawn's PathMapper into discoversInputs, so I am just having the action create its own.
It is hard for me to make an architectural decision around how this "should" work, because I ... know nothing about Bazel's architecture. 😅

[fmeum]

It looks like Windows sandboxes don't prune the inputs. I don't know how sandboxes work on Windows. Am I fine to just exclude that test from running on Windows?

They don't exist :⁠-⁠)

Yes, I think that's fine for now. On Windows, the action would need to know not to read these files by itself, it won't see a difference in the inputs as it directly operates on the execroot.

[DavidANeil]

@fmeum, @lberki,
I've rebased the branch, since it had gotten fairly stale.

Any idea what this needs to be merged?

[fmeum]

@DavidANeil CI is red, looks like you need to accommodate further changes on master

[fmeum]

🤖 This review was written by Claude (an AI assistant) and posted through @fmeum's account at their request. Findings marked "reproduced" were verified with a local build of this branch.

The approach holds up: reading the list in discoverInputs fits the existing input discovery flow, and adding the list to getSchedulingDependencies() is both correct and needed (after a pruned execution the inputs are reset and only the action cache inputs are requested, so without it the list could be read stale). The inline comments cover a regression in post-execution pruning, two robustness issues and test coverage. A few points that don't map to a changed line:

• Docs: the unused_inputs_list parameter doc in StarlarkActionFactoryApi.java still describes the file as "generally one of the outputs of the action". It should cover the new mode: when the file is an input, it is read before execution; the paths must be in the form the consuming action sees them (i.e. mapped if that action is path-mapped); and unused inputs are still built before the action runs, since getMandatoryInputs() is the full input set on a clean build. What shrinks is the set of spawn inputs (sandbox, remote execution) and the cache key.

[fmeum]

[Claude] Creating the PathMapper in the constructor causes two problems:

1. It regresses post-execution pruning (reproduced). pruneUnusedInputs uses this field, so the pathMapper parameter of afterExecute, which is the one the spawn actually ran with, is now ignored. A mapper created here can't have an InputMetadataProvider, so when two inputs map to the same stripped path with identical contents (supported since Support path mapping collisions for identical files #29601), this mapper is NOOP while the spawn's mapper strips. The action then writes mapped paths to its unused_inputs_list, none of them match and nothing is pruned. With the setup below and --experimental_output_paths=strip --spawn_strategy=sandboxed, master doesn't re-run //pkg:out after unused.in changes, but this PR does.
2. It flattens the inputs during analysis. StrippingPathMapper.tryCreate calls getInputs().toList(), which is why the javadoc of PathMappers.create asks not to call it from the analysis phase. This affects every action with unused_inputs_list or a shadowed action for which path mapping is effective, and the mapper is then retained for as long as the action is.

Suggestion: drop the field and give pruneUnusedInputs a PathMapper parameter. afterExecute passes its own pathMapper argument as it did before, and discoverInputs creates one on the spot via PathMappers.create(...) with actionExecutionContext.getInputMetadataProvider(). outputPathsMode is private in SpawnAction, so this needs an accessor or a field in this class.

Repro

pkg/defs.bzl:

def _gen_impl(ctx):
    out = ctx.actions.declare_file(ctx.label.name + ".txt")
    ctx.actions.run_shell(
        inputs = [ctx.file.src],
        outputs = [out],
        command = "cp %s %s" % (ctx.file.src.path, out.path),
    )
    return [DefaultInfo(files = depset([out]))]

gen = rule(
    implementation = _gen_impl,
    attrs = {"src": attr.label(allow_single_file = True)},
)

def _consume_impl(ctx):
    out = ctx.actions.declare_file(ctx.label.name + ".out")
    unused_list = ctx.actions.declare_file(ctx.label.name + ".unused")
    args = ctx.actions.args()
    args.add(out)
    args.add(unused_list)
    args.add(ctx.file.unused)
    ctx.actions.run(
        executable = ctx.executable._tool,
        # same_target and same_exec map to the same stripped path and have identical contents.
        inputs = [ctx.file.same_target, ctx.file.same_exec, ctx.file.unused],
        outputs = [out, unused_list],
        arguments = [args],
        unused_inputs_list = unused_list,
        execution_requirements = {"supports-path-mapping": "1"},
    )
    return [DefaultInfo(files = depset([out]))]

consume = rule(
    implementation = _consume_impl,
    attrs = {
        "same_target": attr.label(allow_single_file = True),
        "same_exec": attr.label(allow_single_file = True, cfg = "exec"),
        "unused": attr.label(allow_single_file = True),
        "_tool": attr.label(default = "consume.sh", allow_single_file = True, executable = True, cfg = "exec"),
    },
)

pkg/BUILD:

load(":defs.bzl", "consume", "gen")

gen(name = "same", src = "same.in")
gen(name = "unused", src = "unused.in")

consume(
    name = "out",
    same_exec = ":same",
    same_target = ":same",
    unused = ":unused",
)

pkg/consume.sh (executable):

#!/bin/sh
# Usage: consume.sh <out> <unused_list> <path of the unused input as seen by the action>
date +%s%N > "$1"
echo "$3" > "$2"

Build //pkg:out, change pkg/unused.in, build again and compare bazel-bin/pkg/out.out.

[fmeum]

[Claude] If the list names itself, it is pruned from the inputs here and with that also from the action cache entry. A later change to the list then gives an action cache hit, discoverInputs isn't called again and the old pruning sticks.

Reproduced with a source file pkg/self.list containing pkg/b.input and pkg/self.list as the unused_inputs_list: after changing it to name pkg/c.input instead of pkg/b.input and editing b.input, the action wasn't re-run and its output still reflected the old sandbox contents.

Skipping the list's own path during pre-execution pruning, so that it always remains an input, would fix this. A test for it would be good to have.

[fmeum]

[Claude] I don't think this should be swallowed. Neither case named in the comment occurs:

• The list is an input and a scheduling dependency, so it has always been built by the time discoverInputs runs, including on the first build.
• With Build without the Bytes, the action file system downloads the file on demand in getInputStream. I checked this with --disk_cache and --remote_download_toplevel: the list is fetched and the unused input is absent from the sandbox.

What remains are real I/O errors, and silently running with all inputs for those makes the sandbox contents and the remote cache key vary from run to run. Throwing an ActionExecutionException with Code.UNUSED_INPUT_LIST_READ_FAILURE, as afterExecute does via EnvironmentalExecException, would be consistent. Lost inputs are still detected in SkyframeActionExecutor#discoverInputs when an exception is thrown, so rewinding keeps working.

[fmeum]

[Claude] On master this was prunedInputs = sawUnusedInput, which also resets the flag when an execution prunes nothing. Now it is only ever set to true, both here and in discoverInputs.

I assume the reason is that afterExecute still runs when the list is an input: it reads the file a second time, rebuilds the map over all inputs and finds nothing left to prune, which would otherwise reset the flag set by discoverInputs. Could afterExecute return early when isUnusedInputsListAnInput() instead? That avoids the redundant read and flattening on every execution and lets both places assign the flag again (prunedInputs = pruned != null).

[fmeum]

[Claude] With --spawn_strategy=local, the first six tests don't exercise the new code path. I ran this file against master's StarlarkAction.java: only test_input_discovery_removes_from_sandbox and test_input_discovery_with_path_mapping fail. The others already pass there because post-execution pruning reads the list from disk even when it is an input.

Suggestions:

• Run the incremental scenarios sandboxed and assert on what the action sees. For example, test_input_discovery_list_changes could check that b.input is back in the sandbox once the list stops naming it.
• Add the path collision case from my comment on the constructor and a case in which the list names itself.
• Consider moving the tests into starlark_dependency_pruning_test.sh rather than adding a new target with 8 shards that runs bazel clean and bazel shutdown after every test.

Nit: the comments mentioning "traces the import tree" (line 260) and "imported by a.input" (line 280) don't match what the tests do.

[fmeum]

I wouldn't get to a proper review before BazelCon, but sent out the Claude findings that seem reasonable to me.

[DavidANeil]

Thanks. I think I responded to everything.
I tried to make the path mapper come from the spawn, but then I realized that wouldn't be possible because the spawn doesn't exist when input discovery is running, so I've just consolidated the logic a tiny bit and made it get passed in at the right spot.

[fmeum]

Looks good, I just sent a PR with a test I'd like to see against your branch: lucidsoftware#7

[lberki]

@c-mita WDYT? I'm fully aware of my predilection to say yes more eagerly than I should and I'm inclined to say that this is a pretty reasonable piece of functionality. You might think otherwise, though because you spent some brain cycles recently thinking about how to make Starlark support what C++ compilation does.

[c-mita]

Is it wise to have this functionality attached to the unused_inputs_list parameter? Is there a call for being able to filter inputs before action execution and also recording which were actually used?

At the very least it just seems odd that a parameter might be an "output" file or an "input" file and it may not be obvious at the call site which it is.

You might think otherwise, though because you spent some brain cycles recently thinking about how to make Starlark support what C++ compilation does.

I think it's largely orthogonal. I've not looked closely at this PR yet, but it seems like it would neither solve nor obstruct what we might need to do for C++ or Java compilation (notably it requires an action to generate the initial file, which is something we would want to avoid for C++ compilation).

[lberki]

which is something we would want to avoid for C++ compilation

Is that due to the extra overhead associated with it?

I was hoping you'd say that yes, this is a viable path forward for C++ compilation in which case this would have been an easy "yes". As it is, I'd hate supporting two mechanisms for input discovery. @c-mita any clever ideas? Maybe whatever you have in mind for C++ compilation can be used instead?

(I do share your concerns about a single parameter doing two wildly different things, but that discussion comes after the one about whether we should have this functionality at all)

[c-mita]

Is that due to the extra overhead associated with it?

It was thought that adding an extra action per compile action would be too expensive, although I don't know if anyone actually measured what it would cost in terms of memory requirements.

I was hoping you'd say that yes, this is a viable path forward for C++ compilation in which case this would have been an easy "yes". As it is, I'd hate supporting two mechanisms for input discovery. @c-mita any clever ideas? Maybe whatever you have in mind for C++ compilation can be used instead?

I don't have anything in mind yet. Maybe after Bazelcon I'll have a little bit more to talk about.

But C++ compilation requires at least a little more magic than just include scanning to support everything it currently does and I'm not even sure this feature would even be sufficient for include scanning. This feature allows reducing the set of inputs, but include scanning can (or at least could) add inputs.

[lberki]

Three years after #14292 (comment) I think it makes some sense to introduce a simple mechanism for Starlark rules to access input discovery in some way, even if C++ cannot make use of the mechanism because C++ does have some very special needs.

I propose the following course of action: we wait until @c-mita either comes up with something for C++ or decides that one cannot do that in a way that's useful for Starlark, but the latest, say, a week after the end of BazelCon, then separate unused_input_list into two attributes to avoid the ambiguity (one for its current semantics and another for the input file proposed here) and get on with out lives?

@brandjon mind being the counterweight to my predilection to say "yes", if need be?

[c-mita]

It was thought that adding an extra action per compile action would be too expensive, although I don't know if anyone actually measured what it would cost in terms of memory requirements.

I did a quick test and it seems to be far less expensive than originally thought. It's not ideal for us, but it's a cost I think we're willing to bear.

I propose the following course of action: we wait until @c-mita either comes up with something for C++ or decides that one cannot do that in a way that's useful for Starlark, but the latest, say, a week after the end of BazelCon, then separate unused_input_list into two attributes to avoid the ambiguity (one for its current semantics and another for the input file proposed here) and get on with out lives?

Whatever we end up doing for CppCompileAction, I do not think it'll be implemented via ctx.actions.run. So we can relax a little on API needs right now.

But I would like to be consistent across our API surface. And I do think we will need both of the the following:

• A way of specifying a list of inputs that will be used before an action runs (corresponding to include scanning).
• A way of recording a list of inputs that were used after an action runs (corresponding to the .d file).

For some reason, the current API records "unused" inputs, which is weird because Bazel and compilers only care about used inputs - I don't know why we invert this. I fully expect whatever we do for C++ compilation, it will be phrased in terms of "used" inputs instead of "unused".

All that said:
I will insist this functionality be on a separate parameter to ctx.actions.run instead of overloading an existing one, and I would prefer both it and the "unused_inputs_list" actually reflect "used" inputs (but this might be too much).