Allow exec groups to inherit from the rule

[juliexxia]

10/13 EDIT

Proposal to extend the exec groups API to allow exec groups to inherit requirements from the rule. Extending from other exec groups may happen if there's a documented need in the future.

# instead of duplicating the toolchains and constraints of the rule like so:
my_rule = rule(
  implementation = _impl,
  toolchains = ["//my/toolchain/type"],
  exec_compatible_with = ["//my/constraint"],
  exec_groups = {
   "foo": exec_group(
       toolchains = ["//my/toolchain/type"],
       exec_compatible_with = ["//my/constraint"],
   ),  
 }
)

# use the `copy_from_rule` attribute which will automatically use the toolchains and constraints
# from the rule:
my_rule = rule(
  implementation = _impl,
  toolchains = ["//my/toolchain/type"],
  exec_compatible_with = ["//my/constraint"],
  exec_groups = {
    "foo": exec_group(
      copy_from_rule = true, # it is an error to set this parameter and set toolchains or constraints
    ),  
  }
)

Advantages over original proposal:

• Doesn't imply complicated situations with the word "inheritance"
• Doesn't prevent the exec group "rule" from being created
• Tells exactly what it does.

Disadvantages over original proposal:

• Less extensible if we want to be able to copy from other exec groups in the future. But let's not design for features we don't have a documented need for yet.

ORIGINAL PROPOSAL

Proposal to extend the exec groups API to allow exec groups to inherit requirements from either other exec groups or the rule.

Allowing exec groups to declare "I have the same toolchains and constraints as the rule to which I'm attached" is helpful to create exec groups that are just for setting per-action execution requirements via exec_properties. It will be done via an inherits param to the exec_group constructor method. For example:

# defs.bzl
def _impl(ctx):
  ctx.actions.run(
    exec_group = "foo",
    ...
  )
  ctx.actions.run(
    //no exec_group set
    ...
  )
  ...

my_rule = rule(
  implementation = _impl,
  toolchains = ["//my/toolchain/type"],
  exec_compatible_with = ["//my/constraint"],
  exec_groups = {
   "foo": exec_group(inherits = "rule")   
 }
)

# BUILD
load(":defs.bzl", "my_rule")
my_rule(
  name = "my-target",
  exec_properties = {
    "foo.mem": "20g",
  }
)

This example shows how you can allocate extra (20g) memory to a specific action without allocating it to all the actions. The inherits param to the exec group is a nice convenience so that the exec group doesn't have to copy over the rule's toolchains and constraints. It's more true to what the rule author is trying to express and also isn't liable to people forgetting to update if they're updating the rule's toolchains/constraints.

The inherits param can also take the name of another exec group on the same rule. This will allow exec groups to to resolve to more specific platforms of other exec groups. The list of toolchains for an exec group that inherits from another rule will the be other exec group's toolchains + the toolchains explicitly listed for this exec group.

If the inherits param is given a string other than "rule" (which is a banned exec group name) or another exec group on the same rule, it will error out.

This will help resolve #10799 and replace #10866 with exec groups instead of a brand new attribute. We will implement a "test" exec group that inherits from whatever rule it's on and can be set via the standard exec_properties attr.

[juliexxia]

cc @laurentlb

[juliexxia]

@laurentlb do you have any thoughts here? Would love to address any concerns from the FE team before proceeding.

[juliexxia]

cc @lberki

[lberki]

/cc @alandonovan

Are there any motivating use cases other than #10799 ? (which is kinda weird because test execution doesn't require toolchains, only exec_compatible_with properties)

My first thought is that the addition of this feature would only save passing two arguments because as I understand it, exec groups have two properties: the list of required toolchains and the list of exec_compatible_with labels.

In addition, the semantics are not obvious: if you inherit from an exec group but also define your own toolchains, are they in addition to those of the base exec group? (yes, I know that you specify this above but it's something people need to specifically know).

This would also allow creating inheritance chains of exec groups, which is, whereas benign, is even more conceptual load.

So I'd prefer not to have this unless there are other use cases.

[juliexxia]

Allowing exec groups to declare "I have the same toolchains and constraints as the rule to which I'm attached" is helpful to create exec groups that are just for setting per-action execution requirements via exec_properties

I would consider this the main use case with the test exec properties being a nice example of this. Exec groups were originally built as a response to a (long standing and high priority) request that we give users a way to specify per-target per-action execution properties. The exec group API wasn't originally designed for that, but is a nice way to tie in forge's request to a feature that was already happening to try to consolidate APIs.

Behind the scenes, we're using similar functionality (java method that declares an exec group that inherits from the rule) to declare a cpp_link exec group which has proven really useful already. And I have plans to implement other action-based exec groups that inherit toolchains/constraints from the rule for the major OOM culprits (test actions are actually one of the major sources of internal OOMs already, another example is go_link). So there's internal precedent.

My first thought is that the addition of this feature would only save passing two arguments because as I understand it, exec groups have two properties: the list of required toolchains and the list of exec_compatible_with labels.

This is correct but it also saves having to remember to update in two places if the rule's toolchains/constraints change. And I think it's totally likely/possible that a rule will have multiple exec group that want to inherit from the rule AKA just be used for setting per-action execution properties, not for resolving to a different execution platform.

In addition, the semantics are not obvious: if you inherit from an exec group but also define your own toolchains, are they in addition to those of the base exec group? (yes, I know that you specify this above but it's something people need to specifically know).

I agree it's maybe not obvious but I'd argue it's not un-intuitive. I imagine if the behavior was different, we'd throw an error when setting both. But I hear you. This is potentially something @katre's --toolchain_resolution_debug flag can help with. I also would definitely make this behavior very clear in the documentation and point to --toolchain_resoution_debug as a helpful tool.

This would also allow creating inheritance chains of exec groups, which is, whereas benign, is even more conceptual load.

This is a really good point. I can see this introducing complexity for sure. The inheritance chains would be limited to the scope of a single rule, so at least there's limits to it. We could enforce something like exec groups that depend on other exec groups have to be declared after the exec group they depend on (not sure if it's acceptable to force any ordering in BUILD files). But I think this is an argument for not extending the inherits param to other exec groups. Just allowing inherits = "rule" doesn't open us up to too much.

[juliexxia]

To be clear, the only update I definitely want to make right now is allow inheriting from the rule. I think creating an API to do this that lends itself to an extension down the road that allowing inheritance from other exec groups is a good idea. But I'm not aware of pressing use cases for exec group to exec group inheritance today the way I see it for rule to exec group inheritance.

[katre]

One of the impulses behind adding this is our experience with platforms: initially inheritance wasn't allowed, and users ended up with a lot of workarounds (mostly using ugly macros) to fake it. Finally after investigating #6218 and related issues, we added the feature and it's been very useful for users, and much cleaner in BUILD files. I feel like we're at a similar point with exec groups: uses want some functionality like this, and if we don't add an actual API we'll start seeing lots of ad hoc implementations (probably also using macros).

[lberki]

Thanks for your patience! I'm a little over-booked these days.

Inheriting only from the rule makes the situation much simpler indeed. I don't understand, though, how this would solve #10799 and #10866: my understanding is that the requirement is that they want the exec properties of the test action to be different from the build action, so the syntax exec_properties = { "test.mem": "20G" } is enough, isn't it? Why is inheritance needed in that case?

I thought that exec groups are specifically for actions that do different things. But then what exec properties are envisaged to be common between them?

[juliexxia]

Thanks for your patience! I'm a little over-booked these days.

Very understandable (:

so the syntax exec_properties = { "test.mem": "20G" } is enough, isn't it? Why is inheritance needed in that case?

The inheritance is used to create the "test" exec group during rule definition. This bit:

my_rule = rule(
  implementation = _impl,
  toolchains = ["//my/toolchain/type"],
  exec_compatible_with = ["//my/constraint"],
  exec_groups = {
   "foo": exec_group(inherits = "rule")   
 }
)

Without this bit, exec_properties = { "test.mem": "20G" } won't have an exec group to apply to. The exec properties attr can only reference existing exec groups.

[lberki]

I'm even more confused :( This rule definition doesn't create an exec group called test, it creates one called foo but then how does that make the key test.mem valid? Maybe you wanted to write "foo": exec_group(inherits = "rule")?

The part I don't understand is how inheritance helps with tests. I thought that:

1. Test actions themselves don't need any toolchains
2. Building a test binary and running it generally require different amount of resources, so it's not very useful for one to inherit from the other
3. If you want to put the test action into its own separate exec group, it's better defined in the core of Bazel since that's common to every test rule

But if (1) and (2) are true, what properties should the test exec group inherit from the rule?

[lberki]

To recap the discussion over VC we had with @juliexxia right now: the prior art of the cpp_link exec group using the exec platform of the rule and the testing use case makes it useful to have some sort of way to say "this exec group uses whatever exec platform/target platform/toolchains the rule has", but "inheritance" has connotations of long inheritance chains and subtle behavior. In addition, let's not special-case the "rule" string in exec group names if we can avoid it.

How about an API like this (quick sketch, don't read too much into it): exec_group(uses_rule_target_platform=True) / exec_group(uses_rule_toolchains=True) / exec_group(uses_rule_exec_platform=True) (maybe not with these awkward long names...)

This covers all the use cases (testing + C++ linking) mentioned, I'm somewhat ambivalent about the ability to use the rule toolchains, because it feels like that if you declare an exec group, you should know what you are running in it and the amount of repetition is very little, i.e:

rule(
  toolchains = [":cc_toolchain_type"],
  exec_groups = {
    "cpp_compile": exec_group(rule_platforms=True, rule_toolchains=True),
    "cpp_link": exec_group(rule_platforms=True, rule_toolchains=True),
  }
)

Is not much different from:

rule(
  toolchains = [":cc_toolchain_type"],
  exec_groups = {
    "cpp_compile": exec_group(rule_platforms=True, toolchains=[":cc_toolchain_type"]),
    "cpp_link": exec_group(rule_platforms=True, toolchains=[":cc_toolchain_type"]),
  }
)

And you only realize the savings if the toolchain you want to use in these two actions is the same.

One more piece of food for thought: it appears that the test actions in particular should have a little different platforms than the actions building the binary: e.g. if you cross-compile from x86 to ARM, your compile action has x86 as an exec platform and ARM as the target platform. But presumably since you compile for ARM, you want to run your tests there, so the exec platform of the actual test action should be ARM?

[juliexxia]

Updated the original proposal at the top for visibility and the issue name. The new boolean attribute is called "copy_from_rule", LMK if you have thoughts about that naming.

Briefly discussed with @katre, not currently aware of a need to inherit either the toolchains or the constraints but not both. So in the spirit of not designing without need, I'm going to keep it to a single boolean attribute that inherits both for this proposal.

One more piece of food for thought: it appears that the test actions in particular should have a little different platforms than the actions building the binary: e.g. if you cross-compile from x86 to ARM, your compile action has x86 as an exec platform and ARM as the target platform. But presumably since you compile for ARM, you want to run your tests there, so the exec platform of the actual test action should be ARM?

We actually don't have a way of cross compiling test actions today. Though we've discussed it as a nice to have, we haven't prioritized it yet.

[lberki]

My very slight preference would be to have two Boolean arguments and then raise an error if they are not the same because that would save us a migration if it turns out that we need to inherit toolchains or platforms separately. Maybe use_rule_platforms / use_rule_toolchains if you go that route? (for the single flag, I can't come up with a better name than copy_from_rule :( )

[lberki]

At risk of derailing the thread: what do you mean we don't have a way to cross compile test actions? You can set the exec and target platforms to different things, and if your compiler supports that, you get cross compilation, no extra magic needed.

[katre]

what do you mean we don't have a way to cross compile test actions? You can set the exec and target platforms to different things, and if your compiler supports that, you get cross compilation, no extra magic needed.

Absolutely you can cross-compile test actions. What we don't have currently are any test rules that use multiple execution groups: one to build the test, and another to run it. This should be possible to arrange, but no one's done it yet, and no one is clamoring for it as far as I can tell.

[lberki]

Wasn't the plan to put the test action in a separate execution group so that one can set its resource requirements? (that of course doesn't imply that it has to have a different exec platform, but it certainly makes doing that easier, if need be)

[katre]

Also, back to the actual proposal:

My very slight preference would be to have two Boolean arguments and then raise an error if they are not the same because that would save us a migration if it turns out that we need to inherit toolchains or platforms separately.

Telling rule authors "you have to set this twice, and they have to agree" seems overcomplicated. I don't expect a lot of usage of this: that points to both "it's not hard to ask them to set two things" and "it won't be hard to do a migration later". Given that, I'd go for the option that's simpler for users and only have one flag to set.

[lberki]

What I'm hearing is "YAGNI". And you're right.

[katre]

Wasn't the plan to put the test action in a separate execution group so that one can set its resource requirements?

Yes, and that's being used internally to run tests on bigger workers. We have all the pieces for mixed test execution, we just haven't put them together yet.

[juliexxia]

Agreed, I'd rather design for the need we have today than the need we anticipate tomorrow. So leaving the proposal as is with copy_from_rule.

[juliexxia]

Wasn't the plan to put the test action in a separate execution group so that one can set its resource requirements?

Yes, and that's being used internally to run tests on bigger workers. We have all the pieces for mixed test execution, we just haven't put them together yet.

This actually doesn't happen yet! The creation of that exec group is chained off this change so as soon as it goes in, we'll be able to create the test exec group (and close the loop on the original request #10799). But yes, the plan is to use the test exec group to run tests on bigger workers internally.

[lberki]

Sounds like a plan. Let me review the change.