Improve performance and output of pyenv virtualenvs
#502
Conversation
Use the code from pyenv-versions for efficiency and consistent output. The main performance problem was in the call to pyenv-virtualenv-prefix, which called pyenv-prefix, which then enumerated every virtual environment. This was done inside a loop, compounding the problem. Simply the virtual environment listing so that it does not have to call pyenv-virtualenv-prefix anymore.
This is useful for listing the “frinedly” virtualenv names instead of the short path which includes the Python version.
…vironments Having the current virtual environments listed as options in the competion is noisy since only bare Python versions, such as 3.11.1, make sense as suggested completions for `pyenv virtualenv [version]`.
This makes the suggested completetions cleaner.
|
I could use some help getting the tests correct. I made a few changes but I don't understand why the |
|
Seems like only two tests are failing 😄 I just randomly checked into virtualenv. I recently did some perf improvements on the main I am not sure if this applies to your situation as well, haven't looked much into it. The main idea I have is:
|
|
The tests do need some adjusting since I've changed the output a bit. I'm just having a hard time figuring out what exactly |
|
Hehe, and the test file hasn't been touched in 10 years 😆 I did not encounter |
|
The code itself that was the source of the performance issue is 10-12 years old. Not that old code is bad, but there were some inefficiencies in there. |
|
No it's not that the old code is bad. It was probably good at the time it was written. It's more of a problem that the code has design principles which made sense back then, but fast forward 10 years and it's not as intuitive anymore. And if you're unlucky, you're facing tests which were quick-fix set up 10 years ago, and suddenly you need to fix the problem of ... It's a challenge for sure 😄 |
| @@ -34,10 +34,11 @@ for arg; do | |||
| case "$arg" in | |||
| --complete ) | |||
| echo --bare | |||
| echo --skip-aliases | |||
| echo "${miss_prefix}${1}${print_origin+$2}" | ||
| version_repr="$version" | ||
| fi | ||
| if [[ ${BASH_VERSINFO[0]} -ge 4 && ${current_versions["$1"]} ]]; then |
There was a problem hiding this comment.
Version check inconsistent with other cases. Not a malfunction but a code smell. Is the difference significant?
| if ! enable -f "${BASH_SOURCE%/*}"/pyenv-realpath.dylib realpath 2>/dev/null; then | ||
| if [ -n "$PYENV_NATIVE_EXT" ]; then | ||
| echo "pyenv: failed to load \`realpath' builtin" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| READLINK=$(type -P readlink) | ||
| if [ -z "$READLINK" ]; then | ||
| echo "pyenv: cannot find readlink - are you missing GNU coreutils?" >&2 | ||
| exit 1 | ||
| fi | ||
|
|
||
| resolve_link() { | ||
| $READLINK "$1" | ||
| } | ||
|
|
||
| realpath() { | ||
| local path="$1" | ||
| local name | ||
|
|
||
| # Use a subshell to avoid changing the current path | ||
| ( | ||
| while [ -n "$path" ]; do | ||
| name="${path##*/}" | ||
| [ "$name" = "$path" ] || cd "${path%/*}" | ||
| path="$(resolve_link "$name" || true)" |
There was a problem hiding this comment.
We already source pyenv-virtualenv-realpath. Does it not work or smth?
The new code doesn't seem to even be using realpath -- if that's correct, neither is needed.
| if [ -z "$only_aliases" ]; then | ||
| for env_path in "${venv_dir_entries[@]}"; do | ||
| if [ -d "${env_path}" ]; then | ||
| print_version "${env_path#"${PYENV_ROOT}"/versions/}" "${env_path}" | ||
| fi | ||
| virtualenv_prefix="$(pyenv-virtualenv-prefix "${path##*/}" 2>/dev/null || true)" | ||
| if [ -d "${virtualenv_prefix}" ]; then | ||
| print_version "${path##*/}" " (created from ${virtualenv_prefix})" | ||
| done | ||
| fi | ||
|
|
||
| if [ -z "$skip_aliases" ]; then | ||
| for env_path in "${version_dir_entries[@]}"; do | ||
| if [ -d "${env_path}" ] && [ -L "${env_path}" ]; then | ||
| print_version "${env_path#"${PYENV_ROOT}"/versions/}" "${env_path}" | ||
| fi | ||
| for venv_path in "${path}/envs/"*; do | ||
| venv="${path##*/}/envs/${venv_path##*/}" | ||
| virtualenv_prefix="$(pyenv-virtualenv-prefix "${venv}" 2>/dev/null || true)" | ||
| if [ -d "${virtualenv_prefix}" ]; then | ||
| print_version "${venv}" " (created from ${virtualenv_prefix})" | ||
| fi | ||
| done | ||
| fi | ||
| done | ||
| done | ||
| fi |
There was a problem hiding this comment.
What happens with conda installations? They are currently listed, now they seem to stop being so.
Not sure why they are listed and if that's intended. Technically, each one contains a "base" environment -- but the same could be said about any other Python installation.
| ln -s "venv27" "${PYENV_ROOT}/versions/venv27" | ||
| ln -s "venv33" "${PYENV_ROOT}/versions/venv33" |
There was a problem hiding this comment.
AFAICS the links are created in the current directory. That doesn't seem correct.
There's a dedicated function to create aliases.
| 2.7.6/envs/venv27 | ||
| * 3.3.3/envs/venv33 |
There was a problem hiding this comment.
Diagnosed this failure. The logic does not equate "xxx" with "yyy/envs/xxx". If you set current version to "venv33", the alias will be the highlighted entry, not the full name.
To diagnose stuff in tests, I set PS4 to the long value from the launcher and run the command with PYENV_DEBUG=1. Then make sure there's a command later that prints its output in some form (in this case, assert_output failure handling logic).
| stub pyenv-virtualenv-prefix "venv27 : echo \"${PYENV_ROOT}/versions/2.7.6\"" | ||
| stub pyenv-virtualenv-prefix "venv33 : echo \"${PYENV_ROOT}/versions/3.3.3\"" |
There was a problem hiding this comment.
Not sure if this change improves anything. As a rule of thumb, one should not change test cases unless you've changed the behavior in them -- to avoid accidentally losing test cover.
| OUT | ||
|
|
||
| unstub pyenv-version-name | ||
| unstub pyenv-virtualenv-prefix | ||
| # unstub pyenv-version-name |
There was a problem hiding this comment.
unstub checcks if the specific stubbed executable was called exactly according to the sequence set up with stub calls. If not, it fails and prints the plan and the fact.
The implementation is in test_helper.bash and the stubs/stub script which the former calls.
| @@ -17,7 +17,7 @@ set -e | |||
| # Provide pyenv completions | |||
| if [ "$1" = "--complete" ]; then | |||
| echo --unset | |||
| exec pyenv-virtualenvs --bare | |||
| exec pyenv-virtualenvs --bare --only-aliases | |||
There was a problem hiding this comment.
This is a design change -- so this should be discussed separately. Envs are supposed to be accessible under both full name and alias. I'm not sure why that is; aliases are internally called "compat", indicating that they were added for compatibility with... something.
| @@ -51,7 +51,7 @@ while [ $# -gt 0 ]; do | |||
| "--complete" ) | |||
| # Provide pyenv completions | |||
| echo --unset | |||
| exec pyenv-virtualenvs --bare | |||
| exec pyenv-virtualenvs --bare --only-aliases | |||
|
Thank you for the feedback @native-api. I'll address this later in the week. |
|
I had a crazy week and a full weekend away from home. I will work on this during the week. |
The biggest change is reusing most of the code used for
pyenv versionsinpyenv virtualenvs. This dramatically improves performance while making the output consistent.Related to #490. Maybe fixes the problem?
With 128 virtual environments, it currently takes about 13 seconds to run
pyenv virtualenvs --bare.Output
With the changes in this branch it is 236ms:
Output
Output formatting changes
This PR also includes changes to the output format to make it more consistent with
pyenv versions.pyenv versions output
There is now an
--only-aliasesflag. I found this useful to make the shell completions less verbose.The completions for
pyenv virtualenvnow omit aliases and envs. I believe only installed Python versions should be suggested instead of current virtual environments, which are aliases.