Should allow_cache_upload be available on more rules?

[cormacrelf]

As far as I can tell, the allow_cache_upload flag is available only on 3 specific binary rules, and nowhere else. allow_cache_upload is also default False on every actions.run() call, so there are probably a hundred places in prelude that have yet to get it threaded through. If that was ever the goal.

Available:

• python_binary
• cxx_binary
• rust_binary

Not available, for example:

• cxx_library
• rust_library
• rust_test

There's obviously some rhyme to this, in that somebody committed those three | buck.allow_cache_upload_arg() attributes only on binary rules, but I can't find a rationale. I have a few guesses:

• Cache uploads have just not been used much, given they do not work yet in OSS (pending Implement missing parts of OSS RE caching #477). Don't know what happens at Meta. Maybe it's RE all the way, and never keep a local build.
• Maybe RE backends are always able to push and pull from the cache no matter what local buckd instances do, so nobody cares about local builds?
• Conservatively enabling for only binary builds papers over any issues with uploading local action results that might not work on other machines. Binaries are more likely to be portable if you used a slightly different compiler or linked to a different shared library version. We just don't trust local execution or the exhaustiveness of hidden dependency inclusion (to cache-bust different configurations) enough to enable it.

I just quickly patched prelude to enable it on rust_library and supply it in the macro reindeer calls for all crates.io targets, and it seems to work great. You can launch another buckd on a separate isolation dir on a big crate graph that reindeer made, and it eats it for breakfast.

[cormacrelf]

Ah, I think I see what's going on. allow_cache_upload is getting slowly rolled out to more rules as the responsible team becomes sure they've dealt with the issues. 211d555

[krallin]

The default is that actions executed on RE get cached there implicitly, this flag only controls behaviour when actions get executed locally Internally the majority of actions execute on RE, so this flag only makes sense on things that may disable running on RE in the first place. That said, the rules you see it on are typically rules where cache misses were found to be problems on things that were forced to execute locally. We’re not really fans of how this behaviour is configured; it’s OK but it’s clearly a local optimum.

…

On Sun, 5 Nov 2023 at 15:20, Cormac Relf ***@***.***> wrote: Ah, I think I see what's going on. allow_cache_upload is getting slowly rolled out to more rules as the responsible team becomes sure they've dealt with the issues. 211d555 <211d555> — Reply to this email directly, view it on GitHub <#478 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AANIHVR7UXK5TNLI3OCCREDYC6OELAVCNFSM6AAAAAA66LFYDCVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTOOJTG42TCMRXGU> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[cormacrelf]

Outside of an RE-majority context, choosing to allow cache uploads seems to be more a function of the hermeticity of toolchains, rather than something you'd want to be forced to enable and disable on individual rules. I agree the default should probably be off on the system_*/demo toolchains that are probably not going to cache well. But other toolchains can default to true.

Then I could configure my rust toolchain to allow cache uploads, and the full crate graph would start flowing into the cache.

Rust_library et al would read the toolchain's value for allow_cache_upload. You can keep a flag on those individual rules as an override, treating it as an optional bool with the default value as None. Whatever works.

[ndmitchell]

I wonder if the more pragmatic thing is to have allow_cache_upload default to None, where True means yes, False means no, and None means you consult some config. We'd leave it off at Meta, but if someone outside Meta wants to cause everything to upload I see no reason not to make that easy.