bazelbuild / bazel

a fast, scalable, multi-language and extensible build system
https://bazel.build
Apache License 2.0
22.99k stars 4.03k forks source link

Compact exec log: remoteCacheable should be "false" when remote_upload_local_results is disabled #22816

Open sluongng opened 3 months ago

sluongng commented 3 months ago

Description of the bug:

In a local-exec build with --remote_upload_local_results=0, we observed that no action cache was uploaded to our Remote Cache server, which is correct.

However, the compact exec log has these fields all set to true.

> bb print --compact_execution_log ~/Downloads/execution_log.binpb.zst | grep -iE '"cache|"remot' | sort | uniq
  "cacheable": true,
  "remotable": true,
  "remoteCacheable": true,

Among them, I think at least remoteCacheable should be set to false to accurately reflect the caching status of the action.

Which category does this issue belong to?

Remote Execution

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Do a build with --remote_upload_local_results=0 and --experimental_execution_log_compact_file= set to some files.

Which operating system are you running Bazel on?

MacOS

What is the output of bazel info release?

7.2.0

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse HEAD ?

No response

If this is a regression, please try to identify the Bazel commit where the bug was introduced with bazelisk --bisect.

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

sluongng commented 3 months ago

cc: @tjgq

tjgq commented 3 months ago

I think it could be argued both ways. Right now the fields only reflect the properties of the spawn, regardless of how the disk/remote cache is configured; you're arguing for them to also take the configuration into account. (Presumably, if you were to run a local build with no disk nor remote cache, you'd also want all three fields to be false?)

sluongng commented 3 months ago

Hmm, that is indeed confusing. Because from the end user perspective, folks don't want to use this information to debug Spawn's internal state inside Bazel, and how that state would later get transformed by the cache configuration. Our users want to know the final state: whether the action was cached/executed remotely or not. The previous intermediary -able states are not actionable to them.

I briefly considered using build_event_stream.TestResult.ExecutionInfo.cached_remotely instead, but that is only available for tests and not actions.

If the current state is valuable to keep, shall we add new fields to indicate the final state of exec/cache status? instead of just -able status?

tjgq commented 1 month ago

Per internal discussion, we're leaning towards not changing the interpretation of the existing fields.

new fields to indicate the final state of exec/cache status

Whether the spawn was executed or retrieved from the cache is already indicated by the runner field (e.g. disk cache hit). We're only missing whether the action result was uploaded or not in the execution case. But I'm also not sure it makes sense to add it - in a future where uploading always happens asynchronously in the background, we wouldn't know at the time of logging.