Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Alterating PATH env invalidate project bazel cache #6746

Open
bhack opened this issue May 21, 2022 · 12 comments
Open

Alterating PATH env invalidate project bazel cache #6746

bhack opened this issue May 21, 2022 · 12 comments
Assignees
Labels
containers Issue in vscode-remote containers feature-request Request for new features or functionality
Milestone

Comments

@bhack
Copy link

bhack commented May 21, 2022

  • 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:
tensorflow/tensorflow#51439 (comment)
microsoft/vscode-dev-containers#1453

@chrmarti
Copy link
Contributor

chrmarti commented Jun 1, 2022

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

@chrmarti chrmarti self-assigned this Jun 1, 2022
@chrmarti chrmarti added the info-needed Issue requires more information from poster label Jun 1, 2022
@bhack
Copy link
Author

bhack commented Jun 1, 2022

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

@chrmarti
Copy link
Contributor

chrmarti commented Jun 3, 2022

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

@chrmarti chrmarti added feature-request Request for new features or functionality and removed info-needed Issue requires more information from poster labels Jun 3, 2022
@chrmarti chrmarti added this to the Backlog milestone Jun 3, 2022
@bhack
Copy link
Author

bhack commented Jun 3, 2022

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

@chrmarti
Copy link
Contributor

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.

@aeschli
Copy link
Contributor

aeschli commented Jun 22, 2022

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).

@bhack
Copy link
Author

bhack commented Jun 22, 2022

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
Copy link
Author

bhack commented Aug 5, 2022

/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:

tensorflow/build#48

@chrmarti
Copy link
Contributor

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?)

@bhack
Copy link
Author

bhack commented Dec 15, 2022

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?

@chrmarti
Copy link
Contributor

@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.

@bhack
Copy link
Author

bhack commented Feb 24, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
containers Issue in vscode-remote containers feature-request Request for new features or functionality
Projects
None yet
Development

No branches or pull requests

3 participants