Need string_keyed_label_list_dict

[excitoon]

Motivation.

1. Have an ability to write my_pkg_tar not repeating directories list:

my_pkg_tar(
  name='xxx'
  files={
    'bin': [ *loooong_list ], # * just for clarity
    'lib': [ *one_more_long_list ],
  }
)

instead of:

my_pkg_tar(
  name='xxx'
  files={
    ':aaa': 'bin',
    ':aab': 'bin',
    ':aac': 'bin',
    ':aad': 'bin',
    ':aae': 'bin',
    ':aaf': 'bin',
    ':aag': 'bin',
......... looooooong list
    ':zzs': 'lib',
    ':zzt': 'lib',
    ':zzu': 'lib',
    ':zzv': 'lib',
    ':zzw': 'lib',
    ':zzx': 'lib',
    ':zzy': 'lib',
    ':zzz': 'lib',
  }
)

2. Dicts can not be added to select's of dicts (and even more + is deprecated), one can not write:

{ ':aaa' : 'bin' } + select(....) + { ':zzz': 'lib' }

And we could write:

{ 'bin' : some_list + select(...), 'lib' : some_other_list }

[brandjon]

See this explanation of why we're loathe to add new attribute types.

[aaronmondal]

The linked explanation explains well why adding new attribute types is something one really wants to avoid, but I think string_keyed_label_dict would be general-purpose and useful in quite a few circumstances.

Any new attribute type should be general-purpose and meet a high bar of usefulness (unlikely since we seem to be doing fine so far without it), and not overly complicate BUILD files or rule implementation functions

The way I see it, the absence of string_keyed_label_dict complicates BUILD files and implementation functions.

If one has a collection of very similar targets that one requires to have individual control of, it feels unnatural to do something like this:

TOOLCHAIN_ATTRS = [
    "compiler": attr.label(...),
    "asan": attr.label(default="@myworkspace//:asan"),
    "lsan": attr.label(default="@myworkspace//:lsan"),
    "ubsan": attr.label(default="@myworkspace//:ubsan"),
    ...
]

In BUILD files we would have to put selects on every attribute to configure this. That seems kind of bugprone and not very DRY.

With string_keyed_label_dict we would retain the ability to get individual labels AND have the ability to e.g. completely disable building of any sanitizers via selects:

TOOLCHAIN_ATTRS = [
    "compiler": attr.label(...),
    "sanitizers": attr.string_keyed_label_dict(
        default = {
            "asan": "@myworkspace//:asan",
            "lsan": "@myworkspace//:lsan",
            "ubsan": "@myworkspace//:ubsan",
            ...
        },
    ),
)

The current attributes already cover the usecase. But we technically don't need attr.label either. We could just use attr.string for everything (reminder of how long it took for cc_toolchain to accept tools via labels). We could also use only attr.label and always use config_setting as labels that map to targets. And at some point we are at CMake variables where configs, compiler flags and targets are indistinguishable from each other 🫠

string_keyed_label_dict is not a vital functionality, and it seems to be quite hard to implement since there are so many downstream dependencies. But I think it is a generally useful functionality for a wide range of applications. Considering that this issue has ~50 upvotes, at least a more extensive discussion from the Bazel team would be really cool ❤️

[github-actions]

Thank you for contributing to the Bazel repository! This issue has been marked as stale since it has not had any activity in the last 1+ years. It will be closed in the next 90 days unless any other activity occurs or one of the following labels is added: "not stale", "awaiting-bazeler". Please reach out to the triage team (@bazelbuild/triage) if you think this issue is still relevant or you are interested in getting the issue resolved.

[Wyverald]

I'm also of the opinion that string_keyed_label_dict would be very useful, and wouldn't be very disruptive to our attribute-handling machinery (e.g. it's very intuitive to think about how to represent these in query). It's a rather annoying structure to represent using existing attribute types; presumably, one would use a label_list with a bunch of targets each of which would serve as an entry in the dict, so a <string, label> tuple -- which makes the representation much more verbose. Additionally, this "deconstruction" is even more awkward in a module extension API.

With that said, string_keyed_label_list_dict seems less generally useful to me. It could fairly easily be deconstructed into {string: label} + targets with [label] (for BUILD) or {string: string} + tags with [label] (for module extensions), and this deconstruction doesn't blow up the representation since the "label list" part would presumably take up most of the space.

cc @fmeum

[lberki]

3ef2abc implements the string-to-label-dict attribute type.

For complicated reasons, string-to-label-list-dict would be considerably more complicated to implement and as the last comment by @Wyverald says, the utility of string-to-label-list-dict over string-to-label-dict is marginal. So I consider this bug fixed; on the off chance that string-to-label-list-dict is indispensable, please re-open, but don't expect a very quick resolution.

[fmeum]

Could we backport this to 7.4.0 and 8.0.0? Otherwise it will take a long time for this to be adoptable by rulesets.

[lberki]

Yeah, I will need to do some backporting (not just this change), but I'll also need to earmark some time for it...

[lberki]

#23997 for 8.0.0

[lberki]

#23998 for 7.4.0

[fmeum]

@lberki Now that I think of it, aren't we going to add a string-to-label-list type for #23802?

[lberki]

@fmeum I don't know, I was hoping @katre would decide that -- it's just that I didn't want to commit to it on its own merits because it's way too much work for the little improvement over string-to-label-dict.

[katre]

Yes, string-to-label-list would be very useful for #23802. I;m afraid I don't understand @Wyverald's comment because I am not familiar with the internal representation of attribute typed.

[lberki]

@Wyverald's comment says that you don't usually need a string-to-label-list-dict because you can transform that into a string-to-label-dict and a few separate rules like this:

rule(
  name = "main", 
  attr = {
    "1": ["//one", "//eins", "//egy"],
    "2" :["//two", "//zwei", "//ketto"]})

can be transformed into

rule(
  name = "main",
  attrs = {
    "1": ":un",
    "2": ":deux"]}

newrule(name = "un", numbers = [":one", ":eins", ":egy"])
newrule(name = "deux", numbers = [":two", ":zwei", ":ketto"])

[katre]

Yes, it's possible to do that, but it's not ergonomic, and requires an extra dependency lookup.

The proposed use case from #23802 is for adding new exec constraints to exec groups. The specific proposal is for the syntax:

some_target(
    ...,
    exec_compatible_with = {
        "": ["//:has_fast_cpu", "@platforms//:cpu:x86_64"],
        "test": ["//:has_gpu"],
    },
)

So a) any target can use this attribute, and b) it's part of toolchain resolution, and thus happens before any dependencies are resolved.

Due to this, trying to decompose into a separate constraint_value_group rule just for bundling adds new dependency edges and nodes and slows down ConfiguredTargetFunction (by adding at least one new skyframe restart to resolve these). While this won't be a commonly-used feature, it is a requested feature, and we should try to make it as simple as possible for users, even if it makes for some complexity in the attribute parsing process.

[lberki]

I won't volunteer for adding string_keyed_label_list_dict, but I did do some research and it looks like it's much less work to do so than I had originally thought.

[purkhusid]

Can this issue be reopened since this was not implemented?

[katre]

Re-opened, not sure if I'm going to have time to tackle it, however.

[katre]

Good news, being fixed as part of #24964.