Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change uses of SplitTransitionProvider to TransitionFactory. #7833
Change uses of SplitTransitionProvider to TransitionFactory. #7833
Changes from all commits
d5fc444
7521d81
fdb9cf9
5702061
fdc0b38
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: rename
SplitTransitionProviderApi
toTransitionFactoryApi
. After all, the class is not aSplitTransitionProvider
anymore.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave it up to @juliexxia whether to rename, she was discussing a future plan to separate starlark patch and split transitions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is confusing to me too. What's the purpose of SplitTransitionProviderApi? It looks like just an empty interface to expose SplitTransitionProvider to Starlark? And what does it mean to say isSplit() is true below?
My understanding is this may or may not be a split. So for the scenarios where it isn't a split what do those words mean?
(I guess this is mostly a question for @juliexxia).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think how it stands makes sense for this PR. I can always do clean up later.
Currently all Starlark transitions attached to attributes (i.e., StarlarkAttributeTransitionProvider transitions) are natively treated like SplitTransitions. This isn't something that's exposed to the user (i.e. if you return a map instead of a map of maps, that works just fine), just something that was done so we didn't have to handle patch and split transitions here.
SplitTransitionProviderApi was originally introduced for the benefit of exposing MultiArchSplitTransitionProvider to Starlark. That's it's only use case other than this. So SplitTransitionProviderApi is still technically the truth. And TransitionFactory seems to obscure the difference between split and patch transitions - @katre as a catch up question, are those both going to fall under this? I think it's mostly important in regards to the restriction that split transitions can't be attached directly to rules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, TransitionFactory can produce both PatchTransition or SplitTransition. I had considered making it generic in which type it returns (so the signature would become TransitionFactory<PatchTransition, RuleTransitionData>), but
Actual code that cares can check the value of TransitionFactory.isSplit to see if the factory thinks it's returning a SplitTransition or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How is this method used? Like I mentioned above, StarlarkAttributeTransitions are always SplitTransitions according to native code (for now, I suppose this distinction could be made more clear) but might not actually map to multiple output BuildOptions in practice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Callers of TransitionFactory that care whether the transition is a PatchTransition or a SplitTransition can use this to distinguish. It's about the return type and the intent, not on how many actual splits are produced.
Ideally at some point we should unify these further, but I'm not working on that immediately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do these get printed in Starlark with this method removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only starlark transitions need to be printed from Starlark, and they all implement SplitTransitionProviderApi (and so have a repr method).