-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Support running tests on the target platform #12719
Conversation
This change *includes* another change by @juliexxia that was previously merged and rolled back: bazelbuild@c266ac9 This adds an --use_target_platform_for_tests option that changes tests to use the execution properties from the target platform rather than the host platform. I believe that the code is currently incorrect - if the host and target platforms differ, then tests are cross-compiled for the target platform, but use the execution properties for the host platform. This matters for remote execution, where host and target platform may be different CPU architectures and operating systems (e.g., compiling on x64 Linux and running on ARM64 Mac). Currently, such tests will typically fail to run if they contain architecture or OS-specific code. Progress on bazelbuild#10799. Change-Id: I774bd4442044d6725e78f496b9991368e73ffa00
@juliexxia Would you mind taking a look at this? |
Also, does this need a flag? This looks like a bugfix to me, but it may break users that rely on the legacy behavior. |
Here's an example use case where the legacy behavior is useful and desired: Consider an embedded OS target which could never support running a remote worker on its own, but for which we have an emulator. Then, the "test" could be performed by executing the pure target binary in the emulator. The emulator could be described by a toolchain and have different variants for different execution platforms. In this case we want the execution properties of the host, not the target. |
@pcjanzen, I think your use case works even better with the proposed change. IMO, what you're doing is lying to Bazel and then hoping that everything will work out. To me, that doesn't seem like a promising long-term strategy. For local execution, Bazel could automatically recognize that a given binary requires emulation to run. For example, If you are using remote execution, the remote execution service could be similarly configured, i.e., it could provide x64 machines with arm64 emulators and route arm64 actions to those and start them under an emulator. You could even provide both emulated and actual hardware and use additional per-rule attributes to select which should be used for what, e.g., run unit tests in an emulator but integration tests on actual hardware. |
In our case, our remote execution environment knows nothing of the emulator (or even the target architecture), and we do not want it to; the emulator is provisioned by the client and provided as part of the test action. I'm not sure that I agree that we are lying to Bazel. We use the documented toolchain resolution mechanism to choose a platform to execute the action, and we expect its execution properties to be the ones associated with that platform. If we wanted the RE service to route the test action to a host that could execute it "natively", we would structure the list of execution platforms differently. In other words, we're assuming that Bazel treats test actions exactly the same as build actions. You're saying that test actions are fundamentally different, which is certainly a reasonable argument, but it doesn't mean that we are lying to Bazel; there are lots of places where Bazel treats test actions and build actions similarly. If I understand your proposed change correctly, we would lose the ability to use platform/toolchain resolution to control the execution properties of test actions. This would be unfortunate, because the toolchain mechanism is basically perfectly designed for our use case -- we need to ensure that the emulator binary and the RE properties are aligned. I am not opposed to this change in general, however, please keep it behind a flag, because we rely on the current behavior. |
Ok, let me briefly explain what this change does. Right now, Bazel has a host and target platform; I see these as a model of reality. It uses them to select appropriate toolchains using constraints. In addition, the platforms provide execution properties to be sent to the remote execution system. Note that these properties are completely independent of the constraints. Currently, it always uses the host platform execution properties for all actions (including tests) and ignores any execution properties on the target platform. This means that it is not possible to cross-compile tests and (automatically) run them on appropriate hardware. With this change, Bazel now uses the target platform execution properties for test actions. Note that this has no effect on toolchain resolution, and toolchain resolution has no effect on execution properties. That means you should be able to set the target platform execution properties to the same value as the host platform execution properties, and then this should be a no-op for you. Philosophically, I think that's not a good approach because it introduces inconsistent platform views between Bazel and the remote execution system. I believe that this inconsistency is likely to bite you in the future, maybe not soon, but eventually. For example, if you ever want to run tests on actual hardware rather than on an emulator, your current approach won't work - by design. If you're using |
That's a bit of an oversimplification, isn't it? Bazel has one target platform, and then a list of N execution platforms. The host platform is always appended to the end of the list of execution platforms, so it's the one that happens to get used if you don't use "register_execution_platforms" or "--extra_execution_platforms". For each target, Bazel uses the target's exec_compatible_with attribute and its toolchains to select an execution platform. It's the execution properties of the selected execution platform that get sent to RE, for both build and test actions. This behavior is useful even in the non-cross-compiling case. In any given build, we might have half a dozen different execution platforms: one might have extra memory, another might have GPU hardware, etc. Some of these constraints might be irrelevant for build actions, but necessary to execute some subset of the build's tests. We cannot just set the target platform properties to the execution platform properties, because there's only one target platform, and many (possibly conflicting) execution platforms. |
My understanding is that Bazel selects one platform for each configured target, and within the scope of that target, that becomes the 'target platform'. Hopefully, @juliexxia can clear this up if that's not correct. Yes, different targets may have different platforms, and - in fact - you can also add or override execution properties for each target and (with this change) also for subsets of actions within the target (action groups). |
Maybe we're using different terminology? My understanding is that the 'target platform' is part of the configuration. It starts off with the argument to '--platforms' and thereafter, the only way to change it is to take an explicit configuration transition. Then, for each configured target, Bazel selects one platform to be the 'execution platform.' The execution platform is therefore much finer-grained than the target platform. With the addition of exec groups, it can become finer still - there can be different execution platforms for different actions created by the target. But in any case, the execution platform is selected from the list of registered execution platforms, using the toolchain/exec_compatible_with/registered_execution_platforms dance. |
Hi @katre, it would be great if we could move this forward. There's been a long discussion about exec platforms above, maybe you can clear up our confusion here. |
Reading the history of both changes and the discussion to get current on this. |
Okay, first some general background statements to clear up this thread:
Also, we have a currently proposal to unify execution strategy selection with toolchain resolution. When this is implemented, the execution platform will be easier to keep in sync with the execution strategy, especially for remote execution. Unfortunately, we are short on engineer time to make this happen. |
To get to this actual proposal, using the target platform to execute tests seems incorrect to me, but becomes tricky due to how tests work. As an example, let's consider an The easiest way to consider this is that the build phase (which corresponds to the default action group) takes one execution platform, and then test execution (which uses the |
Bazel itself never executes anything on the target platform ( |
Final comment (thanks for everyone's patience): there is a clear need to set the execution properties of the platform that tests are executed on. With @juliexxia's change, that is the execution platform of the My understanding is that, once @juliexxia's change is in place, it will be possible to add new execution properties to a test target like so:
We could also fairly simply extend the current support for
This would allow users to add an attribute to tests in the BUILD file and influence which execution platforms are available for the test execution group, similarly to how the (Also, I just noticed that I have been saying "action groups" when I mean to say "execution groups". I appear to have been using outdated terminology). Final doc links: |
@katre, thanks for the very detailed explanation! This sounds like what I've been trying to achieve in quval@af02b71 (we've actually been using a version that has this patch for a few weeks now - it looks pretty much like what you outlined). Could you please take a look at that change as well? Edit: just to be precise - it doesn't allow setting constraints per execution group on a target, and |
I am 100% in support of the proposed extension to The thing that is tricky is that all execution groups created by If I'm writing my own rule suite in starlark, I can do something like
or at least I could, if bazel magically assigned every test action to the exec group named "test", if it was defined by the corresponding rule. This would allow me to do my compilation actions on the execution platform for the compiler, and the test actions on the target platform. Extending this concept to native rules is trickier. Is there one global test_toolchain_type, or does every rule-suite need to define its own? Would bazel somehow synthesize default test toolchains? |
The intent is for both native and Starlark tests to be updated to assign test actions to the test action group. This is hard to automate but should be possible to update manually (if rather extensive). Instead of using a test toolchain, it's possible to directly specify
Adding a way to do this on a per-target basis is possible but we need to work out the best syntax. |
In this case:
then wouldn't I have to register a toolchain of Requiring exec_compatible_with as an attr at rule-definition time might be OK for some use cases, but it makes it really hard to write a rule that might be used in multiple target configurations. As a rule author, I don't want to have to write "foo_linux_test" and "foo_macos_test" and "foo_windows_test". Having a separate toolchain type for test actions is one idea that would solve both of these problems, but there are probably others. |
@pcjanzen: Yes, if the It only rarely makes sense to have OS-type execution constraints on a rule, unless that rule for some reason is only compatible with a given OS. Some of the macOS tooling might fit that, and might not. Many of these are probably better expressed using toolchains (and then only defining toolchains for platforms where they actually exist). |
@quval: Sorry I hadn't noticed and replied to that earlier. Could you please send that as a separate PR, with an example of how it would be used (ie, what a platform with exec groups would look like, and how that would translate into remote exec requests)? Just looking at the commit it's hard for me to see how this actually works, the examples will help clarify it for me. |
When computing exec properties from the execution platform for an action, take into account only the properties that are relevant to the action's exec groups. In particular, allow setting exec properties for arbitrary exec groups on platforms. Previously, any such properties were rejected. With this change, the following becomes possible: ``` cc_test( name = "my_test", ..., exec_properties = { "test.key": "value", }, ) ``` This will apply `{"key": "value"}` for the test-runner action only (i.e., compilation and linkage won't be affected). The following also becomes possible: ``` platform( name = "test_platform", constraint_values = [":test_constraint"], exec_properties = { "test.key": "value", }, ) cc_test( name = "my_test", ..., exec_compatible_with = [":test_constraint"], ) ``` This achieves the same in a more succinct way. For related discussion, see PR #12719 by @ulfjack. Closes #13110. PiperOrigin-RevId: 361167318
The use case I'm looking at is where a Annotating tests individually is not an option; even ignoring the amount of manual labor required, it seems like that would require duplicating all It may be that we need a mechanism to annotate in the rule whether it's using an emulator or not? If not, Bazel could automatically use the target platform for the exec platform of the test exec group? |
I had a look at #13110 and that seems only tangentially related to my use case. It may provide a workaround for running tests on another platform by explicitly setting "test.key" settings on the general exec platform. However, this still breaks if a build has both cross-compiled and emulated tests in the same invocation. I understand that this generally won't work on a single machine, but it is a concern for general remote execution. It seems to me that cross-compiled tests should use the target platform as their exec platform, for rules that don't use emulators (like |
The solution isn't to special case tests to use the target platform as the execution platform, but for tests to use multiple exec groups, to have the correct execution platform for building and executing the test. |
I don't care all that much about how you want to call these things. What I do care about is this: |
When computing exec properties from the execution platform for an action, take into account only the properties that are relevant to the action's exec groups. In particular, allow setting exec properties for arbitrary exec groups on platforms. Previously, any such properties were rejected. With this change, the following becomes possible: ``` cc_test( name = "my_test", ..., exec_properties = { "test.key": "value", }, ) ``` This will apply `{"key": "value"}` for the test-runner action only (i.e., compilation and linkage won't be affected). The following also becomes possible: ``` platform( name = "test_platform", constraint_values = [":test_constraint"], exec_properties = { "test.key": "value", }, ) cc_test( name = "my_test", ..., exec_compatible_with = [":test_constraint"], ) ``` This achieves the same in a more succinct way. For related discussion, see PR #12719 by @ulfjack. Closes #13110. PiperOrigin-RevId: 361167318
When computing exec properties from the execution platform for an action, take into account only the properties that are relevant to the action's exec groups. In particular, allow setting exec properties for arbitrary exec groups on platforms. Previously, any such properties were rejected. With this change, the following becomes possible: ``` cc_test( name = "my_test", ..., exec_properties = { "test.key": "value", }, ) ``` This will apply `{"key": "value"}` for the test-runner action only (i.e., compilation and linkage won't be affected). The following also becomes possible: ``` platform( name = "test_platform", constraint_values = [":test_constraint"], exec_properties = { "test.key": "value", }, ) cc_test( name = "my_test", ..., exec_compatible_with = [":test_constraint"], ) ``` This achieves the same in a more succinct way. For related discussion, see PR #12719 by @ulfjack. Closes #13110. PiperOrigin-RevId: 361167318
We cherry-picked this into our Bazel and were able to run aarch64 tests from an amd64 machine very nicely using remote execution. Thanks @ulfjack ! I've been wanting to do this for years, very exciting to see it actually work. |
When computing exec properties from the execution platform for an action, take into account only the properties that are relevant to the action's exec groups. In particular, allow setting exec properties for arbitrary exec groups on platforms. Previously, any such properties were rejected. With this change, the following becomes possible: ``` cc_test( name = "my_test", ..., exec_properties = { "test.key": "value", }, ) ``` This will apply `{"key": "value"}` for the test-runner action only (i.e., compilation and linkage won't be affected). The following also becomes possible: ``` platform( name = "test_platform", constraint_values = [":test_constraint"], exec_properties = { "test.key": "value", }, ) cc_test( name = "my_test", ..., exec_compatible_with = [":test_constraint"], ) ``` This achieves the same in a more succinct way. For related discussion, see PR #12719 by @ulfjack. Closes #13110. PiperOrigin-RevId: 361167318
When computing exec properties from the execution platform for an action, take into account only the properties that are relevant to the action's exec groups. In particular, allow setting exec properties for arbitrary exec groups on platforms. Previously, any such properties were rejected. With this change, the following becomes possible: ``` cc_test( name = "my_test", ..., exec_properties = { "test.key": "value", }, ) ``` This will apply `{"key": "value"}` for the test-runner action only (i.e., compilation and linkage won't be affected). The following also becomes possible: ``` platform( name = "test_platform", constraint_values = [":test_constraint"], exec_properties = { "test.key": "value", }, ) cc_test( name = "my_test", ..., exec_compatible_with = [":test_constraint"], ) ``` This achieves the same in a more succinct way. For related discussion, see PR bazelbuild#12719 by @ulfjack. Closes bazelbuild#13110. PiperOrigin-RevId: 361167318
defaultValue = "false", | ||
documentationCategory = OptionDocumentationCategory.EXECUTION_STRATEGY, | ||
effectTags = {OptionEffectTag.EXECUTION}, | ||
help = "If true, then Bazel will run coverage postprocessing for test in a new spawn.") |
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.
Is it possible that this help
is a copy-paste artifact? It looks to me in need of updating.
What's the current status of this PR? It is very out of date and needs to be synced with the main branch. Specifically, most of the Also, I'm not sure that my earlier feedback has been incorporated. Specifically, I am not convinced that the target platform is appropriate for executing the test. In simple cases this is fine, but as an example, when running an Android test the target platform is an Android device, but the actual test is executed either on a real device or an emulator. In either of these cases, the actual test action can't just execute the APK: either an adb command is executed to send the APK to the real device, or the emulator is started and the APK is sent to that. I feel that the test exec group needs to be used to execute test actions (that is the point of exec groups in general), and what we really need is better control over what the execution platform for the test exec group is. |
I'm also looking at this patch with interest. My use case is similar to the author's: cross compiling on an x86 host architecture for an aarch64 target architecture with both x86 and aarch64 execution platforms available for remote execution. I would like to specifically avoid adding target-level exec_properties to every test as that implies that ever test must have knowledge of all toolchains and platforms it's compatible with. @katre I think your example is a good one, but conceptually it seems like In the absence of a flag such as |
Hi there! Thank you for contributing to the Bazel repository. We appreciate your time and effort. We're doing a clean up of old PRs and will be closing this one since it seems to have stalled. Please feel free to reopen/let us know if you’re still interested in pursuing this or if you'd like to discuss anything further. We’ll respond as soon as we have the bandwidth/resources to do so. |
This change includes another change by @juliexxia that was previously
merged and rolled back:
c266ac9
This adds an --use_target_platform_for_tests option that changes tests
to use the execution properties from the target platform rather than
the host platform. I believe that the code is currently incorrect -
if the host and target platforms differ, then tests are cross-compiled
for the target platform, but use the execution properties for the host
platform.
This matters for remote execution, where host and target platform may
be different CPU architectures and operating systems (e.g., compiling
on x64 Linux and running on ARM64 Mac). Currently, such tests will
typically fail to run if they contain architecture or OS-specific code.
Progress on #10799.
Change-Id: I774bd4442044d6725e78f496b9991368e73ffa00