Starlark Transition Composition

[katre]

We've had native transition composition for a long time, let's make that available to Starlark users!

Design proposal here: https://github.com/bazelbuild/proposals/blob/main/designs/2024-04-16-transition-composition.md

Tagging @gregestren as reviewer, but anyone with comments feel free to contribute.

[fmeum]

I am overall very supportive of this addition, it would help clean up a bunch of indirections required to combine an exec transition with a non-exec transition.

3. The actual configuration change applies the first transition, and then applies the second transition to each split of the result.

What if the second transition in transition1 + transition2 is the split transition? It could make sense to formalize this section with references to the individual transitions in such a composition.

2. The string "exec", for the execution transition on the default exec group.

Now that there is config.exec, I'm not a fan of this kind of syntactic sugar. transition + "exec" looks magical (or even wrong) to me and breaks in a very bad way as soon as transition == "target". It also requires special-casing in LSPs.

[katre]

To clarify further: the semantics of the exec transition are that trans + "exec" would effectively only be the exec transition, because the current exec transition resets almost all flags to stable values.

[fmeum]

Why does transition == "target" break more than any other transition?

If you have some code in Starlark that does transition + "exec" for a transition obtained from elsewhere and someone passes in "target" or "exec", the result will suddenly be a string such as "execexec", not a transition.

[gregestren]

@fmeum which indirections specifically inspired you?

IMO 99% of the conceptual complexity in this proposal is the exec transition integration. We could even do wild things like invoke a build with a custom exec transition that's defined as a composition of trans1 plus the default exec transition. I think these issues scope-bleed into wider philosophy about what we want the exec transition to be.

If the use cases you're thinking about aren't trying to actually "modify" the exec transition but remain clear cases of "apply arbitrary transition B, then apply the exec transition, recognizing these are unrelated transitions and just happen to be sequenced together", that's IMO easier to reason about now.

[katre]

@fmeum You're right , "exec" + "target" would give the wrong result, which is unfortunate.

To clarify, does this mean you support:

1. Don't allow using shorthand names "exec" and "target" with composition (and expose something like config.target to mean the no-op transition)?
2. Support both transition + "exec" and "exec" + transition (but not, sadly, "exec" + "exec")?

[fmeum]

I support 1. I don't know whether we even need the config.target noop, I would consider usages such as transition + config.exec() if transition else config.exec() reasonable as well.

[sluongng]

Nit: I don't like using the plus + operator here as this does not have the commutative property that's often a part of addition.

Could we use a different operator? If it's not allowed in Starlark, I'm ok with a new built-in function as well.

[fmeum]

@katre Sorry, I accidentally edited your comment on mobile. I restored it.

My reply:

I am fine with the method on a transition, but then we should entirely drop support for the legacy string names for clarity.

Do you mean even disallowing cfg = "exec" on rules? That would be very breaking - why would that need to happen as a precondition?

[katre]

No, those we would leave, but for composition you'd need to use an actual transition object. Is that too restrictive?

[fmeum]

No, those we would leave, but for composition you'd need to use an actual transition object. Is that too restrictive?

No, that sounds good!

[gregestren]

Will config.exec() seamlessly work in the implementation? That's an ExcecutionTransitionFactory and transition is a ConfigurationTransitionApi which if I read correctly is a distinct interface/class.

[katre]

It will require some coding to make it work (and either some overloads or a common superinterface) but it should be supportable.

Or we could get rid of transitions and only use transition factories, but that's a fairly big change IIRC.

I'll clarify the spec on this point as part of marking it accepted.

[katre]

Pushed an update to clarify the precise semantics. @fmeum, I know you were interested, let me know if this is more clear.

[fmeum]

I think it is, thanks!

[gregestren]

LGTM on latest revision.

[katre]

More than a year later, an update:

I started trying to implement this last year, and was swamped by the intersection of transition semantics and the underlying Java types used for TransitionFactory.

To recap, the semantics and types involved:

1. An attribute transition can be a patch or split transition, and has access to the target's attribute values (possibly after resolving unrelated configurable attributes via select)
   1. This is represented in Java by the TransitionFactory<AttributeTransitionData> type
2. A rule transition can only be a patch transition, and has access to the targets raw attribute values (with some partial resolution for configurable attributes but not entirely).
   1. This is represented in Java by the TransitionFactory<RuleAttributeTransition> type.
3. A patch transition that does not use target attribute values can be used for either an attribute or a rule.
   1. This is represented in Java by the TransitionFactory<TransitionFactory.Data> type.

Except almost all of those statements are false.

Native transitions use those types, and can correctly distinguish patch transitions from split transitions. Starlark transitions are all instances of StarlarkDefinedConfigTransition, which is then wrapped in a StarlarkAttributeTransitionProvider or StarlarkRuleTransitionProvider when the transition is declared (with attr.label(cfg = transition) or rule(cfg = transition), respectively).

Trying to implement a version of ConfigurationTransitionApi.andThen or StarlarkConfig.compose that handles all of these types and checks the correct semantics became far too complex, and so the work has stalled.

A useful first step to regularize the types involved: trying to express the transition semantics with Java types doesn't work, and so changing everything to use the Starlark API-style distinction between "transition implementation" and "transition usage" is probably worth doing.

I'd like to resume tackling this in the future, but it's fairly unclear when.