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

pyenv init - performance improvements; recommend using pyenv init - <shell> #3136

Merged
merged 15 commits into from
Dec 22, 2024

Conversation

ChristianFredrikJohnsen
Copy link
Contributor

Make sure you have checked all steps below.

Prerequisite

  • Please consider implementing the feature as a hook script or plugin as a first step.
    • pyenv has some powerful support for plugins and hook scripts. Please refer to Authoring plugins for details and try to implement it as a plugin if possible.
  • Please consider contributing the patch upstream to rbenv, since we have borrowed most of the code from that project.
    • We occasionally import the changes from rbenv. In general, you can expect changes made in rbenv will be imported to pyenv too, eventually.
    • Generally speaking, we prefer not to make changes in the core in order to keep compatibility with rbenv.
  • My PR addresses the following pyenv issue (if any)

Description

  • Here are some details about my PR
  1. Changed the local pyenv function generated by pyenv init -, and replaced calls to cat with calls to echo.

Instead of having:

pyenv() {
  local command
  command="${1:-}"
  if [ "$#" -gt 0 ]; then
    shift
  fi

  case "$command" in
  activate|deactivate|rehash|shell)
    eval "$(pyenv "sh-$command" "$@")"
    ;;
  *)
    command pyenv "$command" "$@"
    ;;
  esac
}

We now get:

pyenv() {
  local command=$1
  [ "$#" -gt 0 ] && shift
  case "$command" in
  activate|deactivate|rehash|shell)
    eval "$(pyenv "sh-$command" "$@")"
    ;;
  *)
    command pyenv "$command" "$@"
    ;;
  esac
}
  1. DOCUMENTATION: Update README.md to make people specify their shell when setting up pyenv init in their shell configuration file.

Example:

Under the section: Set up shell environment for zsh:

echo 'eval "$(pyenv init - zsh)"' >> ~/.zshrc

Instead of

echo 'eval "$(pyenv init -)"' >> ~/.zshrc

My computer uses 50ms on pyenv init -, and 30ms on pyenv init - zsh. Simple speedup.

  1. General code cleanup in pyenv-init
    Removed unnecessary declarations of variables: (e. g. no_rehash=""), replaced a series of if-statements with a case block (code essentially imported from rbenv), and change completions path such that we get:
source '/home/christian/.pyenv/completions/pyenv.zsh'

(essentially PYENV_ROOT/completions/pyenv.zsh)

instead of:

source '/home/christian/.pyenv/libexec/../completions/pyenv.zsh'

(equal to something like: $(dirname "$(realpath pyenv-init)")/../completions/pyenv.zsh)

RESULTS:

The first command is run by master branch, second command is used with perf improvements.

Bildschirmfoto vom 2024-12-19 16-50-47
Bildschirmfoto vom 2024-12-19 16-52-16

Not much to speak of, but will hopefully come with more improvements later. Interestingly, the speedup seems to be more significant when zsh is specified.

Tests

  • My PR adds the following unit tests (if any)

3 tests have been modified, all inside test/init.bats.

  • fish instructions: Now expects pyenv init - fish | source
  • setup shell completions and setup shell completions (fish): PYENV_ROOT is exported before running pyenv init - <shell>.
@test "setup shell completions" {
  root="$(cd $BATS_TEST_DIRNAME/.. && pwd)"
  export PYENV_ROOT=$root
  run pyenv-init - bash
  assert_success
  assert_line "source '${root}/completions/pyenv.bash'"
}

Assumes that BATS_TEST_DIRNAME/.. is equal to PYENV_ROOT. Enables us to have a simpler path for sourcing of completions, as mentioned in point (3) above.

Copy link
Member

@native-api native-api left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! An almost twofold speedup just from specifying the shell is definitely a noteworthy discovery!
I guess the rest comes from case and echo?
It might be worth specifying in the README, in the advanced shell setup section, that pyenv init - does work without an argument but specifying the shell explicitly results in an almost twofold speedup.

@ChristianFredrikJohnsen
Copy link
Contributor Author

Reverted to using:

function print_completion() {
  completion="${0%/*/*}/completions/pyenv.${shell}"
  if [ -r "$completion" ]; then
    echo "source '$completion'"
  fi
}

Inside /libexec/pyenv-init.
I see this as a quick fix, but it does the job.

The major obstacle in making any further progress is the fact that homebrew has a directory setup which is quite bad from my viewpoint. It has core functionality like libexec and completions in this path:

/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23/libexec/pyenv-init

While shims and python versions are stored in PYENV_ROOT, which is set to ~/.pyenv. I don't see why you wouldn't want to set

PYENV_ROOT=/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23

Instead of having it set to ~/.pyenv, since we now have a situation where shims and python versions are stored outside of the actual executable directory of pyenv.

Additonally, if you install pyenv-virtualenv via homebrew, it is installed in a directory like:

/home/linuxbrew/.linuxbrew/Cellar/pyenv-virtualenv/1.2.4

instead of

/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23/plugins

It's very confusing, since even in the README, we have this note:

Pyenv plugins
...
The main idea is that most things that you can put under $PYENV_ROOT/<whatever> you can also put under $PYENV_ROOT/plugins/your_plugin_name/<whatever>.
...

Which tells us to drop plugins in PYENV_ROOT/plugins, but if you do this with homebrew, you will end up with:

/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23/plugins
/home/linuxbrew/.linuxbrew/Cellar/pyenv-virtualenv/1.2.4
~/.pyenv/plugins

And your plugins are now all over the place. Of course, it works, but is very confusing to a developer which comes from the setup you get when installing via pyenv run
Crucially though, it doesn't allow us to optimize very aggressively, because scripts can be in all sorts of places.

My conclusion is that it would be very nice to have the homebrew installation standardized to the same format as the installation you get from pyenv run.

handling for case where unknown option is provided.

- Error handling involves writing error message to stderr, and
  forcefully exiting the init script, forcing user to fix their setup.
- Local testing shows that you get a trivial 1.6x speedup in command execution
  time (over doing `pyenv init -`) by specifying which shell you are using.
to specify the shell they are using when adding:
`eval $(pyenv init - <shell>)` to their config file.
pyenv-init.

- It turns out that the code I wrote was very similar to what was
  already present in rbenv-init.

As rbenv is referenced when I am writing a pull request, I am assuming
that maintainers want code similar to what's already there.

WARNING:
The code will now not validate whether the specified shell is valid or
not, and if you make a silly mistake like writing `--parh` instead of
`--path`, the shell may be set to `--parh` and so forth. But I guess
this is acceptable.
…urce' instead of 'pyenv init - | source'.
The old tests seems to have been copy-paste from some 11 year old
rubyenv setup, and included a long and windy pathname. The old setup was
extremely bad in my view, current test is much better.

Exported variables are local to the specific test, so they will not
affect other test instances.

---> Enables us to remove unnecessary `root` variable in pyenv-init.
echo is a bash built-in, and thus you don't need to launch a new
process for something as simple as writing to stdout.
Saves about 2-3ms.
print_shell_function().

-> Maintainers prefer single quotes instead of double quotes, since it
allows us to look away from tracking what to escape.
Tell users that `pyenv init -` works, but that specifying the shell
which is used is preferable, as it reduces launch time.
Homebrew installations have PYENV_ROOT set to `~/.pyenv`, while the
actual completion file resides in /home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23/completions/pyenv.zsh

Thus, if you try to use `$PYENV_ROOT/completions/pyenv.${shell}`, the
completions file will not be found, since you are pointing to:

`~/.pyenv/completions/pyenv.zsh`

when the actual location is:

`/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23/completions/pyenv.zsh`

This problem is resolved by using the parent folder of the currently
executing file:

`/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23/libexec/pyenv-init`

which is

`/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23`

And then go to completions from there.

I don't like the fact that homebrew installations store some things in
`/home/linuxbrew/.linuxbrew/Cellar/pyenv/2.4.23/libexec/pyenv-init`

while stuff like `shims` and python versions are stored in `~/.pyenv`, but
that's how it currently is.
In `test_helper.bash`:
set

PATH="${BATS_TEST_DIRNAME%/*}/libexec:$PATH"

instead of

PATH="${BATS_TEST_DIRNAME}../libexec:$PATH"

it essentially enables us to expect

`${exec_root}/completions/pyenv.bash`

instead of something like

`${exec_root}/test/../completions/pyenv.zsh
Copy link
Member

@native-api native-api left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only some nitpicks remaining. Good job!

…\` setup instead of `echo

'`

1) If people are using bash with `set -u` (enable unset variable
   checking), then they will get an error if they run `pyenv` in the
   CLI under the current setup. To avoid any errors when no positional
   arguments, we use ${1:-} instead of $1.

2) We find that the script looks better when using:

```bash
echo \
'pyenv() {
  local command=${1:-}'
```

As opposed to:

```
echo 'pyenv() {
  local command=${1:-}'
```
@native-api native-api merged commit c697339 into pyenv:master Dec 22, 2024
@native-api native-api changed the title pyenv-init | performance improvements pyenv-init performance improvements Dec 22, 2024
@native-api native-api changed the title pyenv-init performance improvements pyenv init - performance improvements; recommend using pyenv init - <shell> Dec 22, 2024
@ChristianFredrikJohnsen ChristianFredrikJohnsen deleted the perf/pyenv-init branch December 22, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants