Avoid different transition output directories when only unrelated command line flags changed

[ulrfa]

Description of the problem / feature request:

Bazel is currently including user defined build settings from the command line, when calculating hash for transition output directory, even if those settings are not affected by any transition. That result in zero cache hits, despite changing a command line build setting for only a small subset of the actions.

The problem only occurs if also enabling transitions (for other options). Switching flags on command line, without any transitions works fine.

Would it make sense to replace bazels current hashing of all starlark options:

bazel/src/main/java/com/google/devtools/build/lib/analysis/starlark/FunctionTransitionUtil.java

Lines 436 to 437 in d679546

	// hash all starlark options in map.
	toOptions.getStarlarkOptions().forEach((opt, value) -> toHash.put(opt.toString(), value));

and instead only hash those starlark options that have been affected by any transition, similar to how bazel do it for native options with affectedByStarlarkTransition?

Feature requests: what underlying problem are you trying to solve with this feature?

If automatic configuration trimming becomes reality, we would love to use “bazel test …” with transitions and without command line flags, to test all configurations with a single bazel invocation. But that is not feasible with the current transition scalability and our huge number of user defined build settings.

Instead, as a workaround, we consider setting only a few common options via transitions, such as choosing platform for different target hardware. And use command line flags for most other user defined build settings. But that workaround is not feasible due to this ticket.

Bugs: what's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

$ bazel clean

# Rebuild both myA and myB
$ bazel build //test:myFoo --//test:myCmdLineFlag=True

# Only myA should be rebuilt, but unfortunately also myB is rebuilt.
$ bazel build //test:myFoo --//test:myCmdLineFlag=False

test/defs.bzl:

def _transition_impl(settings, attr):
    return {"//test:myTransitionFlag": attr.myTransitionFlagValue}

myTransition = transition(
    implementation = _transition_impl,
    inputs = [],
    outputs = ["//test:myTransitionFlag"],
)

def foo_impl(ctx):
    return [DefaultInfo(files = depset(ctx.files.dep))]

foo = rule(
    implementation = foo_impl,
    cfg = myTransition,
    attrs = {
        "dep": attr.label(),
        "myTransitionFlagValue": attr.bool(),
        "_whitelist_function_transition": attr.label(
            default = '@bazel_tools//tools/whitelists/function_transition_whitelist',
        ),
    })

test/BUILD:

load(
    "@bazel_skylib//rules:common_settings.bzl",
    "bool_flag",
)

load(":defs.bzl", "foo")

bool_flag(
    name = "myTransitionFlag",
    build_setting_default = False,
)

bool_flag(
    name = "myCmdLineFlag",
    build_setting_default = False,
)

config_setting(
    name = "myTransitionSetting",
    flag_values = {"myTransitionFlag": "True"},
)

config_setting(
    name = "myCmdLineSetting",
    flag_values = {"myCmdLineFlag": "True"},
)

foo(
    name = "myFoo",
    myTransitionFlagValue = True,
    dep = ":myA",
)

genrule(
    name = "myA",
    outs = ["myA.out"],
    srcs = [":myB"],
    cmd = "cat $< > $@; " + select({
        "//test:myCmdLineSetting": "echo with myCommandLineSetting >> $@",
        "//conditions:default":    "echo with default >> $@"}),
)

genrule(
    name = "myB",
    outs = ["myB.out"],
    cmd = select({
        "//test:myTransitionSetting": "echo with myTransitiveSetting >> $@",
        "//conditions:default":       "echo with default >> $@"}),
)

What operating system are you running Bazel on?

Linux

What's the output of bazel info release?

3.7.0

Have you found anything relevant by searching the web?

#12171 is related, but not the same.

[gregestren]

Can you clarify your desired behavior specifically for this example?

I think I understand the basic themes, both what the bug requests and what the example does. But I don't fully parse how you want to integrate the two?

My reading of the example is:

• First build of //myB uses --//test:myCmdLineFlag=True, --test:myTRansitionFlag=True
• Second build of //myB uses --//test:myCmdLineFlag=False, --test:myTRansitionFlag=True
• Since //myB only needs //test:myTransitionFlag, it does the same thing in both builds
• Trimming //test:myCmdLineFlag from //myB would accomplish this

This reads like a straightforward graph trimming request. Are you suggesting an alternative non-trimming approach here?

[ulrfa]

Thanks @gregestren,

This feature request has a narrow scope: user defined build settings set on command line and never touched by any transition.

Bazel is already handling this for native options, by using affectedByStarlarkTransition:

bazel/src/main/java/com/google/devtools/build/lib/analysis/config/CoreOptions.java

Lines 286 to 302 in d679546

	/**
	* This internal option is a *set* of names (e.g. "cpu") of *native* options that have been
	* changed by starlark transitions at any point in the build at the time of accessing. This is
	* used to regenerate {@code transitionDirectoryNameFragment} after each starlark transition.
	*/
	@Option(
	name = "affected by starlark transition",
	defaultValue = "",
	converter = EmptyListConverter.class,
	documentationCategory = OptionDocumentationCategory.UNDOCUMENTED,
	effectTags = {
	OptionEffectTag.LOSES_INCREMENTAL_STATE,
	OptionEffectTag.AFFECTS_OUTPUTS,
	OptionEffectTag.LOADING_AND_ANALYSIS
	},
	metadataTags = {OptionMetadataTag.INTERNAL})
	public List<String> affectedByStarlarkTransition;

Native options set on command line, but not in affectedByStarlarkTransition, is currently not affecting the output directory calculation. This feature request is about similar behavior also for user defined build settings.

This feature request would allow us to combine transitions with command line options:

• Setting a limited number of common options via transitions, affecting large parts of the code base, but with limited number of variants.
• When needed, also setting a few of our many specific options via command line, each of them affecting only their parts of the code base, but with many variants.

That would be a workaround to get reasonable scalability until generic graph trimming become available in a distant future.

What do you think?

[gregestren]

Got it. I think #13317 (comment) covers similar territory, and it's a legitimate request.

[ulrfa]

Thanks @gregestren! I added pull request #13580. What do you think?

[gregestren]

Following up, as promised (and as I've been internalizing all the related issues):

and instead only hash those starlark options that have been affected by any transition, similar to how bazel do it for native options with affectedByStarlarkTransition?

I agree with this basic thesis. We already have a clear precedent in Bazel that flags only affect outputs if transitions affect them. Because transitions create the possibility of two variations of the same output in the same build, at which point you have to have some distinction. This is not true for settings that are ubiquitous throughout the same build, regardless of value.

So I want to move forward with this idea. The only thing I need to resolve in my head is why this difference currently exists for Starlark transitions. I remember Julie and I carefully thinking through the semantics of Starlark transitions, so the current choice was made after some very detailed consideration. I need to convince myself that those considerations don't break down by this approach.

One distinction, for example, is that we don't actually know the default values of Starlark flags that haven't been used anywhere in the build (or even those flags' existence!). Since their definitions are in BUILD files, we only know anything about them when their BUILD files are loaded. This is fundamentally different than native flags, and I believe contributed to the current approach.

But I still don't see how that conflicts with what you're proposing here, so that's what I'm running through in my head...

[gregestren]

Assigned @ulrfa since you have the open PR, but happy to adjust ownership responsibility however make sense to everyone.