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

Use compatible Python executable installed by Pyenv #4164

Closed
wants to merge 20 commits into from
Closed

Use compatible Python executable installed by Pyenv #4164

wants to merge 20 commits into from

Conversation

ggicci
Copy link

@ggicci ggicci commented Jun 9, 2021

Pull Request Check List

Resolves: #4079 #4101

  • Added tests for changed code.
  • Updated documentation for changed code.

A new config item virtualenvs.use-pyenv with default value true has been instroduced.

Features

Integrate pyenv with poetry's discovery logic

If no compatible versions were found in the system, poetry will keep looking for compatible Python interpreters installed by pyenv.

Fixes

  1. Poetry uses wrong Python version #4101 Create virtualenvs by passing the absolute path to the Python executable to virtualenv.
  2. Incorrect sorting algorithm to sort the available Python versions.

@ggicci
Copy link
Author

ggicci commented Jun 10, 2021

Test output here:

$ python2 -V
Python 2.7.17

$ python3 -V
Python 3.6.9

$ pyenv versions
* system (set by /home/ggicci/.pyenv/version)
  3.7.10
  3.8.10
  3.9.4

$ cd ~/workspace/src/ggicci.me/poetry-pyenv-test
$ grep "python =" pyproject.toml
python = "~3.7"

$ poetry@local -vvv install
Loading configuration file /home/ggicci/.config/pypoetry/config.toml
The currently activated Python version 3.6.9 is not supported by the project (~3.7).
Trying to find and use a compatible version. 
Pyenv loaded: /home/ggicci/.pyenv/bin/pyenv
Trying python3
Trying python3.7
Trying /home/ggicci/.pyenv/versions/3.7.10/bin/python3.7
Using /home/ggicci/.pyenv/versions/3.7.10/bin/python3.7 (3.7.10)
Creating virtualenv poetry-pyenv-test-gx4qtdVT-py3.7 in /home/ggicci/.cache/pypoetry/virtualenvs
Using virtualenv: /home/ggicci/.cache/pypoetry/virtualenvs/poetry-pyenv-test-gx4qtdVT-py3.7
Installing dependencies from lock file

Finding the necessary packages for the current system

@sonarcloud
Copy link

sonarcloud bot commented Jun 24, 2021

Kudos, SonarCloud Quality Gate passed!

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

poetry/utils/env.py Outdated Show resolved Hide resolved
):
if "." not in python_to_try:
if parse_constraint(f"^{python_to_try}.0").allows_any(supported_python):
candidates.append("python" + python_to_try)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
candidates.append("python" + python_to_try)
candidates.append(f"python{python_to_try}")

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggestion to improve loop performance: assign the append function to a variable before starting the for loop to avoid dots inside the loop:

Example:

append = candidates.append

So inside the for loop you just use the append(...) directly.

For more information, I found this list with some examples (see item 3):
https://newrelic.com/blog/best-practices/python-performance-tips

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for this good suggestion. I never noticed this tip of performance improvement before.

poetry/utils/env.py Outdated Show resolved Hide resolved
poetry/utils/env.py Outdated Show resolved Hide resolved
return self._command is not None

def load(self) -> None:
if self._command is not None:
Copy link
Contributor

Choose a reason for hiding this comment

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

Why don't you do it like this?

Example:

    def load(self) -> None:
         if self._command is None:
              self._command = self._locate_command()

Copy link
Author

@ggicci ggicci Jul 22, 2021

Choose a reason for hiding this comment

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

I prefer putting "fast fail" branches of statements in the head of a method and leaving "main logic" at the last. Maybe the pros of this methodolgy is not obivious here.

tests/utils/test_env.py Outdated Show resolved Hide resolved
tests/utils/test_env.py Outdated Show resolved Hide resolved
tests/utils/test_env.py Outdated Show resolved Hide resolved
tests/utils/test_env.py Outdated Show resolved Hide resolved
tests/utils/test_env.py Outdated Show resolved Hide resolved
@ggicci ggicci closed this Dec 3, 2021
@kytta
Copy link

kytta commented Dec 18, 2021

@ggicci why did you close this PR?

@ggicci
Copy link
Author

ggicci commented Dec 19, 2021

This pr brought too many changes and should break down into small pieces. @NickKaramoff

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
Development

Successfully merging this pull request may close these issues.

Automatic Activation of Virtual Environment and integration with Pyenv
3 participants