vscode-remote-release icon indicating copy to clipboard operation
vscode-remote-release copied to clipboard

Alterating PATH env invalidate project bazel cache

Open bhack opened this issue 2 years ago • 8 comments

  • VSCode Version: insider
  • Local OS Version: Ubuntu
  • Remote OS Version: Debian
  • Remote Extension/Connection Type: Docker
  • Logs:

Steps to Reproduce:

  1. Build a project with bazel inside a container launched by Vscode remote
  2. Install a new vscode insider or a new Vscode stable release
  3. The PATH env is alterated every day with the (nightly hash) e.h. PATH=/vscode/vscode-server-insiders/bin/linux-x64/<daily/new-release-sha>-insider/bin/remote-cli

This is going do invalidate the whole project cache and it requires to rebuild large programs from scratch (every day with code-insider) as many projects that use the Bazel build system export PATH env in --action_env:

https://docs.bazel.build/versions/4.1.0/remote-caching.html#known-issues

Environment variables leaking into an action An action definition contains environment variables. This can be a problem for sharing remote cache hits across machines. For example, environments with different $PATH variables won’t share cache hits. Only environment variables explicitly whitelisted via --action_env are included in an action definition. Bazel’s Debian/Ubuntu package used to install /etc/bazel.bazelrc with a whitelist of environment variables including $PATH. If you are getting fewer cache hits than expected, check that your environment doesn’t have an old /etc/bazel.bazelrc file.

See also: https://github.com/tensorflow/tensorflow/pull/51439#discussion_r835326171 https://github.com/microsoft/vscode-dev-containers/issues/1453

bhack avatar May 21 '22 11:05 bhack

The documentation you quote suggests to use /etc/bazel.bazelrc to exclude variables from this check. Does that not work?

chrmarti avatar Jun 01 '22 09:06 chrmarti

Yes but the problem is that "de-facto" many Bazel based projects rely on that export: https://github.com/tensorflow/tensorflow/blob/master/.bazelrc#L158

bhack avatar Jun 01 '22 12:06 bhack

We could add a symlink to ~/bin and add that to PATH to keep that stable.

chrmarti avatar Jun 03 '22 15:06 chrmarti

We could add a symlink to ~/bin and add that to PATH to keep that stable.

The problem is that also in this case the cache will be invalidated when you build with exactly the same image with docker run vs VsCode

bhack avatar Jun 03 '22 16:06 bhack

We could add it to /usr/local/bin or some other system-wide path.

VS Code is adding the remote-cli folder to the PATH. Since the VS Code Server doesn't always run as root in the container only Remote-Containers could use root access to add a symlink to a system-wide bin path. So maybe the VS Code Server could offer a CLI flag to turn off its addition to the PATH.

@bhack As a temporary workaround you could remove this PATH entry in your shell's startup script.

chrmarti avatar Jun 22 '22 07:06 chrmarti

Using the PATH env variable has the advantage it's really just visible and usable when running the VS Code internal terminal. If it were a symlink, it would have to be removed when the process goes down. It could also conflict with a second remote instance of VS Code of different version connected to the same container (a corner case, I admit).

aeschli avatar Jun 22 '22 10:06 aeschli

I suppose that the Bazel/Tensorflow logic about exporting PATH env is that It could be a too sensitive env for reproducible builds E.g. see step 6 on Nvidia official guide: https://docs.nvidia.com/cuda/cuda-quick-start-guide/index.html#redhat-x86_64-run

bhack avatar Jun 22 '22 11:06 bhack

/cc @bamurtaugh

This is going also to invalidate the prepared docker nighty Bazel cache produced by our CI and published RO on GCS when an user will try to consume it inside a devcontainer/codespace:

https://github.com/tensorflow/build/pull/48

bhack avatar Aug 05 '22 12:08 bhack

There are other env variables added by VS Code that change, are these a problem too? Would you have filter these out?

E.g., VSCODE_IPC_HOOK_CLI is used to make the code CLI work. If that needs to be filtered the code CLI won't work and instead of looking for a way to keep it on the PATH when it does not have a separate PATH entry, we should just remove it from the PATH. (Maybe you don't use the code command much inside the container anyway?)

chrmarti avatar Dec 15 '22 08:12 chrmarti

Generally Env variables need to be explicitly passed to bazel so it is quite hard to have "custom env" variable involved in this issue.

But as PATH is not Vscode reserved and it is often included in bazel it could be quite sensitive not only for TF but probably for other bazel projects.

Can we create a soft link Instead?

bhack avatar Dec 15 '22 11:12 bhack

@bhack Would removing it from the PATH in .bashrc/.zshrc make caching work? I have found that adding the following to the devcontainer.json works for removing the entry:

	"postCreateCommand": "echo '\nexport PATH=$(echo \"$PATH\" | sed -e \"s/\\/[^:]*vscode-server[^:]*\\/bin\\/remote-cli://\")' | tee -a ~/.zshrc >>~/.bashrc"

You could also add it in the Dockerfile if that is more convenient.

chrmarti avatar Feb 24 '23 20:02 chrmarti

Sorry I don't have the setup ready anymore to test it.

bhack avatar Feb 24 '23 21:02 bhack