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

test_exec_properties differentiation #10799

Closed
werkt opened this issue Feb 15, 2020 · 33 comments
Closed

test_exec_properties differentiation #10799

werkt opened this issue Feb 15, 2020 · 33 comments
Assignees
Labels
P1 I'll work on this now. (Assignee required) team-Configurability platforms, toolchains, cquery, select(), config transitions type: feature request

Comments

@werkt
Copy link
Contributor

werkt commented Feb 15, 2020

Description of the problem / feature request:

Provide a test_exec_properties option for *_test rules.

Feature requests: what underlying problem are you trying to solve with this feature?

The test execution platform properties should be differentiated from exec_properties. Currently setting an exec_properties of a *_test will apply to the actions used to build the target, as well as the test itself. In order to apply specific runtime configurations to the platform of the test, but not, say, the compiles of a cc_test, a different rule attribute is required.

@aiuto aiuto added team-Configurability platforms, toolchains, cquery, select(), config transitions untriaged labels Feb 18, 2020
@jlisee
Copy link
Contributor

jlisee commented Feb 28, 2020

This would be a really great feature to have. Some example "user stories":

  • Alice wants to say this test needs 16 cores and 64GB of RAM, but allow the compilations of the 10 files in the test to use only a single core and 8GB of RAM. This greatly reduces the resources needed for the test.

  • John wants to says this test needs a GPU and 128GB RAM but not consume GPU resources when build the files for that test

Without this level of control we are blocking on fully leveraging a large shared Buildfarm cluster. To do this we need to give each job just the resources it needs and those needs widely vary. With wide ranges in needed cores, memory, wall clock time, network, GPU requirements, and overall duration.

@katre
Copy link
Member

katre commented Mar 3, 2020

Taking a look at this (and the PR #10866), I think this is strongly related to the proposal for Action Groups. Action groups would allow test rules to declare that the test execution actions are in a separate, named group, and then targets can use the proposed syntax:

my_test(
  ...
  exec_properties = {
    "test": {
      "cores": "16",
      "ram": "64GB",
    },
  },
)

The proposal itself doesn't mention having all tests create this action group, but I think that would be a reasonable thing to implement once the functionality is available, and doesn't require either a) a new globally-available attribute, or b) more target-specific knowledge baked into Bazel's core.

(Pinging @juliexxia to make sure we are considering this use for Action Groups).

@werkt
Copy link
Contributor Author

werkt commented Mar 3, 2020

I'm just stuck on not being able to do this from the target itself without the action group - exec_properties as identified here should have the same principles as those in rule-base, it could even be left backwards compatible with dict detection in values allowing the new behavior.

What is the ETA on ActionGroups? That along with the above is what matters to me.

@juliexxia
Copy link
Contributor

ActionGroups is in the design review stage now and will hopefully be approved and ready for implementation by early next week. It's a P0 OKR for Q1 but since we only have a month of Q1 left I wouldn't be surprised if some work bled into Q2.

@katre are you proposing that all test rules automatically have the "test" action group? That seems reasonable to me

@juliexxia
Copy link
Contributor

Added a note about this to the 'Potential Next Steps' section of the action groups doc: https://docs.google.com/document/d/1m9xLRLg09lncQTuBUMhhG_lqDQGJRYMZPejvwdBXvqo/edit#bookmark=id.nh45ww714xso

@brandjon
Copy link
Member

If we're going to have a special action group for "test" actions, does that mean we should also have a special action group for "run" actions, used when you do a bazel run command? And would they be equivalent for test actions?

@brandjon
Copy link
Member

Also it's not clear to me what the execution platform for a "run" or "test" action group would be. By definition, wouldn't it have to be the same as the target platform of that target? Likewise, do toolchains for "run" or "test" action groups make sense? If these concepts do not map well, it may be better to think of "run"/"test" as just things that exec properties can apply to, and not as proper action groups.

@katre
Copy link
Member

katre commented Mar 13, 2020

"Run" is a special case: it's not technically an action generated by a rule, it's Bazel executing the output from a build step.

"Test" actions are a separate issue: Bazel first builds the test executable, then executes it, then examines the output and reports on it.

@brandjon
Copy link
Member

From a user's POV is there any reason to call one an action and not the other? Or is this just an implementation difference?

If they (or just "Test") are actions, do execution platforms and toolchains make sense for them, or only execution properties?

If "Run" is not an "action", can execution properties apply to it nonetheless?

@katre
Copy link
Member

katre commented Mar 13, 2020

"Run" always means "run on the host platform". Ideally we'd change the default of the "--platforms" flag to make that more true, but for now there's a chance of mismatch.

"Test" is trickier: you're right that it's not technically an action, but there are several requests to manage test execution with platforms: using a platform to describe the remote platform where a test executes, or an android emulator that runs the test process, for example. Setting specific execution properties for a test for another.

@brandjon
Copy link
Member

In that case I suggest that "Run" and/or "Test" not be considered "action groups", even if they share some common subset of features with action groups -- "things that can have execution properties" or what not.

That said, maybe we want to make "run"/"test" reserved names from action groups, so that they can be used in execution properties dictionaries. (Of course, that complicates any code that needs to introspect the execution properties dict and treat the keys like action groups, kinda like how code that iterates over struct fields must discard to_json and to_proto.)

@werkt
Copy link
Contributor Author

werkt commented Apr 28, 2020

So how's ActionGroups going? is there a tracking issue proper for it, or is this it?

@katre
Copy link
Member

katre commented Apr 28, 2020

@juliexxia has been working on it, I'll let her update. @juliexxia, is there a Github tracking issue?

@juliexxia
Copy link
Contributor

Newly minted as of two seconds ago: #11250

I back-added some existing commits that are contributing work and I'll attach the issue to commits going forward.

@juliexxia
Copy link
Contributor

juliexxia commented Aug 19, 2020

@werkt Thanks so much for this PR and apologies it's taken so long to get you and update. Now that native exec groups are fully landed I'm in the process of adding an exec group for TestRunnerActions that can be used something like:

some_test_rule(
  name = "my-test",
  exec_properties = {
    "property-a": "value-a"
    "test.property-b": "value-b",
  },
  ...
)

All actions produced by the test rule will see the exec properties dict {"property-a", "value-a"} and TestRunnerActions will see {"property-a": "value-a", "property-b": "value-b"}.

I think that should cover your use case (your PR was particularly helpful for helping me understand it) but let me know if it doesn't sound like it will!

@jlisee
Copy link
Contributor

jlisee commented Sep 2, 2020

@juliexxia what version of Bazel supports the test.property-b syntax for exec_properties (or what is the target release)?

Also an aside: does the exec groups system allow me to apply tags like no-remote only to the compile or link actions of a cc_compile target for example?

@juliexxia
Copy link
Contributor

@jlisee, the test.property-b syntax isn't released yet. It's in development, blocked by #12006 which I'm waiting on comments from from the bazel starlark team.

re: no-remote, etc. No, not yet. But that's in the pipeline for the coming quarter or so. The plan is to deprecate that functionality from tags and add it to exec_properties. Doc is in the works.

@ulfjack
Copy link
Contributor

ulfjack commented Sep 16, 2020

We have a use case where we want to run tests on a different architecture from builds. Ideally, this would not require annotating every test rule individually, for example, this could be inferred from the target platform (does such a thing exist?). As far as I can tell from the source code, there is no working mechanism for this right now. I'd also be happy to file this as a separate bug - it might be sufficiently different from this proposal.

bazel-io pushed a commit that referenced this issue Nov 2, 2020
…er exec groups

Add a "test" exec group for TestRunnerActions. This will allow users to set {"test.key", "value"} inside their exec properties and {"key", "value"} will propagate as to just TestRunnerActions.

This addresses user request #10799

PiperOrigin-RevId: 340301230
@juliexxia
Copy link
Contributor

When c266ac9 is released, prefixing exec_properties entries with test. will apply the property to test actions only e.g.

exec_properties = {
    "test.mem": "16g", 
    "mem": "12g",
}

Test actions would see the exec properties dict {"mem": "16g"} and other actions would see the dict {"mem": "12g"}

@werkt
Copy link
Contributor Author

werkt commented Nov 3, 2020

Nice job on this. I will take a look at the capabilities, as I'm curious of the extents of this feature from the starlark side - I hope that more than just 'test' is available

bazel-io pushed a commit that referenced this issue Nov 4, 2020
*** Reason for rollback ***

Broke Bazel Toolchain on RBE
https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1743#1dba3b6d-8ea5-4efc-b696-f48e62b035fa

*** Original change description ***

Work towards #12006 Allow exec groups to inherit from the rule or other exec groups

Add a "test" exec group for TestRunnerActions. This will allow users to set {"test.key", "value"} inside their exec properties and {"key", "value"} will propagate as to just TestRunnerActions.

This addresses user request #10799

PiperOrigin-RevId: 340635429
@ulfjack
Copy link
Contributor

ulfjack commented Nov 6, 2020

It looks like this was rolled back. The linked error logs indicate a failure in curl, and I don't see how that could be related.

bazel-io pushed a commit that referenced this issue Mar 1, 2021
Work towards #12006.

Add a "test" exec group for TestRunnerActions. This will allow users to set {"test.key", "value"} inside their exec properties and {"key", "value"} will propagate as to just TestRunnerActions.

This addresses user request #10799

This is a rollforward of c1ae939, which
was reverted in c266ac9.

Closes #13119.

PiperOrigin-RevId: 360168649
philwo pushed a commit that referenced this issue Mar 15, 2021
Work towards #12006.

Add a "test" exec group for TestRunnerActions. This will allow users to set {"test.key", "value"} inside their exec properties and {"key", "value"} will propagate as to just TestRunnerActions.

This addresses user request #10799

This is a rollforward of c1ae939, which
was reverted in c266ac9.

Closes #13119.

PiperOrigin-RevId: 360168649
philwo pushed a commit that referenced this issue Mar 15, 2021
Work towards #12006.

Add a "test" exec group for TestRunnerActions. This will allow users to set {"test.key", "value"} inside their exec properties and {"key", "value"} will propagate as to just TestRunnerActions.

This addresses user request #10799

This is a rollforward of c1ae939, which
was reverted in c266ac9.

Closes #13119.

PiperOrigin-RevId: 360168649
katre added a commit that referenced this issue Jul 12, 2021
Work towards #12006.

Add a "test" exec group for TestRunnerActions. This will allow users to set {"test.key", "value"} inside their exec properties and {"key", "value"} will propagate as to just TestRunnerActions.

This addresses user request #10799

This is a rollforward of c1ae939, which
was reverted in c266ac9.

Closes #13119.

PiperOrigin-RevId: 360168649
katre added a commit to katre/bazel that referenced this issue Jul 13, 2021
Work towards bazelbuild#12006.

Add a "test" exec group for TestRunnerActions. This will allow users to set {"test.key", "value"} inside their exec properties and {"key", "value"} will propagate as to just TestRunnerActions.

This addresses user request bazelbuild#10799

This is a rollforward of c1ae939, which
was reverted in c266ac9.

Closes bazelbuild#13119.

PiperOrigin-RevId: 360168649
@EdSchouten
Copy link
Contributor

This can be closed, right? You can nowadays use exec_properties = {"test.foo": "bar"} to set properties on the test exec group.

luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
    *** Reason for rollback ***

    Broke Bazel Toolchain on RBE
    https://buildkite.com/bazel/bazel-at-head-plus-downstream/builds/1743#1dba3b6d-8ea5-4efc-b696-f48e62b035fa

    *** Original change description ***

    Work towards #12006 Allow exec groups to inherit from the rule or other exec groups

    Add a "test" exec group for TestRunnerActions. This will allow users to set {"test.key", "value"} inside their exec properties and {"key", "value"} will propagate as to just TestRunnerActions.

    This addresses user request bazelbuild/bazel#10799

    PiperOrigin-RevId: 340635429
luca-digrazia pushed a commit to luca-digrazia/DatasetCommitsDiffSearch that referenced this issue Sep 4, 2022
…her exec groups

    Add a "test" exec group for TestRunnerActions. This will allow users to set {"test.key", "value"} inside their exec properties and {"key", "value"} will propagate as to just TestRunnerActions.

    This addresses user request bazelbuild/bazel#10799

    PiperOrigin-RevId: 340301230
AustinSchuh added a commit to AustinSchuh/bazel that referenced this issue Feb 2, 2023
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.

Based on work by Ulf Adams <[email protected]> and updated by
[email protected]

Progress on bazelbuild#10799.
AustinSchuh added a commit to AustinSchuh/bazel that referenced this issue Feb 3, 2023
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.

Based on work by Ulf Adams <[email protected]> and updated by
[email protected]

Progress on bazelbuild#10799.
AustinSchuh added a commit to AustinSchuh/bazel that referenced this issue Feb 3, 2023
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.

Based on work by Ulf Adams <[email protected]> and updated by
[email protected]

Progress on bazelbuild#10799.
katre pushed a commit to katre/bazel that referenced this issue Feb 6, 2023
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.

Based on work by Ulf Adams <[email protected]> and updated by
[email protected]

Progress on bazelbuild#10799.
AustinSchuh added a commit to AustinSchuh/bazel that referenced this issue Feb 10, 2023
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.

Based on work by Ulf Adams <[email protected]> and updated by
[email protected]

Progress on bazelbuild#10799.

RELNOTES: Add --use_target_platform_for_tests which uses the target platform for executing tests instead of the execution platform.
copybara-service bot pushed a commit that referenced this issue Feb 15, 2023
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.

Based on work by Ulf Adams <[email protected]> and updated by [email protected]

Progress on #10799.

RELNOTES: Add --use_target_platform_for_tests which uses the target platform for executing tests instead of the execution platform.

Closes #17390.

PiperOrigin-RevId: 509734553
Change-Id: I889f2b1322564298ef4145562afc47623e2c2745
jerrymarino added a commit to bazel-ios/rules_ios that referenced this issue Feb 24, 2023
Thank to bazelbuild/bazel#10799 we now have
`exec_properties` and the tests are missing `exec_properties`, so set
them. If you have different exec properties for inputs, you can set them
with `exec_properties`

Conditional to not expand the size of the action graph
@jerrymarino
Copy link
Contributor

Overall this works quite well for me - thanks for fixing 👍 !

jerrymarino added a commit to bazel-ios/rules_ios that referenced this issue Feb 24, 2023
Thank to bazelbuild/bazel#10799 we now have
`exec_properties` and the tests are missing `exec_properties`, so set
them. If you have different exec properties for inputs, you can set them
with `exec_properties`

Conditional to not expand the size of the action graph
jerrymarino added a commit to bazel-ios/rules_ios that referenced this issue Feb 24, 2023
Thank to bazelbuild/bazel#10799 we now have
`exec_properties` and the tests are missing `exec_properties`, so set
them. If you have different exec properties for inputs, you can set them
with `exec_properties`

Conditional to not expand the size of the action graph
jerrymarino added a commit to bazel-ios/rules_ios that referenced this issue Feb 24, 2023
Thank to bazelbuild/bazel#10799 we now have
`exec_properties` and the tests are missing `exec_properties`, so set
them. If you have different exec properties for inputs, you can set them
with `exec_properties`

Conditional to not expand the size of the action graph
@werkt
Copy link
Contributor Author

werkt commented Mar 6, 2024

Considering this completed, docs are available at https://bazel.build/extending/exec-groups (not sure if this was ever linked)

@werkt werkt closed this as completed Mar 6, 2024
@anguslees
Copy link

re: no-remote, etc. No, not yet. But that's in the pipeline for the coming quarter or so. The plan is to deprecate that functionality from tags and add it to exec_properties. Doc is in the works.

Did this ever get implemented? I'm trying exec_propertes = {"test.no-remote-exec": "true"} and it seems no-remote-exec is (incorrectly) sent to my remote platform, which then chokes. (Bazel 7.2.0)

@katre
Copy link
Member

katre commented Jul 24, 2024

@anguslees: You should be able to use exec_properties like this: setting test.no-remote-exec should only apply no-remote-exec to the actions in the test exec group (which is the actual test execution).

It's possible that there are bugs here: the overall project to merge tags, execution requirements, and execution properties stalled (mostly because it's complex, no one has time to take it on, and dealing with cleanups in existing codebases is a long slog).

If this is not working properly, please file a new issue with a reproduction and we'll try to get someone to take a look.

nataliejameson pushed a commit to discord/rules_ios that referenced this issue Aug 13, 2024
Thank to bazelbuild/bazel#10799 we now have
`exec_properties` and the tests are missing `exec_properties`, so set
them. If you have different exec properties for inputs, you can set them
with `exec_properties`

Conditional to not expand the size of the action graph
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 platforms, toolchains, cquery, select(), config transitions type: feature request
Projects
None yet
Development

Successfully merging a pull request may close this issue.