search

pyenv / pyenv-virtualenv

a pyenv plugin to manage virtualenv (a.k.a. python-virtualenv)
MIT License
6.3k stars 398 forks source link

Fix zsh shell hook performance issue #456

Closed reegnz closed 1 year ago

reegnz commented 1 year ago

The precmd hook is causing a significant overhead on every command executed in the shell. The shell hook does not need to be this strict however, it can also run only on directory changes, ensuring that the shell remains snappy while executing commands.

Also contains a similar fix for fish.

Fixes #259.

furudean commented 1 year ago

Is checking for $PWD in fish sufficient? What if the virtualenv changes without you changing directory?

What do you think about the (second part of the) workaround outlined in https://github.com/pyenv/pyenv-virtualenv/issues/259#issuecomment-1502164726?

reegnz commented 1 year ago

What if the virtualenv changes without you changing directory?

As a zsh poweruser I can safely state that the changing venv is a way too infrequent edge-case to sacrifice shell performance for. In case you'd still run into that, you can cd out from the dir and back again. People seem to be OK with not handling that edge-case but re-gaining shell performance.

TBQH I'm not a fish user, so I might remove the fish fix and let someone with fish experience fix that one.

Gonna look into the --no-rehash solution. EDIT: nope, the --no-rehash solution does not eliminate the lag that comes from forking python process on every shell command.

native-api commented 1 year ago

As a zsh poweruser I can safely state that the changing venv is a way too infrequent edge-case to sacrifice shell performance for.

That's actually the sole job of pyenv local.

native-api commented 1 year ago

Currently, the shell cmd

As a zsh poweruser I can safely state that the changing venv is a way too infrequent edge-case to sacrifice shell performance for.

That's actually the sole job of pyenv local.

So by not rechecking after an invocation, you single-handedly break this entire command.

reegnz commented 1 year ago

@native-api interesting, because when I use pyenv local 3.11.2 it immediately activates the pyenv for me using precwd, it's not broken. I wonder why that is, because I'd also expect it to break.

native-api commented 1 year ago

@native-api interesting, because when I use pyenv local 3.11.2 it immediately activates the pyenv for me using precwd, it's not broken. I wonder why that is, because I'd also expect it to break.

I suggest tracing. I'm also very curious.

native-api commented 1 year ago

The current hook is rather dumb -- it just activates the selected environment blindly.

AFAICS, the most productive way, instead of trying to dodge executing it altogether and hope no-one notices, would be to make it more intelligent and not do any unnecessary work.

furudean commented 1 year ago

TBQH I'm not a fish user, so I might remove the fish fix and let someone with fish experience fix that one.

I would really dislike the behavior as implemented for fish shell. It seems very unintuitive. Maybe let's settle for just the zsh behavior for this PR. I wouldn't mind championing this as a different issue.

furudean commented 1 year ago

See also #451 and https://github.com/pyenv/pyenv-virtualenv/issues/338

reegnz commented 1 year ago

Removed the fish parts.

Gonna spend some additional time to figure out how to best deal with the zsh precmd use-case. I checked yesterday and for me the hook in general doesn't seem to set VIRTUALENV_DIR and other env vars (even without these changes). Maybe it's set up incorrectly for me. (Still have to understand a bit more what exactly the env vars bring to the table compared to the pyenv shims).

native-api commented 1 year ago

We aren't merging anything that breaks things. Or we'll be swamped in bug reports that things no longer work and will have to fix that pronto in addition to reputational damages.

reegnz commented 1 year ago

Do you have any intention of fixing the highest voted issue on your project? Closing the PR as a twitching reflex is not constructive. I would have been open to doing this in a backward-compatible manner given some feedback. You just lost a contributor.

native-api commented 1 year ago

PRs can be reopened. And it seems to me that feedback is exactly what I gave in the closing message.

I closed it rather than just request changes because the way it is now, it looks like it has to be completely different to actually fix the issue -- so there's little point in keeping it in this topic, you'll have to start a new branch from scratch anyway.

native-api commented 1 year ago

...So I honestly don't see what the big deal is here.

Sorry if I came as haughty/condescending/annoyed, I had no intention to insult anyone. I am annoyed because people offering flawed solutions are kinda wasting everyone's time and giving other people false hope ("Oh, there already is a fix in the works! So I don't have to look into this myself!").