precompiling results in action output conflicts

[keith]

🐞 bug report

Affected Rule

load("@rules_python//python:defs.bzl", "py_test")

py_test(
    name = "first",
    srcs = ["foo.py"],
    main = "foo.py",
)

py_test(
    name = "second",
    srcs = ["foo.py"],
    exec_properties = {"foo": "bar"},
    main = "foo.py",
)

With this BUILD file, where 2 python rules include the same file, building both of these targets results in:

ERROR: file '__pycache__/foo.cpython-311.pyc' is generated by these conflicting actions:
Label: //:second, //:first
RuleClass: py_test rule
JavaActionClass: class com.google.devtools.build.lib.analysis.actions.StarlarkAction
Configuration: 8e4ab22e89843aa348ba782be25eadb5bae40f2ae62dec3edd2cdaf809e8d7b1
Mnemonic: PyCompile
Action key: aa843bb97b9c08aba98aa87861cf3bba7f6f80370dcc227a3aa766a618449a45, d8983143b0af1168a9ab184492ac6ef78a440269bfd600178f4e6d984e208bf7
Progress message: Python precompiling foo.py into bazel-out/darwin_arm64-fastbuild/bin/__pycache__/foo.cpython-311.pyc
Action describeKey: Python precompiling foo.py into bazel-out/darwin_arm64-fastbuild/bin/__pycache__/foo.cpython-311.pyc
  Environment variable: PYTHONHASHSEED=0
  Environment variable: PYTHONNOUSERSITE=1
  Environment variable: PYTHONSAFEPATH=1
  Argument: bazel-out/darwin_arm64-opt-exec-ST-d57f47055a04/bin/external/rules_python~/python/private/python3
  Argument: bazel-out/darwin_arm64-opt-exec-ST-d57f47055a04/bin/external/rules_python~/tools/precompiler/precompiler
  Argument: --invalidation_mode
  Argument: checked_hash
  Argument: --src
  Argument: foo.py
  Argument: --src_name
  Argument: foo.py
  Argument: --pyc
  Argument: bazel-out/darwin_arm64-fastbuild/bin/__pycache__/foo.cpython-311.pyc
  Argument: --optimize
  Argument: 0
  Argument: --python_version
  Argument: 3.11
  Output paths mode: OFF

PrimaryInput: File:[/Users/ksmiley/Downloads/repro[source]]foo.py
PrimaryOutput: File:[[<execution_root>]bazel-out/darwin_arm64-fastbuild/bin]__pycache__/foo.cpython-311.pyc
Owner information: ConfiguredTargetKey{label=//:second, config=BuildConfigurationKey[8e4ab22e89843aa348ba782be25eadb5bae40f2ae62dec3edd2cdaf809e8d7b1]}, ConfiguredTargetKey{label=//:first, config=BuildConfigurationKey[8e4ab22e89843aa348ba782be25eadb5bae40f2ae62dec3edd2cdaf809e8d7b1]}
MandatoryInputs: are equal
Outputs: are equal
Use --verbose_failures to see the command lines of failed build steps.
ERROR: com.google.devtools.build.lib.actions.MutableActionGraph$ActionConflictException: for __pycache__/foo.cpython-311.pyc, previous action: action 'Python precompiling foo.py into bazel-out/darwin_arm64-fastbuild/bin/__pycache__/foo.cpython-311.pyc', attempted action: action 'Python precompiling foo.py into bazel-out/darwin_arm64-fastbuild/bin/__pycache__/foo.cpython-311.pyc'

Is this a regression?

I bisected to eb2225c, we hit it when updating from 0.36.0 to 0.40.0

🔬 Minimal Reproduction

Build in this repo: repro.zip

[rickeylev]

Just recapping what I said in slack. This particular issue is really annoying and frustrating. I ran into this within Google and spent about a week trying to find workarounds, but eventually gave up. Instead, I updated a few hundred targets to have them use the exec-group for exec properties instead of using general keys. e.g. using the key cpp_link.mem instead of simply mem. What I found was exec props were being set to affect specific actions and affecting every action was unnecessary.

---

This is due to two targets with the same .py source file, but different exec_properties.

Unfortunately, there isn't much we can do in the rules because of a confluence of things.

First: the generated file name has to be based on the source file, not the target, otherwise the pyc file won't end up in a location the interpreter can find it.

The basic thing we have to do is ensure that the py_precompile exec group is always getting the same exec properties. Ideally, both targets are set to "use the defaults for the py_precompile exec group", e.g. the equivalent of not setting any exec properties. However, having both set the same py_precompile.XXX keys/values is also acceptable. The important thing is both targets have the same values for the py_precompile exec group.

Unfortunately, we can't override this when ctx.actions.run() is called -- this is because the exec_properties set on the target have precedence over exec properties set in action; this is WAI (the point of setting a target-level exec_properties is to override what the rules do)

This means we have to set the exec_properties during the loading phase, before the rule is invoked. Unfortunately, this has several issues and caveats, but the two big ones are:

1. There is no way to "reset" exec properties to "missing"
2. The set keys/values accepted for exec properties is unbound.

(1) means, when given {"foo": "bar"}, we can't transform it to an equivalent dictionary that means "for the py_precompile exec group, act like the foo key was unset" (e.g. {"foo": "bar", "py_precompile.foo": "SPECIAL_VALUE_TO_INDICATE_UNSET"}.

An alternative would be to always set the py_precompile keys to some default value, e.g. merge in {"py_precompile.foo": "default"}. But (2) prevents this: the key-values are arbitrary, and we don't know what the default is for an arbitrary key.

Another complication is exec_properties is configurable, i.e. it might be select(), so we can't reliably inspect it during the loading phase (this complicates (1) above). The | operator can be used to merge dicts into a select, which gives some limited options.

---

gtg now. A bit of a half-baked wild idea would be: perform compiling in a separate target.
e.g. generate a target that takes srcs as input and outputs py/pyc files. We can then control the exec props of that specific target.

[rickeylev]

I ran into this in another project, as did @ybaturina

What I said in my Nov 27, 2024 post still stands, however, I suspect this problem is more pervasive than I originally understood. What I notice is the same test gets run for different ML accelerator backends, and a flag or env var is used to change the test. e.g.

py_test(name="foo_cpu_test", srcs=["foo_test.py"], args=["--backend=cpu"], tags=["requires-cpu"], ...)
py_test(name="foo_cuda_test", srcs=["foo_test.py"], args=["--backend=cuda"], tags=["requires-cuda"], ...)

Which is pretty reasonable thing to do.

Fix wise, what I'm thinking something like: In loading phase, check if exec_properties is set, or if there are any requires-* tags. If so move srcs into a separate (internal) target whose only purpose is to precompile files. This precompile rule doesn't copy over exec_properties and only copies over non-requires-* tags. This should result in both actions having the same action cache key and make Bazel happy.

A separate precompile rule could also help bring some config-graph benefits, too: if it drops/resets certain transitions (e.g. C++ ones), then it'll probably improve the cache-hit rate when those unrelated flags change.