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

Add (experimental) option to use current active python to create venv ("pyenv way") #4852

Merged
merged 5 commits into from
Jan 17, 2022

Conversation

finswimmer
Copy link
Member

@finswimmer finswimmer commented Nov 30, 2021

The "pyenv-way" for switching/creating venvs as described in the docs doesn't work anymore when using the install-poetry.py script.

This Draft-PR is a proof-of-concept to see if it's possible to detect the current running python in the shell, set by pyenv and bring back that functionality and make it also available when installing poetry via pipx.

It will probably never be included in poetry like this, as we don't want to rely on third-party tools if not necessary. But this could be a starting point to find out how to integrate it via a plugin.(Maybe it can be implemented, because I've found a generic way. So it's up to @sdispater if such a solution will be included.)

This PR adds in opt-in option virtualenvs.prefer-active-python. If set to true poetry will try to use the current running python of the shell for creating the venv.

Would be happy if some people could test their usecase with this MR.

Install with pipx:

pip install pipx
pipx install --suffix=@pyenv 'poetry @ git+https://github.com/finswimmer/poetry.git@proof-of-concept-pyenv'
poetry@pyenv --version

Install with install-poetry.py script:

curl -sSL https://install.python-poetry.org | python3 - --git "https://github.com/finswimmer/poetry.git@proof-of-concept-pyenv"

Closes: #4199

@finswimmer finswimmer force-pushed the proof-of-concept-pyenv branch 3 times, most recently from 9d9ac45 to 848af21 Compare November 30, 2021 13:51
@finswimmer finswimmer force-pushed the proof-of-concept-pyenv branch 2 times, most recently from 5ef87d9 to 434fecb Compare November 30, 2021 18:51
@finswimmer finswimmer changed the title proof-of-concept to detect current running python through pyenv Option to use current active python to create venv ("pyenv way") Dec 1, 2021
@finswimmer finswimmer changed the title Option to use current active python to create venv ("pyenv way") Add option to use current active python to create venv ("pyenv way") Dec 1, 2021
@finswimmer finswimmer marked this pull request as ready for review December 1, 2021 05:45
@finswimmer finswimmer requested a review from a team December 1, 2021 05:46
executable = decode(
subprocess.check_output(
list_to_shell_command(
["python", "-c", '"import sys; print(sys.executable)"']
Copy link
Member

Choose a reason for hiding this comment

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

This is not necessarily universal.

Copy link
Member Author

Choose a reason for hiding this comment

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

You mean python? In theory yes.

But if you have e.g. python2 and python3 in your shell but no python it is impossible to decide what python the user expects.

The main use case for the implementation here is for users who switch there python to different versions (e.g. with pyenv). I'm not aware of other tools, but I guess they all will provide a python. As a fallback if no python is found and the option is activated, the default behavior will be used.

The only generic solution for all edge cases I can imagine right now, is to let the user set a "default python executable" that should be used. But in this case I have some security concerns. Running a user provided command listed in a config, doesn't sound good.

Copy link
Member

Choose a reason for hiding this comment

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

This is not just about python2 vs python3. There also are other issues like for .exe extensions on windows etc depending on where poetry is running. Additionally, quite a lot of tools rely on environment magic to wrangle which python version to use etc., this can also be impacted by how the subprocess shell is spawned (eg: is it a login shell?, how does the tool configure the "current" python?, are profile scripts required? is the PATH discoery set correctly? etc.). There are too many variables here. Not sure if this is a show stopper, but we should really consider if this makes things more obscure.

An additional point might be that we should fall back gracefully to compatible version detection than crashing as the option is "prefer" and not "use".

Copy link
Member Author

Choose a reason for hiding this comment

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

An additional point might be that we should fall back gracefully to compatible version detection than crashing as the option is "prefer" and not "use".

This is already the case :)

@tony
Copy link
Contributor

tony commented Dec 31, 2021

QA @ 5c65efd

pipx install --suffix=@pyenv 'poetry @ git+https://github.com/finswimmer/poetry.git@proof-of-concept-pyenv
poetry@pyenv --version

However, if I have a .python-version or .tool-versions (asdf) set, it detects my asdf's global python version, even though my python -V shows I'm using .python-version / .tool-versions config:

Normal shell

~ ❯ python -V
Python 3.7.9
~ ❯ asdf which python
~/.asdf/installs/python/3.7.9/bin/python

Project directory (w/ .python-version)

cd ~/project
❯ cat .python-version
3.9.9
❯ python -V
Python 3.9.9
❯ asdf which python
~/.asdf/installs/python/3.9.9/bin/python
❯ poetry@pyenv shell
The currently activated Python version 3.7.9 is not supported by the project (>=3.9,<3.10).
Trying to find and use a compatible version.
Using python3 (3.9.9)
Creating virtualenv project in ~/project/.venv
Spawning shell within ~/project/.venv
❯ . ~/project/.venv/bin/activate

I'm getting the message from around here:

io.write_line(
f"<warning>The currently activated Python version {python_patch} "
f"is not supported by the project ({self._poetry.package.python_versions}).\n"
"Trying to find and use a compatible version.</warning> "
)

1.1 backport?

Is this something that can be backported to 1.1 potentially?

The reason why is while 1.2 entails a significant release but this, if on 1.1, makes the experience much more convenient.

@finswimmer
Copy link
Member Author

Thanks a lot for your feedback @tony. I'm not familiar with asdf, that's why your feedback is really valuable and I will have a look on it. 👍

Is this something that can be backported to 1.1 potentially?

For now it's unclear if we will will merge this at all or if we try to make a plugin out of it. If we will merge it, I don't think we will backport it to 1.1. There you can have this functionality if you use the get-poetry.py installation script.

@tony
Copy link
Contributor

tony commented Jan 1, 2022

You're welcome and let me know if another QA would be helpful (assuming / if there's any revisions from here)

For now it's unclear if we will will merge this at all or if we try to make a plugin out of it.

Also following this and interested to see where it goes. pyenv and asdf are becoming so common it's interesting to know if we found a generic solution or not

@finswimmer
Copy link
Member Author

@tony: I've found some time to test asdf within a docker container and couldn't find any problems. Did you set virtualenvs.prefer-active-python' to truebypoetry@pyenv config virtualenvs.prefer-active-python true`?

docker run -it  ubuntu:21.10 /bin/bash
apt-get update
apt-get install curl git nano make build-essential libssl-dev zlib1g-dev \
libbz2-dev libreadline-dev libsqlite3-dev wget curl llvm \
libncursesw5-dev xz-utils tk-dev libxml2-dev libxmlsec1-dev libffi-dev liblzma-dev
git clone https://github.com/asdf-vm/asdf.git ~/.asdf --branch v0.8.1
echo '. $HOME/.asdf/asdf.sh' >> ~/.bashrc 
echo '. $HOME/.asdf/completions/asdf.bash' >> ~/.bashrc
exec "$SHELL"
asdf plugin add python
asdf install python 3.7.12
asdf install python 3.9.9
asdf global python 3.7.12 3.9.9
curl -sSL https://install.python-poetry.org | python3 - --git "https://github.com/finswimmer/poetry.git@proof-of-concept-pyenv"
export PATH="/root/.local/bin:$PATH"
poetry config virtualenvs.prefer-active-python true
poetry new my-project
cd my-project
asdf local python 3.9.9
python --version
Python 3.9.9
poetry install
poetry run python --version
Python 3.9.9

@tony
Copy link
Contributor

tony commented Jan 2, 2022

@finswimmer I made a mistake, I didn't test your PR with poetry@pyenv config virtualenvs.prefer-active-python true since it detected the correct python version for me out of the box.

And upon further examination, I'm detecting my asdf's project-based python correctly on 1.1.12 installed on install-poetry.py (but not 1.1.7 via get-poetry.py).

A shot in the dark: Do we have any high-level, integration-level tests for get-poetry.py + install-poetry.py and creating an environment across poetry versions? This is something where it'd be helpful to have a matrix of where it works and is broke. Would it benefit if I made some pytest ones for python-poetry/install.python-poetry.org repo?

Enlisting more assistance since there are asdf-poetry users affected by this:

@shawon-crosen @crflynn @Kurt-von-Laven As you are part of a related issue at asdf-community/asdf-poetry#10 this relates to you, would you like to QA finswimmer's 1.2 branch and see if it fails to detect your python version? Without and with poetry@pyenv config virtualenvs.prefer-active-python true?

@tony
Copy link
Contributor

tony commented Jan 3, 2022

Good news:

Assume asdf has 3.10.0 and 3.10.1 are installed w/ project's .tool-versions set to python 3.10.1:

  • 🚫 poetry@pyenv via pipx install with poetry@pyenv config virtualenvs.prefer-active-python false: virtualenv installed 3.10.0
  • poetry@pyenv via pipx isntall with poetry@pyenv config virtualenvs.prefer-active-python true virtualenv installed 3.10.1

With this PR, the minor version works correctly

@finswimmer
Copy link
Member Author

Sounds good @tony 👍

Do we have any high-level, integration-level tests for get-poetry.py + install-poetry.py and creating an environment across poetry versions? This is something where it'd be helpful to have a matrix of where it works and is broke.

We don't have something like this yet. We would like to have some kind of end-to-end-test for several usecases, in different environments and with different settings. But I'm afraid this is nothing what can be done easily, because the possible combinations are endless ...

For now I'm happy to see, that the implementation seems to work for pyenv and asdf, which should cover a relevant among of users.

I would like to hear something from people, that have use cases with virtualenvs.create set to false and/or virtualenvs.options.system-site-packages set to true. Does poetry behave as expected here, when virtualenvs.prefer-active-python is set to true?

@crflynn
Copy link

crflynn commented Jan 3, 2022

I borrowed @finswimmer's shell commands and have a simple Docker test here using asdf: https://github.com/crflynn/poetry-current-active-python-test. It looks to be working correctly 👍 .

@finswimmer finswimmer changed the title Add option to use current active python to create venv ("pyenv way") Add (experimental) option to use current active python to create venv ("pyenv way") Jan 16, 2022
Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

This change is relatively minor, marked experimental, and arguably is less surprising behavior for the end user. I think we're good to go ahead and merge this, and tweak it if needed in the future.

Personally, I think that we should check the current python3 instead of python given that Python 2 is well and truly dead now, but we can correct that later if it's an issue.

@neersighted neersighted merged commit 619488c into python-poetry:master Jan 17, 2022
@Kurt-von-Laven
Copy link

@neersighted, I don't have a lot of expertise on the topic, but that makes sense to me. This may already be what you are suggesting, but to clarify I would think it wise to also continue to check python if python3 is absent?

@neersighted
Copy link
Member

neersighted commented Jan 18, 2022

@neersighted, I don't have a lot of expertise on the topic, but that makes sense to me. This may already be what you are suggesting, but to clarify I would think it wise to also continue to check python if python3 is absent?

We only support Python 3 (Python 2 has been out of support for years now) and a python3 should always be present (both a PEP and distro packaging guidelines call for this) when Python is installed. Therefore I see no need to check both as the python binary is either Python 2 (unsupported), Python 3 (good), or does not exist.

Some distros provide python as a symlink to python3, and others require a package like Debian's python-is-python3 or FreeBSD's lang/python.

That being said, asdf and pyenv will both work with this change, and I can see this version as being more in line with the principle of least surprise. We can always change it if needed.

@Kurt-von-Laven
Copy link

Thank you for the explanation! That was very educational.

Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
6 participants