tags propagation: Starlark rules part #8612

ishikhman · 2019-06-12T13:42:14Z

Tags declared on targets are not propagated to actions and therefore are not taken into consideration by bazel. This causes some issues, for instance, target marked with a tag 'no-remote' will still be executed remotely.
As it was agreed in the design doc (see doc and #7766 for details), set of tags to be propagated to actions as a first iteration.
This change is responsible for that first step for the Starlark Rules.

RELNOTES: tags 'no-remote', 'no-cache', 'no-remote-cache', 'no-remote-exec', 'no-sandbox' are propagated now to the actions from targets.

Closes #7766

Tags declared on targets are not propagated to actions and therefore are not taken into consideration by bazel. This causes some issues, for instance, target marked with a tag 'no-remote' will still be executed remotely. As it was agreed in the design doc (see bazelbuild#7766 for a link), set of tags to be propagated to actions as a first iteration. This change is responsible for that first step for the Starlark Rules. RELNOTES: tags 'no-remote', 'no-cache', 'no-remote-cache', 'no-remote-exec', 'no-sandbox' are propagated now to the actions from targets. Closes bazelbuild#7766

buchgr

I am not a fan of white listing tags. It's error prone. Couldn't we apply the same filter that we apply to execution_info also to tags? What's the downside?

buchgr · 2019-06-13T08:38:30Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+  // we do not want to propagate custom user's tags or create potential conflicts
+  // with execution requirements declared on the rules.
+  // See https://github.com/bazelbuild/bazel/issues/7766 for details.
+  private static final Predicate<String> TAGS_PROPAGATED_TO_EXEC_INFO =


how is this compatible with LEGAL_EXEC_INFO_KEYS defined a few lines above, which also filters exec requirements and the comment says "We also don't want to exhaustively enumerate all the legal values here.". Couldn't we unify these two predicates?

we could, but this is not what we've agreed on in the initial design.

Independent what's in the design doc, let's not do it if it makes no sense? :)

I think it makes sense to keep them separate because they serve different purposes: LEGAL_EXEC_INFO_KEYS specifies the universe of execution info keys, and TAGS_PROPAGATED_TO_EXEC_INFO specifies the specific tags that can be propagated.

TAGS_PROPAGATED_TO_EXEC_INFO is a subset of LEGAL_EXEC_INFO_KEYS

In getExecutionInfoFromTags, we only propagate tags that satisfy TAGS_PROPAGATED_TO_EXEC_INFO, irrespective of whether they satisfy LEGAL_EXEC_INFO_KEYS; I don't see how we'd do this check if the two predicates were combined.

buchgr · 2019-06-13T08:39:32Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+          || tag.equals("no-cache")
+          || tag.equals("no-sandbox")
+          || tag.equals("no-remote-exec")
+          || tag.equals("no-remote-cache");


Afaik no-remote-exec and no-remote-cache don't exist as execution requirements (yet)? Also take a look at the ExecutionRequirements where we define names for all the available tags.

~~I also can't find any reference to those two tags having special meaning here: https://docs.bazel.build/versions/master/be/common-definitions.html#common-attributes~~

Ah they're planned tags for more control over remote caching: #7932 (comment)

Yes, those are only planned, but I decided to add them to the whitelist from the beginning. I'll think about it, perhaps to avoid confusions it would be better to not include them here and just add a comment for the future task #7932, where they will be introduced.

buchgr · 2019-06-13T08:41:43Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+    Map<String, String> map = new HashMap<>();
+    for (String tag :
+        NonconfigurableAttributeMapper.of(rule).get(CONSTRAINTS_ATTR, Type.STRING_LIST)) {
+      // We don't want to pollute the execution info with random things, and we also need to reserve


serious question: what's the downside of "polluting" execution info?

removed this comment block at all - should have been more careful with copy-pasting :)

buchgr · 2019-06-13T08:42:44Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+        map.put(tag, "");
+      }
+    }
+    return ImmutableMap.copyOf(map);


instead of a copy use an ImmutableMap in the first place?

buchgr · 2019-06-13T08:43:40Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+    for (String tag :
+        NonconfigurableAttributeMapper.of(rule).get(CONSTRAINTS_ATTR, Type.STRING_LIST)) {
+      // We don't want to pollute the execution info with random things, and we also need to reserve
+      // some internal tags that we don't allow to be set on targets. We also don't want to


couldn't we prevent that by prefixing internal execution requirements with "internal-" or so? also which are those?

removed this comment block at all :)

buchgr · 2019-06-13T08:52:56Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+   * Only supported tags are included into the execution info,
+   * see {@link #LEGAL_EXEC_INFO_KEYS} and {@link #TAGS_PROPAGATED_TO_EXEC_INFO}.
+   */
+  public static Map<String, String> getFilteredExecutionInfo(Object executionRequirementsUnchecked,


Add a javadoc describing the expected type of executionRequirementsUnchecked?

buchgr · 2019-06-13T08:54:45Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+   */
+  public static Map<String, String> getFilteredExecutionInfo(Object executionRequirementsUnchecked,
+      Rule rule) throws EvalException {
+    Map<String, String> executionInfo = Maps.newLinkedHashMap();


Why the LinkedHashMap as opposed to an immutablemap?

ishikhman · 2019-06-13T09:28:26Z

I am not a fan of white listing tags. It's error prone. Couldn't we apply the same filter that we apply to execution_info also to tags? What's the downside?

This is what we've agreed on in the design doc - propagate only a fixed set of tags for now. If we apply the same filter as for execution_info, more tags will be propagated.

hlopko

Thanks for working on this! Only minor things.

hlopko · 2019-06-13T09:38:12Z

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

-                  "execution_requirements")));
-    }
+
+    Map<String, String> executionInfo =


Nit: ImmutableMap

hlopko · 2019-06-13T09:41:02Z

src/main/java/com/google/devtools/build/lib/analysis/skylark/SkylarkActionFactory.java

-    }
+
+    Map<String, String> executionInfo =
+        TargetUtils.getFilteredExecutionInfo(executionRequirementsUnchecked, ruleContext.getRule());


Does it make sense to do the validation here and pass "checked" execution requirements to the TargetUtils method?

I don't have a strong opinion on this, but IMO it's better to keep it all in one place. And it's easier to test is this way ;)

hlopko · 2019-06-13T09:42:53Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+  // with execution requirements declared on the rules.
+  // See https://github.com/bazelbuild/bazel/issues/7766 for details.
+  private static final Predicate<String> TAGS_PROPAGATED_TO_EXEC_INFO =
+      tag -> tag.equals("no-remote")


As somebody completely ignorant of these things my first reaction is hmm where can I find out what these do. Wdyt about making these constants, and adding javadoc and document (or reference existing documentation) from these constants?

hlopko · 2019-06-13T09:44:20Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+    Map<String, String> executionInfo = Maps.newLinkedHashMap();
+    executionInfo.putAll(getExecutionInfoFromTags(rule));
+
+    if (executionRequirementsUnchecked != Runtime.NONE) {


Ignoring my comment above to validate the input before entering this method, do we need to check for None at all when we call castSkylarkDictOrNoneToDict anyway?

true, we don't need it at all

hlopko · 2019-06-13T09:44:35Z

src/main/java/com/google/devtools/build/lib/packages/TargetUtils.java

+   */
+  private static Map<String, String> getExecutionInfoFromTags(Rule rule) {
+    // tags may contain duplicate values.
+    Map<String, String> map = new HashMap<>();


Nit: ImmutableMap.builder()

hlopko · 2019-06-13T09:46:04Z

src/test/shell/bazel/tag_propagation_skylark_test.sh

+
+# Test a basic skylark ctx.actions.run rule which has tags, that should be propagated,
+# when the rule also has execution_info
+function test_tags_propagated_to_run_with_exec_info_шт_кгду() {


buchgr · 2019-06-13T11:44:13Z

This is what we've agreed on in the design doc - propagate only a fixed set of tags for now. If we apply the same filter as for execution_info, more tags will be propagated.

Two things:

Independent of what's in the design doc if it makes no sense we shouldn't do it :).
Also, in the actual implementation we can always be more relaxed and still fulfill the contract of the spec.

The question to answer before we move forward with this implementation is why does it need to be a whitelist and can't just be the existing filters?

ishikhman · 2019-06-13T12:17:02Z

This is what we've agreed on in the design doc - propagate only a fixed set of tags for now. If we apply the same filter as for execution_info, more tags will be propagated.

Two things:

Independent of what's in the design doc if it makes no sense we shouldn't do it :).

Also, in the actual implementation we can always be more relaxed and still fulfill the contract of the spec.

Of course, and I'm not arguing with this :)
I feel a bit confused though because we have already discussed exactly the same issue during the design doc review and I had an impression that we reached an agreement. Apparently that is not the case :) Anyways, I'm open to further discussion, maybe we will come up with a better solution.

The question to answer before we move forward with this implementation is why does it need to be a whitelist and can't just be the existing filters?

The initial idea was to propagate all* tags, but it's not clear what to do in case of conflicts. Currently there is only one example of a conflict requires-network and block-network, but if we allow 'requires-' and 'block-' we will get more conflicts in the future, it's just a matter of time.
If we do not take care of these conflicts and just propagate all the tags via the existing filters, we are potentially introducing unpredictable behavior.

Theoretically, we could just use the current filter and in case of conflicts: 1) do nothing and introduce unpredictable behavior, as mentioned above; 2) allow tags to override exec_requirements declared on rules; 3) rise an exception. And it is not obvious which one to choose or whether we should choose it now. Therefore I got an idea to propagate only the tags we are sure about.

Why do you think that whiltelist is so much worse that the existing filters?

*all - something similar to the current execution info filter

buchgr · 2019-06-14T11:10:35Z

I feel a bit confused though because we have already discussed exactly the same issue during the design doc review and I had an impression that we reached an agreement. Apparently that is not the case :)

Probably but I don't recall and it doesn't really matter - I reserve the right to change my opinion :). I am happy for us to go with a whitelist in the face of good arguments for it but I haven't seen any so far.

If we do not take care of these conflicts and just propagate all the tags via the existing filters, we are potentially introducing unpredictable behavior.

The example of "block-network" and "requires-network" is a purely hypothetical example. Have we seen this causing trouble in the past?
"block-network" and "requires-network" is pretty much only useful for tests and tests have been passing these tags to execution requirements since the very beginning and I am not aware of this being an issue for anyone.
It should be up to the execution strategy to decide what it does with conflicting requirements not the propagation logic.

Why do you think that whiltelist is so much worse that the existing filters?

It's error prone. If an execution strategy adds a new execution requirement it's easy to miss that this list also has to be updated.
If there was a whitelist this would be the wrong place to add it. An execution strategy should define what it accepts.

Currently there is only one example of a conflict requires-network and block-network, but if we allow 'requires-' and 'block-' we will get more conflicts in the future, it's just a matter of time.

Looking at the change history of ExecutionRequirements.java does not back this claim up.

ishikhman · 2019-06-17T08:29:10Z

Probably but I don't recall and it doesn't really matter - I reserve the right to change my opinion :). I am happy for us to go with a whitelist in the face of good arguments for it but I haven't seen any so far.

okay, then why do we need the design doc discussion-approval process at all? :)

The example of "block-network" and "requires-network" is a purely hypothetical example. Have we seen this causing trouble in the past?

"block-network" and "requires-network" is pretty much only useful for tests and tests have been passing these tags to execution requirements since the very beginning and I am not aware of this being an issue for anyone.

It should be up to the execution strategy to decide what it does with conflicting requirements not the propagation logic.

and

It's error prone. If an execution strategy adds a new execution requirement it's easy to miss that this list also has to be updated.

If an execution strategy adds a new execution requirement it's easy to miss that there is a potential conflict.

Looking at the change history of ExecutionRequirements.java does not back this claim up.

Do you mean that it hasn't changed recently/often? This doesn't mean that it won't :)

buchgr · 2019-06-17T08:43:57Z

I think it would be more productive to put forward the argument in favor of having a white list.

ishikhman · 2019-06-17T09:19:32Z

Ok, back to the discussion.

Pros(+) and cons(-) for both options:

Whitelisting:
(-) requires white-list updated for every new exec requirement, that should be propagated

FIX: I can just a simple tests that would fail for every new execution requirement => the person who added it would need to decide - to propagate it or not.

(+) easy to implement
(+) no need in conflicts resolution

More generic filtering:
(-) potential conflicts
If an execution strategy adds a new execution requirement it's easy to miss that there is a potential conflict.

** Potential FIX**: add a test that would check new exec requirements for potential conflicts => the person who added it would need to decide what to do with it. + would be nice to enforce a proper documentation on this. For example, as for block/requires-network, at the moment block-network always takes precedence over the second one.

(+) not that difficult to implement either
(+) no need to update the list for every new exec requirement

Suggestion
I am not a big fan of white-listing as well, but if feels much easier and safer cause we have a better control over what is propagated and we do not introduce any (even potential) conflicts.

Do you have any other arguments that I've missed?

buchgr · 2019-06-17T09:31:54Z

As I wrote above the current behavior is to forward both tags as execution requirements:

sh_test
  name = "foo",
  tags = ["requires-network", "blocks-network"],
)

We have code to deal with this https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/actions/Spawns.java;l=44?q=Spawns.java. "blocks-network" takes precedence.

We'll not introduce different behavior for test rules and build rules.

ishikhman · 2019-06-17T09:45:56Z

As I wrote above the current behavior is to forward both tags as execution requirements:
sh_test
  name = "foo",
  tags = ["requires-network", "blocks-network"],
)
We have code to deal with this https://source.bazel.build/bazel/+/master:src/main/java/com/google/devtools/build/lib/actions/Spawns.java;l=44?q=Spawns.java. "blocks-network" takes precedence.

We'll not introduce different behavior for test rules and build rules.

Yes, I have seen this and mentioned in my previous comment that we deal with this couple of exec requirements already.

What I am trying to say is that we will not add any mechanism to prevent potential conflicts in the future, while will open the door to it.

Okay, I am still not convinced, but this discussion takes too long already. I will switch to the current filter and will try to think of a way to prevent potential conflicts. If it will take more that 1-2 hours, I will just add a comment to the ExecutionRequirements class, so that future developers are aware of the potential problems.

buchgr

LGTM

buchgr · 2019-06-18T10:40:08Z

src/test/java/com/google/devtools/build/lib/packages/TargetUtilsTest.java

+        "tests/BUILD",
+        "sh_binary(name = 'with-prefix-block', srcs=['sh.sh'], tags=['block-some-feature', 'block-network', 'wrong-tag'])",
+        "sh_binary(name = 'with-prefix-cpu', srcs=['sh.sh'], tags=['cpu:123', 'wrong-tag'])",
+        "sh_binary(name = 'with-local-tag', srcs=['sh.sh'], tags=['local', 'some-tag'])"


remove the above two lines. they are not used in the test.

done, thanks :)

ishikhman · 2019-06-18T11:38:31Z

@laurentlb @hlopko please let me know your thought. This is a short summary of what happened:

I added tags propagation not exactly how we agreed in the design doc, instead of while list I re-used already existing filter that has already been used to filter execution_requirements coming from rules (and tags, but only in some cases)
the case that I initially thought to be a conflict (--block_network vs --requires_network) appeared not to be a problem, as this situation is taken care of at the place of usage
I have added both unit tests and integration tests

Therefore it is safe to assume that we can re-use existing filter, as it was introduced with the same purpose - filter tag that should be propagated to the actions.

Please let me know your thoughts, as I'd like to merge this change before the Summit next week :)

…n target and in a rule

ishikhman · 2019-06-28T08:50:15Z

@laurentlb @hlopko friendly ping :) Please let me know whether you are interested in looking into this change. If not - I'll just merge it.

laurentlb · 2019-07-01T12:48:31Z

Based on your last summary, LGTM

(sorry for the delay!)

Tags declared on targets are not propagated to actions and therefore are not taken into consideration by bazel. This causes some issues, for instance, target marked with a tag 'no-remote' will still be executed remotely. As it was agreed in the design doc (see [doc](https://docs.google.com/document/d/1X2GtuuNT6UqYYOK5lJWQEdPjAgsbdB3nFjjmjso-XHo/edit#heading=h.5mcn15i0e1ch) and bazelbuild#7766 for details), set of tags to be propagated to actions as a first iteration. This change is responsible for that first step for the Starlark Rules. RELNOTES: tags 'no-remote', 'no-cache', 'no-remote-cache', 'no-remote-exec', 'no-sandbox' are propagated now to the actions from targets. Closes bazelbuild#7766 Closes bazelbuild#8612. PiperOrigin-RevId: 256369636

googlebot added the cla: yes label Jun 12, 2019

ishikhman requested review from lberki, dslomov, c-parsons, buchgr, laurentlb and hlopko and removed request for lberki, dslomov and c-parsons June 12, 2019 13:42

ishikhman mentioned this pull request Jun 13, 2019

Pass select tags from target to generated actions as execution requirements higherkindness/rules_scala#187

Closed

buchgr suggested changes Jun 13, 2019

View reviewed changes

hlopko suggested changes Jun 13, 2019

View reviewed changes

small fixes: review comments

c561570

added missing docs

24f43cc

SrodriguezO mentioned this pull request Jun 14, 2019

propagate specific target tags to run and run_shell actions as execution_requirements higherkindness/rules_scala#190

Merged

ishikhman mentioned this pull request Jun 17, 2019

Local actions in a remote execution build should be cachable #7932

Closed

commented out not-yet-added tags

64978e5

switching to the current filter

6c4345b

ishikhman requested a review from buchgr June 17, 2019 12:21

ishikhman requested a review from hlopko June 17, 2019 12:21

buchgr approved these changes Jun 18, 2019

View reviewed changes

cleaned up tests

c731a59

irengrig added WIP and removed WIP labels Jun 19, 2019

added a test and a fix for the case, when the same tag is specified i…

7370188

…n target and in a rule

bazel-io closed this in 5f53ab6 Jul 3, 2019

tags propagation: Starlark rules part #8612

tags propagation: Starlark rules part #8612

Conversation

ishikhman commented Jun 12, 2019

buchgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SrodriguezO Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishikhman commented Jun 13, 2019

hlopko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buchgr commented Jun 13, 2019

ishikhman commented Jun 13, 2019

buchgr commented Jun 14, 2019

ishikhman commented Jun 17, 2019

buchgr commented Jun 17, 2019

ishikhman commented Jun 17, 2019

buchgr commented Jun 17, 2019

ishikhman commented Jun 17, 2019

buchgr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishikhman commented Jun 18, 2019 • edited Loading

ishikhman commented Jun 28, 2019

laurentlb commented Jul 1, 2019

SrodriguezO Jun 13, 2019 •

edited

Loading

ishikhman commented Jun 18, 2019 •

edited

Loading