Skip to content
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

Skylark support for custom configuration transitions #5574

Closed
gregestren opened this issue Jul 11, 2018 · 21 comments
Closed

Skylark support for custom configuration transitions #5574

gregestren opened this issue Jul 11, 2018 · 21 comments
Assignees
Labels
P1 I'll work on this now. (Assignee required) team-Configurability Issues for Configurability team

Comments

@gregestren
Copy link
Contributor

Part of the Skylark build configuration project.

This is a tracking issue for the ability of Skylark writers to trigger configuration transitions, which essentially means having a dependency use different flags than its parent. One common example is building a dep with a different --cpu.

Practically, we may limit (via code restrictions or best practice guidance) initial usage to platform transitions. This is because the platform effort is a complementary project aimed at consolidating --cpu, --crosstool_top, etc. into a a single platform entity. And most configuration transitions in practice trigger those kinds of settings. So we want to be careful about fragmenting the transition space too widely too early.

This complements the Bazel Configurability Roadmap. Write questions / comments / status updates here. See there for the bigger picture.

@kamalmarhubi
Copy link
Contributor

kamalmarhubi commented Oct 14, 2018

@gregestren just checking in after bazelcon: is this the right issue to track for split transitions from starlark?

@gregestren
Copy link
Contributor Author

For splits specifically, #5575 is probably a closer match. I intended #5574 to capture just 1 -> 1 transitions, which is a prerequisite step.

It's ultimately up to @juliexxia. But to be safe, track both? :)

@kamalmarhubi
Copy link
Contributor

Ah excellent. Tracking both, and for good measure scanning things under the configurability category.

@gregestren
Copy link
Contributor Author

Commit: "Introduce the transitions() function and "transition" type in Starlark" (e0efc14, @c-parsons)

bazel-io pushed a commit that referenced this issue Oct 18, 2018
This change enforces that for_analysis_test transitions occur only on attributes of rules with analysis_test=True. This restriction is separate from the whitelist restriction of non-analysis-test transitions.

Progress on #5574 and #6237

RELNOTES: None.
PiperOrigin-RevId: 217782561
bazel-io pushed a commit that referenced this issue Oct 25, 2018
…h //command_line_option.

By enforcing label-like syntax on these options, this will make it easier to migrate these functions to use actual labels when this is implemented.

Progress toward #5574 and #6237

RELNOTES: None.
PiperOrigin-RevId: 218734648
bazel-io pushed a commit that referenced this issue Nov 5, 2018
inputs affect the settings parameter to the transition implementation function; only declared inputs are included in that map.
outputs constrain the settings which may be returned by the transition implementation function; the returned setting keys must exactly match the declared outputs.

Progress toward #5574 and #6237.

RELNOTES: None.
PiperOrigin-RevId: 220111780
bazel-io pushed a commit that referenced this issue Nov 5, 2018
…h attributes which use for_analysis_testing transitions.

Any cases exceeding this limit will result in a rule error being thrown on the analysis_test rule.

The limit is configurable via new flag --analysis_testing_deps_limit with a default limit of 500. The feature itself is still guarded behind --experimental_analysis_testing_improvements, so the former flag has no non-experimental meaning.

Progress toward #5574 and #6237.

RELNOTES: None.
PiperOrigin-RevId: 220144957
bazel-io pushed a commit that referenced this issue Nov 13, 2018
" -- " signifies that all remaining arguments are not options. Split the residue into two lists, one for pre-double dash residue and one for post-double dash residue. In most cases, " -- " isn't used and the entire residue will be in pre-double dash residue. This is helpful for starlark build configurations which currently does starlark options using the residue of native options parsing. With this change, starlark build configurations will know not to try to parse residue after " -- ".

Work towards issue #5574

PiperOrigin-RevId: 221336089
@gregestren gregestren added team-Configurability Issues for Configurability team P1 I'll work on this now. (Assignee required) and removed category: extensibility > configurability labels Nov 28, 2018
@gregestren
Copy link
Contributor Author

Discussion context for parameterized transitions (transitions that change a flag on dependencies but read some attribute on the attached rule to determine what to change to): https://groups.google.com/d/msg/bazel-dev/Lvmz6f_p9jY/XlzyT3T8AwAJ

bazel-io pushed a commit that referenced this issue Dec 1, 2018
This includes the the following features available to native options parsing:
* --(no)boolean_flag syntax
* --flag=value syntax
* --flag value syntax
* single-dash syntax
* for multiple values of same option, last option wins
* internal-only options are treated like they don't exist

There is potential here to combine some of the code in StarlarkOptionsParser.java with OptionsParserImpl.java since they support the same formats of flags. This is complicated by the fact that for native flags, you have a lot of information (type, internal vs external) before parsing whereas for starlark flags, you need to parse out the name first and load it to get this information. So I haven't done the combining in this CL.

This brings up an interesting point that now all commands that use starlark flags will need to do loading. For now we limit starlark parsing to commands that already do building (which is possibly all the commands we care about).

This behavior is still guarded behind --experimental_build_setting_api

Work towards #5574 and #5577

PiperOrigin-RevId: 223600443
bazel-io pushed a commit that referenced this issue Jan 15, 2019
Give each starlark transition a stored event handler. After starlark transitions are applied (after top level transitions and inside configuration resolver), replay and then clear events.

Declare a new abstract class for starlark transitions to consolidate fields/behavior.

Add behavior to ComposingTransitions to allow decomposition. This is helpful here but will also be helpful in future work where we'll need to post-process starlark transitions after they've been applied (e.g. output type verification).

#5574

RELNOTES: None.
PiperOrigin-RevId: 229417148
bazel-io pushed a commit that referenced this issue Jan 16, 2019
… application/validation.

Most of this change is just exception propagation immediately after configuration resolution since we usually wait and realize we've made a mistake later on.

#5574

RELNOTES: None.
PiperOrigin-RevId: 229555753
@gregestren
Copy link
Contributor Author

gregestren commented Jan 28, 2019

2018 EOY update:

This feature is partially done.

  • It's now possible to define custom transitions in Starlark that, for example, change the --cpu for a rule's transitive dependencies.
  • This currently requires `--experimental_starlark_config_transitions. But that flag should go away by the next release or two.
  • This also requires documentation to explain the performance & memory consequences. This can easily make the build graph much larger, and harm action caching & general build speed. So it needs to be used with care.
  • Proper documentation is planned for 2019.
  • Certain kinds of transitions with extra power are still in pre-implementation phase.
  • Transitions over user-defined flags is still in progress.

weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
Give each starlark transition a stored event handler. After starlark transitions are applied (after top level transitions and inside configuration resolver), replay and then clear events.

Declare a new abstract class for starlark transitions to consolidate fields/behavior.

Add behavior to ComposingTransitions to allow decomposition. This is helpful here but will also be helpful in future work where we'll need to post-process starlark transitions after they've been applied (e.g. output type verification).

bazelbuild#5574

RELNOTES: None.
PiperOrigin-RevId: 229417148
weixiao-huang pushed a commit to weixiao-huang/bazel that referenced this issue Jan 31, 2019
… application/validation.

Most of this change is just exception propagation immediately after configuration resolution since we usually wait and realize we've made a mistake later on.

bazelbuild#5574

RELNOTES: None.
PiperOrigin-RevId: 229555753
bazel-io pushed a commit that referenced this issue Apr 9, 2019
Calculate and supply defaults for all read starlark options before transitions are applied. Future work includes:
- combining skyfunction calls for inputs and outputs of starlark transitions
- enforcing that BuildOptions.starlarkOptions will never hold default values (opposite of native options which always hold default values) in order to make BuildConfigurationWithDefault == BuildConfigurationWithNoValue
- Making sure BuildConfiguration properly reflects StarlarkOptions

Work towards #5574, #5577, #5578

RELNOTES: None.
PiperOrigin-RevId: 242681946
bazel-io pushed a commit that referenced this issue Apr 9, 2019
Specifically helpful for --define.

Work towards #5574

RELNOTES: None.
PiperOrigin-RevId: 242736849
bazel-io pushed a commit that referenced this issue Apr 9, 2019
*** Reason for rollback ***

Accidentally added some pending changes from a different client right before submission. Partial rollback to get rid of those.

*** Original change description ***

Allow *allowMultiple* options to be *set* by Starlark transitions.

Specifically helpful for --define.

Work towards #5574

RELNOTES: None.
PiperOrigin-RevId: 242759162
bazel-io pushed a commit that referenced this issue Apr 19, 2019
*** Reason for rollback ***

re-evaluation that --define should probably not be exposed as a native option

*** Original change description ***

Allow *allowMultiple* options to be *set* by Starlark transitions.

Specifically helpful for --define.

Work towards #5574

RELNOTES: None.
PiperOrigin-RevId: 244357629
@gregestren
Copy link
Contributor Author

gregestren commented Apr 26, 2019

April '19 update:

  • --experimental_starlark_config_transitions is no longer required for basic transitions. Some more advanced functionality still needs that flag. The goal is to shift more logic out of that flag and into production transitions incrementally over upcoming months.
  • Documentation still remains a TODO. Not top priority at the moment. Still a priority by end of summer.
  • Transitions on user-defined flags have made strides and work in at least simple form. Still pieces missing, notably selecton user-defined flags, that @juliexxia is vigilantly moving forward.
  • Also good progress recently on reducing needs for --define, which is basically a legacy flag that user-defined flags should ultimately replace and be better at in every way
  • Parameterized transitions work (ones that read attributes), but certain corner cases like attribute-reading rule transitions (my_rule = rule(cfg = _transition_reading_my_rules_attributes)) don't work if the rule has a select.

@juliexxia is at the cutting edge of all this and can correct any parts I got wrong.

@steeve
Copy link
Contributor

steeve commented Jun 19, 2019

hey @gregestren, is there a reason the whitelist (@//tools/whitelists/function_transition_whitelist) is still needed for transition now that the flag is no longer required ?

@gregestren
Copy link
Contributor Author

Hi @steeve - those are somewhat orthogonal.

Non-experimental means the API is stable and functional, and basically usable for production code. But the performance challenges remain real. "Unskilled" use of transitions could blow up build graphs and make builds way more sluggish than expected.

We want to give people the power to evaluate this impact for their own projects, vs. blocking the feature outright until all theoretical scale problems are solved. But since these risks remain we kept the whitelist requirement in case project owners feel the need to lock things down. In other words, the whitelist defaults to everything:

but project owners can further restrict that if they feel that's safer.

It looks like external dependencies may still need whitelist support FYI.

Also, I don't think this is documented anywhere. That should be a TODO.

@gregestren
Copy link
Contributor Author

Regarding:

  • Documentation still remains a TODO. Not top priority at the moment. Still a priority by end of summer.

Starting documentation available at https://docs.bazel.build/versions/master/skylark/config.html.

@steeve
Copy link
Contributor

steeve commented Jun 21, 2019

That is a very nice and detailed explanation, thank you!

@juliexxia
Copy link
Contributor

May '20 update:

Closing this issue as done! 🎉

@oehme
Copy link

oehme commented Jul 31, 2020

The documentation for "configurable build attributes" should be updated now that this is implemented. It currently still says that "config_setting currently only supports built-in Bazel flags". Overall the docs on configurability are in a bit of a confusing state at the moment, leaving me to wonder on what the best practices are and how to use all these new features.

@gregestren
Copy link
Contributor Author

@oehme - can you elaborate more on general docs confusion? Feedback would be enormously helpful. We'd love to clarify best practices where not clear.

@bttk
Copy link
Contributor

bttk commented Jul 31, 2020

What @oehme writes seems right. The statement "config_setting currently only supports built-in Bazel flags" is not true: rule config_setting now has an argument flag_values described as The same as values but for Starlark-defined flags.

Use of flag --define is not recommended, see General Rules doc:

If you need to define conditions that aren't modeled by built-in Bazel flags, use Starlark-defined flags. You can also use --define, but this offers weaker support and is not recommended.

Examples using --define shouldn't be as prominent, and Starlark-defined flags should be preferred. Here is an example use of config_setting: https://docs.bazel.build/versions/master/skylark/config.html#build-settings-and-select

@oehme
Copy link

oehme commented Aug 1, 2020

My main feedback is that --define is still widely used in the docs, even though it is discouraged. Also it's not clear how the new custom configs play together with platforms (if at all). E.g. when would I use a constraint_setting vs config_setting. Some examples (that aren't just using abstract names like "myRule", "myConfig") would go a long way. Maybe I just missed a link to them.

On a more general note, I don't like documentation that says "this will be possible in the future", "this will be added in this PR" (which has since been closed, but without updating the docs) or which presents incomplete new APIs mixed in with the existing ones. I'd much rather have the docs reflect the current best practices and then be updated when new best practices are fully ready. At that point the old practices can be moved to their own "outdated" chapter, so newcomers don't run straight into them.

For context, I'm the kind of person who read's a tool's user guide back-to-back to get an overview of everything that is possible and on what the current best practices are. This is to avoid cargo-culting by just copying what older projects have done. But right now the configurability chapter is a mix of old and new APIs that makes it hard for me to figure out what to do and what to avoid in a new project.

@moroten
Copy link
Contributor

moroten commented Aug 1, 2020

@oehme I asked about constraint_setting vs config_setting on bazel-discuss which includes a guideline from @gregestren:

We generally recommend trying to stay pure to the intention of platforms: something belongs in a platform if running the same build on two different machines might produce different results ("exec platform" differences) or running the same built binary on different machines produces different output ("target platform" differences).

@oehme
Copy link

oehme commented Aug 2, 2020

Just reading this section about platforms and config_settings makes me wonder why everything is specified twice. I.e. why do I create a basalt build_setting and a basalt_platform? Why don't I directly select on the platform or directly specify the build_setting on the command line? What problem does this additional indirection solve? Explaining this with a more elaborate example would probably help newcomers like me understand the concepts better.

Edit: I've finally found the chapter on the new config system much later in the user guide. However, it is still unclear to me how these new build_settings relate to config_settings, constraint_settings and platforms. Are all of these concepts still relevant? When would I use which?

@gregestren
Copy link
Contributor Author

gregestren commented Aug 3, 2020

I've scheduled some time to chat with the main devs of these features this Friday. This is great feedback. There are definitely some steps we can take to address some of these points. In the meantime, some random thoughts:

  • --define is a widely used pattern today whether we like it or not. I think we need to at least acknowledge it in the docs for those who are committed to using it. But you're absolutely right we have examples that lean on it too much, which were written before build settings were available.
  • One impediment to simply deprecating --define today is it's such tight syntax vs. build settings. e.g. --define foo=bar vs. --@some_repo//flag_defs/my_flags:foo=bar. Has anyone used build settings and felt this pain? I'd especially love feedback on this. This is exactly what the shorthand flag aliases project is for. Please weigh in there if that's interesting to you!
  • I hear your point about "best practices" docs vs. "coming in the future" docs. I'll make sure that gets discussed this week.
  • However, there is still some subtlety. Platforms is the most prominent example. We consider the new platforms API current best practices. But there remains a long tail of migration support for the rule sets that defined their own APIs before this one was available. This creates the subtlety that your best practices depend on which rules your org uses & how. The more you depend on legacy APIs the more you'd necessarily need to lean into them. Building with platforms tries to provide a coherent overview of the situation. This is also why we're up-prioritizing just getting those migrations done for the big rule sets (C++, Java) to decisively commit everyone globally to the new API.
  • build_setting, config_setting, constraint_setting, and platform are all distinct concepts that should principally work together in coherent ways. I accept your argument about making this distinction clearer, maybe with some dedicated docs to clarify the difference.

Anyway, again this is all really helpful. And really happy to hear you're not a cargo culter! Cargo culting indeed makes more problems for everyone, and it's such a common pattern to slip into.

@oehme
Copy link

oehme commented Aug 3, 2020

I understand, it's hard to make both new and existing users happy. I think a major problem for me was that the information about these different configuration aspects is spread over several chapters, some of them quite recent, others dated. This makes it hard to get an overview of everything that is there and what each concept is used for. I'd definitely appreciate clarification on the way that build_setting, config_setting, constraint_setting and platform work together. Thanks for taking the time to improve this!

@gregestren
Copy link
Contributor Author

FYI @oehme and others, we have some changes coming through the pipeline to address your doc comments (in #11967).

We probably won't address everything perfectly, but can at least get a few good steps up from the status quo.

I'm still torn on how decisive we should be about discouraging --define, but I would like to offer more decisive opinion on that.

Thanks again for your input.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 I'll work on this now. (Assignee required) team-Configurability Issues for Configurability team
Projects
None yet
Development

No branches or pull requests

8 participants