microsoft / vscode-python

Python extension for Visual Studio Code
https://aka.ms/pvsc-marketplace
MIT License
4.31k stars 1.18k forks source link

Fix environment-dependent git pre-commit hooks #10165

Open janosh opened 4 years ago

janosh commented 4 years ago

This is a follow up to #9948 and vscode#90178. After looking at the problem of environment-dependent pre-commit hooks from both the extension's and the editor's side, it seems to me the best solution would be for the Python extension to make sure that whatever environment the user has selected in the status bar is also active during the commit process (using VS Code's source control panel).

I think this would be the expected behavior by most users if you asked them. At least it took me by surprise that my active environment doesn't apply during the commit process even though it's indicated in the status bar. Hence why it took me a while to figure out this is the reason my pre-commit hooks aren't working.

One way to activate the status bar environment in the spawned git process might be as @joaomoreno mentioned to have a git wrapper that's local to the source control panel (i.e. doesn't affect git anywhere else) and simply activates the environment before handing over to real git.

karthiknadig commented 4 years ago

@joaomoreno @janosh In think the best (generic) way is here is if python extension can provide the environment variables. This way pyhton extension can provide the activated environment variables or set the PATH variable (depends on the type of environment). To achieve this the extension will need a hook from VS Code so we can provide these env vars when it spawns git process.

karthiknadig commented 4 years ago

We have marked this issue as "needs decision" to make sure we have a conversation about your idea. We plan to leave this feature request open for at least a month to see how many ๐Ÿ‘ votes the opening comment gets to help us make our decision.

TobiRoby commented 4 years ago

Any updates? I would realy appreciate this behaviour!

karthiknadig commented 4 years ago

Sorry, no updates on this yet.

janosh commented 4 years ago

In my particular case of git hooks powered by pre-commit, an obvious workaround (in hindsight) - as suggested by @asottile in pre-commit#1537 - is to keep the commit hooks and the environment separate.

That is, uninstall pre-commit confined to the virtual/conda/whatever environment (pip uninstall pre-commit) and reinstall it globally (on macOS brew install pre-commit). Simple as that.

Might still be worth it for the Python extension to activate the environment indicated by the status bar during the commit process for cases where a global solution does not exist.

ervteng commented 4 years ago

I'm also having this issue, but only recently. Actually, pre-commit was working fine until the last update of VSCode, so not sure what changed.

jaygorrell commented 3 years ago

Might still be worth it for the Python extension to activate the environment indicated by the status bar during the commit process for cases where a global solution does not exist.

Definitely. In my case, a commit hook runs pyright and needs access to the virtual environment. Thanks for this issue... it took me a while to find one.

Wesleyliao commented 3 years ago

I'm having this issue as well, also recently and on Windows. Specifically the VS Code Source Control panel uses the wrong Python path (does not respect the interpreter selected in the Status Bar) during commit when pre-commit is installed.

LudwigStumpp commented 3 years ago

Same issue for me. Do not have access to the activated conda environment when using the VS Code Source Control Panel.

What I am trying to do is to run pytest (installed in virtual anaconda environment) as a pre-commit hook using the pre-commit library. When commiting from the Integrated Terminal, everything is fine since it can locate the installed pytest library (installed in virtual anaconda environment). However commiting from the Source Control Panel, there is an error saying Executable "pytest" not found.

LudwigStumpp commented 3 years ago

One way to activate the status bar environment in the spawned git process might be as @joaomoreno mentioned to have a git wrapper that's local to the source control panel (i.e. doesn't affect git anywhere else) and simply activates the environment before handing over to real git.

Here is how I managed to get it working:

Create file test.sh that first activates the environment virtual_environment and then runs the test using pytest:

#!/bin/bash
source ~/miniconda3/etc/profile.d/conda.sh
conda activate virtual_environment
pytest

And call it from within .pre-commit-config.yaml

- repo: local
  hooks:
    - id: pytest-check
      name: pytest-check
      entry: bash test.sh
      language: system
chris1248 commented 3 years ago

Just make the git commit hooks respect the python environment selected in the bottom status bar. Why are we sitting here 3 months later arguing about this?

guoquan commented 3 years ago

Any progress on this? My pre-commit hooks have been being consoled only for a while.

@LudwigStumpp s solution seems to work for any individual.

Here is how I managed to get it working:

Create file test.sh that first activates the environment virtual_environment and then runs the test using pytest:

However, when it comes to team development, I don't think it is possible for the team to share the same wrap shell. We use the same python, but there could be different virtual environment names, different managers (conda or venv), or even different shells.

Actually, I had a similar issue when I was working with node.js 's git pre-commit hook, where git did not active correct npm version.

git module should really resolve and load users' real console for running its commands.

LudwigStumpp commented 3 years ago

@guoquan could you please upvote my solution so that others see that it is working? :) Let's hope that this gets fixed soon, though!

guoquan commented 3 years ago

@LudwigStumpp Definitely! That should solve the issue for independent developers.

What I wanted to bring into the discussion is, in team development, one can only make very humble assumptions to this test.sh. For example, people in one team may use conda, miniconda, or venv; people may use different names for the environment.

Digging into pre-commit, it seems it will create and maintain venv by itself for checking. So I am thinking, pre-commit doesn't want the developers to take care of the environment. Thus "environment-dependent" is a pseudoโ€‘problem. The only thing needed in the environment governed by the user is pre-commit.

So my current workaround is to install (only) pre-commit in the system-wise python.

Looking back at the issue, what we need now is vsc to activate the correct environment that contains pre-commit.

JoshFerge commented 3 years ago

any updates on this?

karthiknadig commented 3 years ago

@JoshFerge this issue haven't been prioritized yet. We plan to do it eventually but no ETAs for that yet.

arwedus commented 2 years ago

This problem affects me, too.

Example: I get an error from my pre-commit hook that pymarkdown is not installed when trying to commit from VS code "Source Control" window even though the python virtualenv that contains pymarkdown is selected as interpreter, as can be seen in the bottom status bar, and everything works fine in the integrated terminal.

The temp solution for our project thus has to be to not use the source control window.

floatingpurr commented 2 years ago

Same problem here. It'd be nice if git followed the "python.pythonPath" value from settings

mlisovyi commented 2 years ago

Same problem here :( It would be a very important functionality in the development setup

sebov commented 2 years ago

Hi! Any update on the issue?

mschirdel commented 2 years ago

hi, yeah any updates on this one ?

tonirosemann commented 2 years ago

Crazy this is still not addressed. Any update ?

luabud commented 2 years ago

Hey folks, sorry, no updates yet. We're bringing this up for discussion with the team next week

brettcannon commented 2 years ago

Crazy this is still not addressed. Any update ?

This issue is only the 11th most upvoted issue we have. And if we take velocity into consideration (votes/days-since-creation), it also does not push this issue to the top. And all of this while we have other needs such as maintenance, etc.

So while I understand wanting an integrated solution instead of the stopgap one that was shared above, we do have to balance this with everything else we are being asked to work on. If someone here wants to discuss a solution and propose a PR before we are able to get to this ourselves, then please do and we will happily work with you.

ravi-mosaicml commented 2 years ago

I wrote a git wrapper to work around this issue.

The wrapper script sources my virtual environment before executing git. I then set the git.path setting in my VSCode user settings.json file to my wrapper script.

This is the wrapper script:

#!/bin/bash

# Run git via the virtual environment

source /path/to/venv/bin/activate

exec git $@

Note that the git.path config setting must defined in the user-level settings.json file (this parameter is not supported in the workspace-level settings.json). If you use a different python virtualenv for each workspace, it is possible to condition the path based on $(pwd). For example, you can source $(pwd)/venv/bin/activate >&2 if the virtualenv is located with the venv folder of the workspace.

Then, pre-commit hooks will run inside the virtualenv when using the integrated VSCode git tools.

JamesHutchisonCarta commented 2 years ago

I worked around this by adding a wrapper script around the regular pre-commit hook that does whatever you need, and then executed the regular pre-commit hook.

My use case was dev containers / codespaces, so I stuck this in the .devcontainer subdirectory, then updated the container's postCreateCommand logic to install the wrapped version. The git.path solution doesn't work in this case, as it seems the setting is rejected.

gjoseph92 commented 2 years ago

In my case, a commit hook runs pyright and needs access to the virtual environment.

I had the same issue. In my case, the environment is managed with poetry and pyright is installed via poetry, so switching to this in .pre-commit-config.yaml worked:

  - repo: local
    hooks:
      - id: pyright
        name: pyright
        entry: poetry run pyright  # used to just be `pyright`
        language: node
        pass_filenames: true
        types: [python]
        # additional_dependencies: ["pyright@1.1.251"] <-- Remove this, since poetry installs it for us
dslandry commented 2 years ago

Hi, I have the same issue with my python environment. What is really weird is that everything was working perfectly fine before the lat update. Do we have an ETA on this?

brettcannon commented 2 years ago

@dslandry no ETA. You can always check our latest monthly iteration plan to see what we are hoping to get in at https://github.com/microsoft/vscode-python/labels/iteration-plan (the latest one is also always pinned at the top of our issues page).

NickCrews commented 2 years ago

I'm not using pre-commit, so I can't use the stopgaps mentioned above. I'm using hatch. That means that the venv isn't created at a predictable place like myproject/venv, so I can't do some manual activation. I've tried hatch run bin/lint.sh, but it gives hatch: command not found even though hatch was installed globally with pipx install hatch. vscode is running all git commands in a different environment from the integrated terminal. If I put which git && exit 1 in my hook, then I can see that vscode is using (on macos) /Library/Developer/CommandLineTools/usr/libexec/git-core/git, which is different from the git that is used in the integrated terminal. If I understood what tools/commands were available in the environment that vscode uses, then I could come up with some sort of stopgap. Anyone have any tips here?

What is the stopper here? Do we need to decide if "run all git commands in the currently-selected venv" is the solution? Or that is definitely the solution, and someone just needs to write a PR?

brettcannon commented 2 years ago

@NickCrews the stopper is thinking through the problem, figuring out what the best solution is, and then writing a PR for it. So we're still at the "someone needs to propose a solution" stage so we can discuss whether the proposal makes sense (hence the label ๐Ÿ˜‰).

NickCrews commented 2 years ago

Ok thanks ๐Ÿ‘. Didn't see the label. I wasn't sure if maintainers wanted to even consider proposals or if the whole premise is flawed, eg it's a bad idea to modify the environment that VScode runs git in. Not volunteering to write anything though ๐Ÿ˜‚, classic.

brettcannon commented 2 years ago

I wasn't sure if maintainers wanted to even consider proposals or if the whole premise is flawed, eg it's a bad idea to modify the environment that VScode runs git in.

Yep, that's part of what the discussion would have to cover.

JotaFan commented 1 year ago

Just make the git commit hooks respect the python environment selected in the bottom status bar. Why are we sitting here 3 months later arguing about this?

how exactly?

JotaFan commented 1 year ago

I solve this by adding a new environmental variable to .git/hooks/pre-commit containing the path to the conda enviroment python, to be used in the the hook. .git/hooks/pre-commit

INSTALL_PYTHON='c:\path\to\.conda\envs\my_env\python.exe'
ARGS=(hook-impl --config=.pre-commit-config.yaml --hook-type=pre-commit)
replace='\'
replacewith='/'
USE_PYTHON="/${INSTALL_PYTHON/${replace}/${replacewith}}"
export USE_PYTHON=$USE_PYTHON

Since I ran pre-commit inside the conda enviroment the pre-commit hook already had the INSTALL_PYTHON variable. This will be used in the .pre-commit-config.yaml Example using black: repos:

  - repo: local
    hooks:
      - id: black
        name: "black check"
        language: system
        entry:  bash -c 'exec $USE_PYTHON -m black . --check -q -tpy38'
        verbose: true
        exclude: ^(.ipython)
        require_serial: true
        pass_filenames: false

Since I am making this work for a team, and all working in windows, I made a bat script that would insert those lines in the hooks, and I made the pre-commit call bash and execute the new assigned python path. This seems very hacky, but ends up doing the trick.

Edit: I got the idea after roaming for hours on this and other related issues, and I saw a comment that was proposing the use of a long bash command, I did not save the link, nor I recall where I got it.

afischer-opentext-com commented 1 year ago

Since pip 23/pep 668 we are discouraged to deploy pre-commit into the systems python deployment. As a consequence this feature is more needed than ever before. While pre-commit may still be available, the use of local pre-commit-hooks (which allow to decouple from systems such as GitHub) no longer works.

Also, the workaround using the git.path setting does not work as I do not see that setting in current versions of vscode. Has this been renamed?

Upgwades commented 1 year ago

Adding onto https://github.com/microsoft/vscode-python/issues/10165#issuecomment-818725613 for the specific issue around pytest local testing this worked for me when running git commit via VSCODE to run pytest without defining an extra script:

  - repo: local
    hooks:
      - id: pytest
        name: pytest
        language: system
        entry: poetry run pytest
        pass_filenames: false
        always_run: true
stefanbschneider commented 1 year ago

To me, it's not even clear which environment the pre-commit hook inside VSCode uses? Conda's base env or sth else? It doesn't find one of the dependencies that is installed in the correct env. Ideally, the pre-commit would also use the env. As a workaround, I could also install the dependency elsewhere; but I don't even know where.

This is easily the most annoying part about VSCode atm...

Paul-Durrant commented 1 year ago

I have a pre-commit hook that calls mypy. It ought to work, but mypy isn't found, apparently because the remote virtual environment isn't being activated before git is called.

Terminals get the environment activated correctly. So commits work in the terminal.

I can, in an ugly way involving full paths, use the localgit suggestion above, so that git runs in my remote virtual environment. But I really didn't expect to have to do this.

LudwigStumpp commented 1 year ago

Adding onto #10165 (comment) for the specific issue around pytest local testing this worked for me when running git commit via VSCODE to run pytest without defining an extra script:

  - repo: local
    hooks:
      - id: pytest
        name: pytest
        language: system
        entry: poetry run pytest
        pass_filenames: false
        always_run: true

Works assuming one uses Poetry.

gresavage commented 1 year ago

How is this still open after 3+ years? Kind of absurd when you think about how simple the fix is and how many people are having the issue.

brettcannon commented 1 year ago

@gresavage it's actually not simple as the SCM component of VS Code has no concept of Python interpreters or environments. So to make this work the SCM system in VS Code core would need to be expanded so we could somehow inject environment details so that git itself picks up. This would probably come about via environment variables and we are actively reworking our environment activation support in the terminal to use environment variables, so hopefully this work can then inform an API change in VS Code core for SCM execution and we can build one feature on top of the other's hard work.

Also note that this issue finally hit the top 5 most upvoted issues, but it's also half the number of upvotes of the first and second issue (and the first issue is actually newer than this one). So while we understand this impacts people, we still have other issues that the community has flagged as more important to them overall. We are definitely not ignoring this issue, but our team is only so big and we are expected to support all Python users in VS Code which is not an insignificant number based on our download count.

But if you believe I have misunderstood the complexity of what it will take to resolve this problem, then this extension and VS Code are open source and we do appreciate and accept community contributions. If you or anyone else wants to propose a fix and a PR for it we will happily review it.

gresavage commented 1 year ago

Thank you for the information! That is quite helpful and definitely puts the issue into a clearer scope. Until your comment it seemed like there was little to no activity from devs towards closing this issue.

I'd agree the overall handling of virtual environments and environment variables needs TLC and would trickle-down to closing a host of other issues... at least with most other issues relating to those two things there is a more elegant workaround which at least somewhat integrates with VS Code even if it might be round-about. With this particular issue, however, I am currently forced to modify .git/hooks/pre-commit and activate the workspace environment there... fortunately my local workspace env doesn't change much. Nevertheless in my case it makes writing dockerfiles for developer use a tricky business for the patchwork that needs to be done.

At the moment I unfortunately have neither the time nor the expertise to contribute - as enticing as that standing offer is. I am just a simple mech e rather than a proper comp sci so I don't have much programming skill outside of decades of python so it will be a while before I would be able to be up-and-running enough to meaningfully contribute.

I am sure we all eagerly await those changes - they sound extremely useful. Maybe some fleshed out CLI documentation and functionality will accompany them too ๐Ÿ˜œ. I will say I am surprised that there is not a bigger team dedicated to Python since it seems like it has one of the highest user bases for VS Code.

On Tue, Aug 22, 2023, 13:49 Brett Cannon @.***> wrote:

@gresavage https://github.com/gresavage it's actually not simple as the SCM component of VS Code has no concept of Python interpreters or environments. So to make this work the SCM system in VS Code core would need to be expanded so we could somehow inject environment details so that git itself picks up. This would probably come about via environment variables and we are actively reworking our environment activation support in the terminal to use environment variables, so hopefully this work can then inform an API change in VS Code core for SCM execution and we can build one feature on top of the other's hard work.

Also note that this issue finally hit the top 5 most upvoted issues https://github.com/microsoft/vscode-python/issues?q=is%3Aissue+is%3Aopen+sort%3Areactions-%2B1-desc, but it's also half the number of upvotes of the first and second issue (and the first issue is actually newer than this one). So while we understand this impacts people, we still have other issues that the community has flagged as more important to them overall. We are definitely not ignoring this issue, but our team is only so big and we are expected to support all Python users in VS Code which is not an insignificant number based on our download count.

But if you believe I have misunderstood the complexity of what it will take to resolve this problem, then this extension and VS Code are open source and we do appreciate and accept community contributions. If you or anyone else wants to propose a fix and a PR for it we will happily review it.

โ€” Reply to this email directly, view it on GitHub https://github.com/microsoft/vscode-python/issues/10165#issuecomment-1688650532, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB45WG6ZTONS52WIU7UTQG3XWTWLRANCNFSM4KW74PJQ . You are receiving this because you were mentioned.Message ID: @.***>

brettcannon commented 1 year ago

Until your comment it seemed like there was little to no activity from devs towards closing this issue.

As you can see by our issue count, there's unfortunately simply too many issues for us to keep all ancillary issues up-to-date while we are working on something. Plus we don't want to give people false hopes on when things will be completed.

Maybe some fleshed out CLI documentation

"CLI documentation" for which CLI? code?

gresavage commented 1 year ago

I appreciate your input. It is nice to know this issue has eyes on it and some impactful changes are indeed on the way which may enable moving this issue along.

"CLI documentation" for which CLI? code?

Yes, both code and code-server

Just about the only commands and options I know the CLI to accept are the ones which are documented explicitly in the CLI documentation (i.e. install extensions, add folders to workspace, set ports). If more exist I don't know of them.

This is all relatively unrelated to the original thread but in particular it would be nice if the CLI supported any of the menu commands. In my case I struggled setting up a Docker container with code-server for development because I couldn't find a CLI command to import a profile. AFAIK devcontainers wouldn't work because it creates a "docker-in-docker" problem. Eventually i settled with saving a code-workspace file but this is limited in its abilities too... such as the position and layout settings of tools.

On Wed, Aug 23, 2023, 14:55 Brett Cannon @.***> wrote:

Until your comment it seemed like there was little to no activity from devs towards closing this issue.

As you can see by our issue count, there's unfortunately simply too many issues for us to keep all ancillary issues up-to-date while we are working on something. Plus we don't want to give people false hopes on when things will be completed.

Maybe some fleshed out CLI documentation

"CLI documentation" for which CLI? code?

โ€” Reply to this email directly, view it on GitHub https://github.com/microsoft/vscode-python/issues/10165#issuecomment-1690478947, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB45WG6M4ZBUAXFUMSNK6ELXWZGZNANCNFSM4KW74PJQ . You are receiving this because you were mentioned.Message ID: @.***>

brettcannon commented 1 year ago

This is all relatively unrelated to the original thread but in particular it would be nice if the CLI

All feedback on the CLI should go to https://github.com/microsoft/vscode .

kfigiela commented 1 year ago

Not sure if this helps, but we had a similar problem, however, we're using pre-commit hooks via nix (https://github.com/cachix/pre-commit-hooks.nix). Nevertheless, I think our workaround could be adapted to other circumstances like python virtual environment. Essentially, we're "polluting" pre-commit hooks, so they'll load the necessary environment without needing VSCode to run in the environment. I hope this helps:

  1. We use direnv to load nix environment
  2. We enable nix in .envrc via use nix. That could be replaced with appropriate code to load python env without the need of nix.
  3. We update pre-commit hook scripts with a small shell script. This is all managed by nix in our case, so pre-commit hooks as well as the patch are installed automatically
    if [ -f .envrc ] && [ -f .git/hooks/pre-commit ]; then
      cat .git/hooks/pre-commit | grep --silent "direnv export bash" || sed --in-place '2 i eval "$(direnv export bash)"' .git/hooks/pre-commit
    fi
gresavage commented 1 year ago

This is all relatively unrelated to the original thread but in particular it would be nice if the CLI

All feedback on the CLI should go to https://github.com/microsoft/vscode .

very good, will do!

@kfigiela very good! That is actually pretty similar to what I've done as well - I also use sed to insert the commands for activating my conda environment in the shell opened by .git/hooks/pre-commit... I will have to look at automating it more as you have done.

My biggest gripe with doing it this way is that if the project/workspace interpreter is changed later on by the user then the script will need to be updated and then sed gets a little trickier if you want to clean up the prior statements. I have not yet been able to discover where VS Code (or VS Code Server, for that matter) store the setting of the workspace interpreter once it's been set through the command pallet (note the setting from command pallet takes precedence over the python.defaultInterpreter setting in settings.json). This is another example of where the documentation, overall API, and especially CLI could be improved

kr-hansen commented 5 months ago

I've been poking around on this and am considering trying to put together an Open Source Contribution (though I'll have some learning to do with VSCode extensions) to try resolving this, but have a couple questions I'm trying to track down/understand before I can do that.

  1. When going to the VSCode Output Tab with "Git" selected, is there a way for me to figure out what shell/terminal that is using? I don't have pre-commit on my system environment, so the fact it is starting it and failing by missing a requirement suggests it isn't using my base system environment.

  2. Re: Discussion on environment variables. I'm using pyenv, which sets an environment variable called VIRTUAL_ENV that seems to point to the version of python in the Terminal that I've assigned to my repo as expected. This may work to pass to SCM when Commit is hit from the UI, as I imagine this environment variable will be set appropriately. Not sure if that's true for other tools like conda. I'm open to trying this though as a first pass, if I can figure out what environment the Git Output in SCM is running in. Or is there something better that the Python Extensions surfaces that it can pass off to the SCM extension?

  3. In the meantime, I've found that having pre-commit in my environment, not installing it with pre-commit install and using the pre-commit extension actually does an ok job as a stop-gap. It doesn't block commits like pre-commit does if there are errors, but if I have the RunOnSave property set to all-hooks, it'll run my standard hooks I want and error in the appropriate environment on the Terminal tab on VSCode. If pre-commit isn't fully installed, than SCM works fine and doesn't get blocked. Using fast linters/formatters like ruff or black work well in this setup. Example of a failure:

    failed_precommit_hook

Either way, I figured I'd surface what I'd learned and prompt with any potential help to try getting momentum on a potential solution going here.

gresavage commented 4 months ago

@kr-hansen thanks for the input. Here is what I can say in regards to (1) and (2)

  1. When going to the VSCode Output Tab with "Git" selected, is there a way for me to figure out what shell/terminal that is using? I don't have pre-commit on my system environment, so the fact it is starting it and failing by missing a requirement suggests it isn't using my base system environment.

In my case it is using /usr/bin/bash, the shebang at the beginning of the hook file should give an clue of which shell is being used. To be certain you can echo the SHELL environment variable from within your /<path>/<to>/<project>/.git/hooks/pre-commit file like so:

echo SHELL="${SHELL}"

This means that there is no project-specific environment configuration being done - just whatever is in ${HOME}/.bashrc

  1. Re: Discussion on environment variables. I'm using pyenv, which sets an environment variable called VIRTUAL_ENV that seems to point to the version of python in the Terminal that I've assigned to my repo as expected. This may work to pass to SCM when Commit is hit from the UI, as I imagine this environment variable will be set appropriately. Not sure if that's true for other tools like conda. I'm open to trying this though as a first pass, if I can figure out what environment the Git Output in SCM is running in. Or is there something better that the Python Extensions surfaces that it can pass off to the SCM extension?

With conda you should call the activate sub-command rather than setting environment variables directly:

conda activate <env_name>