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

How do I perform BXL analysis over targets with incompatible configurations? #757

Open
cbarrete opened this issue Aug 28, 2024 · 7 comments

Comments

@cbarrete
Copy link
Contributor

Consider a BXL script that needs to both perform actions via ctx.bxl_actions().actions and run analysis via ctx.analysis().
We must enable execution platforms for the actions to work, via -c build.execution_platforms=//my:platform. However, this breaks analysis when the target pattern includes transitioned targets (both via rule transitions and attrs.configured_dep()) or in general when not all targets are compatible with the same configuration. This will look something like this:

    0: Error in configured node dependency, dependency chain follows (-> indicates depends on, ^ indicates same configuration as previous):
              root//:main_target (root//initial_platform#862bbf0260c42c92)
           -> root//:transitioned_target (root//new_platform#3b9c87e21a60f426)
           -> toolchains//:cxx (^)
       
    1: Can't find toolchain_dep execution platform using configuration `<unspecified_exec>`

I have tried setting the target_platform and skip_incompatible parameters to ctx.analysis(), but this made no difference.

Here are my observations:

  • It is possible that specifying all possible platforms as execution platforms would solve this issue, but I see no way to do this, and this would be very inconvenient at best.
  • Regular builds do not have this issue, the execution platform is properly determined for any target.
  • I could somewhat work around this if I could filter out targets based on configuration, but I see no way to do that cleanly. I can match on target names or rule names, but that's not quite enough. Even then, I would like to be able to handle those targets, skipping them is not always an acceptable option. Being able to dynamically set the execution platform based on the target platform would potentially help?

Note that the analysis runs fine when I don't set an execution platform, but again this is not an option in order to use actions. I'm not sure which side of the problem should be "fixed" in this case.

If anyone has experience dealing with such a situtation (BXL analysis over configuration-incompatible targets and running actions in the same script), I'd be very interested in hearing about how they do that.

@cbarrete
Copy link
Contributor Author

What I think was happening is that my toolchains contained selects with dependencies that had configuration constraints that wouldn't match the execution platform I was passing. This was very easy to hit due to transitions and targets with various constraints.

I'm leaving this issue open as it seems that this could be improved with better error messages or, ideally, a way to not have to specify a single execution platform when running BXL scripts.

@stepancheg
Copy link
Contributor

I didn't understand anything, but I'd like to help.

Why don't you enable execution platforms globally for everything? In our internal setup it looks like this:

$ buck2 audit config build.execution_platforms
[build]
    execution_platforms = fbcode//buck2/platform/execution:platforms

And this target lists all possible platforms — linux, mac, windows, different cpus etc.

{
  "fbcode//buck2/platform/execution:platforms": {
    "buck.type": "execution_platforms",
    "buck.package": "fbcode//buck2/platform/execution:TARGETS.v2",
    "name": "platforms",
    "default_target_platform": null,
    "target_compatible_with": [],
    "compatible_with": [],
    "exec_compatible_with": [],
    "visibility": [],
    "within_view": [
      "PUBLIC"
    ],
    "metadata": {},
    "tests": [],
    "fallback": "error",
    "platforms": [
      "fbcode//buck2/platform/execution:fat_mac-arm64_linux",
      "fbcode//buck2/platform/execution:linux-x86_64",
      "fbcode//buck2/platform/execution:linux-x86_64-local-only",
      "fbcode//buck2/platform/execution:linux-x86_64-remote-only",
      "fbcode//buck2/platform/execution:linux-aarch64",

You don't specify -c build.execution_platforms on command line, regardless of whether it is build or BXL.

@cbarrete
Copy link
Contributor Author

Right, sorry about not being clear enough, to be honest I'm not sure I understand everything that is/was going on, but my current mental model is that I got the error I posted because a BXL analysis that I ran with a specific execution platform depended on toolchain dependencies that were incompatible with that target platform. I got rid of the target_compatible_with and the problem went away, at the cost of making e.g. our clang18 dependency not be constrained to the clang18 compiler constraint anymore.

There are a few reasons why we don't set build.execution_platforms:

  • There is no good default, as there is no single platform that all our targets can build with.
  • My understanding was that execution_platforms is like --target-platforms: the name is plural, but we can only use one. It seems like this is incorrect based on what you wrote: what rule/providers do you use to generate such a "platforms list" target? Is this documented somewhere?
  • We'd like to support per-project platforms if possible. This is not a hard requirement, but requiring a centralized list of supported platforms closes some doors.

If it is possible to use multiple execution platforms, how is one picked when running buck2 bxl? Are they tried in order until one matches? If so, this would solve my issue I think, and I could add my constraints back.

I hope that this clarifies things a little, thanks for you help!

@stepancheg
Copy link
Contributor

There are a few reasons why we don't set build.execution_platforms:
There is no good default, as there is no single platform that all our targets can build with

build.execution_platforms is a list. From that list, buck2 picks the first which matches target exec requirement (simplified).

My understanding was that execution_platforms is like --target-platforms: the name is plural, but we can only use one

These are very different.

There's a list of execution platforms, for example, it can be windows then linux (local linux and remote execution windows). In single build, one target can be executed locally on linux, and another one remotely on windows.

what rule/providers do you use to generate such a "platforms list" target? Is this documented somewhere?

There's some documentation there https://buck2.build/docs/rule_authors/configurations/#execution-platform-resolution

One platform is registered with execution_platform rule which returns ExecutionPlatformInfo provider.

build.execution_platforms should point to execution_platforms rule (which is not in prelude, but should be) which should return ExecutionPlatformRegistrationInfo() with a list of platforms.

If you plan to use these, consider buck2 audit execution-platform-resolution //my:target command which helps to debug it.

If it is possible to use multiple execution platforms, how is one picked when running buck2 bxl?

From documentation there:

https://buck2.build/docs/api/bxl/bxl.Context/#bxlcontextbxl_actions

This will have the execution platform resolved according to the execution deps and toolchains you pass into this function.

Are they tried in order until one matches?

Yes.

@cbarrete
Copy link
Contributor Author

I think I see where the disconnect is, it seems like your execution platforms are quite low level (OS/CPU/RBE constraints).
In our case our toolchains have constraints on e.g. which package manager environment needs to be used for the tools that compose the toolchain to be fetched, which in turn means that a "vanilla" execution platforms is incompatible with our toolchains. This blurs the line between our execution and target platforms, something that I think I should try to separate more.
I will try to make a ExecutionPlatformRegistrationInfo that references all those platforms and see if that helps.

@cbarrete
Copy link
Contributor Author

Ok, I did that and it is working as intended. I would like to further untangle our target and execution platforms though: could you explain to me which providers should each provide?

  • Is it correct that target platforms only need to provide an empty DefaultInfo() and a PlatformInfo()?
  • Is it correct that target platforms only need to provide an empty DefaultInfo() and an ExecutionPlatformInfo()? Should there be a PlatformInfo() as well?
  • It seems like the following rule is all that is needed to build an execution platform list, am I missing something (besides maybe an optional fallback attribute, but I'm not sure which type it should have given that the docs say the provider takes a typing.Any)? Do you want me to make a PR to add it to the prelude?
def _execution_platforms_impl(ctx: AnalysisContext) -> list[Provider]:
    return [
        DefaultInfo(),
        ExecutionPlatformRegistrationInfo(platforms = [p[ExecutionPlatformInfo] for p in ctx.attrs.platforms]),
    ]

execution_platforms = rule(
    impl = _execution_platforms_impl,
    attrs = {
        "platforms": attrs.list(attrs.dep(providers = [ExecutionPlatformInfo])),
    },
)

Once I have answers to those, I can make a PR to add this information to the docs: would this be welcome, and if so, can I tag you as reviewer?

Thanks again for your help.

@stepancheg
Copy link
Contributor

stepancheg commented Sep 1, 2024

Is it correct that target platforms only need to provide an empty DefaultInfo() and a PlatformInfo()?

Every rule must provide DefaultInfo. Yes, PlatformInfo is how we configuration is created from target label.

(Although we are slowly migrating to configuration factories/modifiers. In the distant future platform rule may not even exist.)

Is it correct that target platforms only need to provide an empty DefaultInfo() and an ExecutionPlatformInfo()? Should there be a PlatformInfo() as well?

You mean, execution platforms?

Check here an example of execution_platforms rule implementations used in e2e tests of buck2:

https://github.com/facebook/buck2/tree/0877eeac4b423c28dbad0c91be2f7ce104137d8d/tests/e2e_util/nano_prelude

(this is used in e2e tests; e2e tests were exported to GitHub today, but they are not executed on external CI yet).

Actual implementation of execution_platforms we use internally is more complicated. (But we should provide some default implementation in prelude).

Do you want me to make a PR to add it to the prelude?

If you can do it without spending too much time on it, please. Otherwise I'll do it myself.

if so, can I tag you as reviewer?

Yep.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants