Per-target execution platform constraints for exec groups

[fmeum]

The official discussion of the "Per-target execution platform constraints for exec groups" proposal, which aims to introduce new *_exec_compatible_with attributes (most importantly, test_exec_compatible_with) to all rules that can be used to influence the selection of execution platforms for actions running in a non-default action group (such as tests).

Feedback is very welcome!

[katre]

Thanks for sending this.

Overall, I agree that this is useful functionality and makes a lot of sense. I have two areas I want to focus in on, so I'm going to create two sub-threads, one for each.

[katre]

Syntax

I have a slight preference for your alternate syntax, although it makes expressing both the default and non-default a bit confusing:

    exec_compatible_with = {
        "": ["//:has_fast_cpu"],
        "test": ["//:has_gpu"],
    },

However, this parallels how exec_properties handles exec group names.

I also am not excited about having semi-dynamic attribute names added to rules, although I acknowledge that since execution groups are a static set the new attributes would not actually change dynamically.

I'm going to ask @comius and @brandjon, as the rule API and Starlark experts: which is cleaner, adding new attributes based on the execution groups, or adding the new dict[str, list[Label]] attribute type? (Also @lberki, who commented directly on the doc on this same question).

[brandjon]

Technically, there is a complication that "label" is really more than one type

AFAIK we don't have any of the exotic label types exposed to Starlark.

If we wanted symmetry / closure, and especially if we wanted to do it with exotic label types, I think we'd have to move to a fancier way of describing attribute types than simply enumerating combinations manually.

Adding new types of attributes has downstream effects on tooling that needs to understand how to read and write them in BUILD files.

[brandjon]

(For some reason I thought GH discussions would allow arbitrary thread trees. But it looks like my knowledge of how to communicate online is still stuck in the days of usenet. So responding to John's question here.)

IIUC, the proposal that programmatically adds new attribute names is: "Process some execution-group-related parameter of a rule() definition, and based on that, automatically add attribute definitions to its schema following a naming pattern. Then expect those attributes to be present at analysis time and automatically consume them with some special logic."

This sounds a bit hideous to me, simply because of the namespace pollution it entails. Imagine dumping a target in bazel query and seeing a few real attributes and a dozen synthetic ones (well, beyond whatever synthetic attributes we already produce -- but at least those don't have programmatically generated names).

If we can address this use case by adding a new attribute type -- a type whose existence is weakly implied by the types we already support -- then I think that's my preference. Even though it may mean we need to define attr.string_keyed_label_list_dict().

[fmeum]

Since the new attribute would be native, we could add support for it without exposing attr.string_list_dict (or whatever we call it) to Starlark. But @lberki's point still stands that the current selection of Starlark attributes is limited in ways that could be extended without severe implications on tooling (an attr.label_list_dict argument in a BUILD file would be a valid value for an attr.string_list_dict).

[lberki]

Actually, I was only thinking about the internals of Blaze: those also only support the odd assortment of dict-type labels that I listed above. Once that is fixed, plumbing that to Starlark is just a tiny amount of work and as I said above, I think it'd be a net improvement. I'd much rather wait until @comius is back since this sounds like something that he might have an opinion about and it doesn't seem to be urgent enough not to be able to wait a week.

[comius]

I think adding exec_group_compatible_with: dict[str, list[Label]] is one of the nicest way to do it. Overloading exec_compatible_with is also an option - it follows the pattern of overloading a lot of other APIs already have, but then it needs a reserved name for "default" exec group, and the name doesn't point users to exec groups. I'm having hard time to decide between the two, so please pick one.

One alternative is to reuse exec_properties with some string parsing to convert a string to a label list. Cons is string parsing, but a plus is, that's probably adding the least new code / i.e. no new types.

[katre]

Automatic Execution Groups

I think that, even with automatic execution groups, users can currently use exec_compatible_with to change the execution platform for the default execution group. Therefore, I think that this would still be useful for users using automatic execution groups, but it's more difficult since those don't have names.

@kotlaja, what do you think? Is this something we should allow users to do, or is it against the ideas behind automatic execution groups.?

[fmeum]

(they could have used "exec_compatible_with" from the very beginning)

Are you referring to the exec_compatible_with parameter of exec_group here? If so, then it's important to keep in mind that it is very common in Bazel to be using rules one hasn't authored. Being able to specify constraints both on the rule definition and on individual targets is more important due to this cultural difference.

[kotlaja]

Could you share the rationale for this behavior? Do you perhaps have usage examples you could share? I initially expected automatic exec groups to have the same behavior as explicitly defined ones in this way.

When AEGs are not enabled, default-exec-group has constraints from all toolchains plus from exec_compatible_with. What AEGs did is just "splitting" the default-exec-group with multiple toolchains. For example, think of an action which uses one toolchain, which by default uses default-exec-group when AEGs are not enabled (specific exec_groups are not created). This means that this action will have constraints from exec_compatible_with plus all toolchains, while it needs only one toolchain. So if AEGs are enabled, one AEG with that needed toolchain can be used for this action. Still, to maintain the same behaviour as before, exec_compatible_with is also needed (difference is that constraints from not-used toolchains for selecting exec platform for the actions are ignored).

Are you referring to the exec_compatible_with parameter of exec_group here?

I'm referring to the exec_compatible_with parameter of a rule. I was just thinking of an example how this could be used in a wrong way by rule authors and rule users, but this is quite unlucky to happen in my opinion. You can ignore this now since I trust John that there could be a use case to use this feature in the future even for AEGs (reward overweight risk).

[katre]

@kotlaja There are two different uses of exec_compatible_with here.

One is during rule definition:

my_rule = rule(
    ...
    exec_compatible_with = [ ... default exec constraints ],
    exec_groups = {
        "link": exec_group(
            exec_compatible_with = [ link-specific exec constraints ],
        ),
    },
)

This syntax doesn't need to change, either with or without automatic exec groups.

The other usage is on targets in BUILD files, and lets users add new exec constraints to exec groups:

my_rule(
    name = "my_specific_target",
    ... other attributes ...
    exec_compatible_with = [
        extra exec constraints for the default exec group
    ],
)

When AEGs are not enabled, default-exec-group has constraints from all toolchains plus from exec_compatible_with.

I think this is referring to the rule parameter exec_compatible_with. This proposal is about the target attribute exec_compatible_with. Does AEGs have any special handling for the target attribute version?

My assertion is that users who write BUILD files may need to add constraints to automatic exec groups, and that they should probably be able to do this based on the toolchain name, making my above example something like:

my_rule(
    name = "my_specific_target",
    ... other attributes ...
    exec_compatible_with = {
        "???": [ extra exec constraints for the default exec group ]
        "@rules_cc//:toolchain_type": [ extra exec constraints for the AEG for this toolchain type ],
    },

Does that make sense?

@fmeum: Can you update the proposal to add details on:

1. How this interacts with AEG, and
2. How to specify default exec group in the dict-based version?

Thanks

[fmeum]

Will do!

@katre With the dict-based approach, how would we avoid breaking existing usages of exec_compatible_with? We could introduce custom logic that converts a list ["@foo"] to {"": ["@foo"]} or whatever we decide the key for the default exec group to be, but that would require changes to the otherwise generic attribute handling.

Alternatively, we could introduce a new exec_group_compatible_with attribute and have that apply to the non-default exec groups only, thus avoiding the bikeshed of how to reference the default exec group in the dict-based attribute.

[fmeum]

I updated the proposal, please take a look.

[katre]

Looks good to me, waiting for @comius to return (Thursday this week) and give an opinion.

[alexeagle]

Hey John, do you know if anyone has reminded Ivo to take a look here?

[comius]

Thanks for a reminder. After 3rd read I finally understood the proposal.

LGTM. I think user interface is nice and the semantics well defined. It is adding complexity, that will need some learning from the users. I'd suggest we don't forget to update Bazel's documentation pages in this area.

[katre]

With @comius's review, go ahead and mark this as Approved. I'm happy to handle the PRs.