Bazel should re-execute actions when switching between sandboxed/non-sandboxed runs

[kchodorow]

Right now, if I do bazel build //foo; bazel build --sandbox_strategy=standalone //foo, the second build will just be the cached results from the first. However, obviously I wouldn't have turned off sandboxing if I expected it to have the same results as the first build.

Right now, the only workaround I know is to manually delete outputs or run a full bazel clean.

[ittaiz]

Wow, isn't this a really big issue correctness wise? We encountered it internally and were really surprised

[ittaiz]

@philwo any thoughts?

[philwo]

@ittaiz The reasoning behind why changing the execution strategy doesn't invalidate any actions is that we assume that all execution strategies yield the same results (just via different ways of executing actions). If it doesn't, it's a bug.

This also means that tweaking the execution strategy shouldn't be used as a normal way to get different build results.

We could revisit this assumption and make the execution strategy used for an action part of its cache key, of course. :) But I'm not sure if it's the right thing to do - we should definitely discuss this on the mailing list first, as it would be a pretty big change.

@dslomov @lberki WDYT?

[lberki]

That's one tough question. We should definitely not make all execution strategies part of the action key because that would make some interesting use cases (e.g. do full build remotely to benefit from increased parallelism, then incremental builds locally to benefit from shorter round-trip times on the critical path) impossible.

On balance, I'd rather we don't add the knob of "are these two execution strategies equivalent". If this use case is important, maybe we could implement a partial bazel clean so that one can re-run a specific set of actions. Actually, that sorta already works by just deleting their output files (of course, you have to know what those files are, but that's why we are adding a way to query the action graph, /cc @meisterT )

[ittaiz]

I see (and agree). On the other hand I think that maybe defining some strategies as “less safe” might be the wise choice. Or allowing users to decide which strategies are “less safe”. We hit this a lot with tests which pass without sandboxing but fail in sandboxing

…

On Mon, 9 Jul 2018 at 12:46 lberki ***@***.***> wrote: That's one tough question. We should definitely not make *all* execution strategies part of the action key because that would make some interesting use cases (e.g. do full build remotely to benefit from increased parallelism, then incremental builds locally to benefit from shorter round-trip times on the critical path) impossible. On balance, I'd rather we don't add the knob of "are these two execution strategies equivalent". If this use case is important, maybe we could implement a partial bazel clean so that one can re-run a specific set of actions. Actually, that sorta already works by just deleting their output files (of course, you have to know what those files are, but that's why we are adding a way to query the action graph, /cc @meisterT <https://github.com/meisterT> ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2765 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUIFxpwItR_rjKBFe7dI7gg8VHZEZG5ks5uEyaTgaJpZM4MvpzX> .

[ittaiz]

Two usecases:

1. I run a test and it fails, I suspect it's related to sandboxing so I run it as standalone and it indeed passes. I now try to re-run it in sandbox to try and better triage the error but now it passes.

2. I run a build of a library which fails, I suspect maybe I have an undeclared dependency (@aehlig mentioned compiler?), I run it as standalone and it indeed passes. I now try to re-run it in sandbox to try and better triage the error but not it's already built.

@talya is the above complete enough? from this it sounds we can make due with some kind of flag which will not write to the local cache for this specific run (it will read from the cache). Am I missing something?

cc @dslomov

[talya]

usecase 1 is indeed what brought us to this thread in the first-place.

@philwo - re "we assume that all execution strategies yield the same results" - imo in the case of sandbox vs standalone strategies this does not always hold.

@ittaiz - A flag which will not write to the local cache for this specific run could be nice, but it could also be easy to forget to add it. and if you ARE paying attention to this then using the --cache_test_results=false is already available.
the direction suggested by @lberki of supplying "a partial bazel clean so that one can re-run a specific set of actions" sounds good.

[helenalt]

@philwo please triage/respond

[philwo]

I really don't see any good way to improve this. If you want to ensure a clean build, run bazel clean before your build. If you want to benefit from the "clean build remotely, then incremental builds locally" use case, then this behavior is WAI. If you want to partially rebuild, you can delete the relevant output files from bazel-out and it should just work.

Supporting bazel clean //my:target sounds nice. I'd be happy to review a pull request that implements this.

[jmmv]

Agree with @philwo's assessment in the latest comment. I'll close this in favor of #1035, which also talks about having better mechanisms of cleaning the cache -- and implementing a bazel clean //target would fall in that category.