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

Improved venv inspection #593

Merged
merged 225 commits into from
Jan 16, 2021
Merged

Conversation

itsayellow
Copy link
Contributor

@itsayellow itsayellow commented Dec 27, 2020

  • I have added an entry to docs/changelog.md

Summary of changes

This is the successor to (and based on) #588 .
Closes #587. Closes #534. Closes #528 .

This integrates the purpose of venv_metadata_inspector.py into code that is part of the pipx package and is no longer stand-alone. The breakthrough is that importlib.metadata allows us to specify the path for it to search for package metadata. This means that the venv inspection code no longer needs to run in the venv environment itself, but simply specify what sys.path would be if the venv environment were active.

EDIT: We can also fetch the venv environment of environment markers and apply it when evaluating package requirements to determine if a dependency is a match for the environment (see packaging.markers.Marker.evaluate).

The new code does not need to be run in a subprocess nor exchange data via json, and benefits from logging and python3.6+ of being a native part of pipx.

There is only one tiny subprocess that runs inside the venv to be examined. It gets the venv python version and what sys.path is when the venv is operational. I used the entire sys.path of the activated venv because that is the existing behavior, and also in case the user includes options to use system libraries to fulfill dependencies. EDIT: the subprocess now also fetches a dict specifying the venv environment for purposes of matching environment markers.

Additionally a huge speedup of the inspection for certain packages has been accomplished. These two lines are responsible for up to a 2 second speedup in certain cases!
https://github.com/pipxproject/pipx/blob/48f6fe60169ef65120dc374152ed5d40a3a36bf2/src/pipx/venv_inspect.py#L92-L93
get_apps() was taking a huge amount of time, and the loop that checks each file was taking all the time. This if-statement was a much faster way of knowing that the file is not an app than the pathlib checks in the loop.
Some selected packages and venv inspection time (start to finish) are shown below. "Optimization" refers to the two lines in the code linked to above.

venv inspection times (less is better)

package this PR pipx v0.15.6.0 this PR no optimization
ansible==2.9.13 259ms 1738ms 1689ms
awscli 176ms 1140ms 998ms
beancount 484ms 1110ms 820ms
cloudtoken 239ms 697ms 462ms
kibitzr 380ms 838ms 779ms
kolibri 270ms 2456ms 2455ms
magic-wormhole 536ms 1410ms 1028ms
mkdocs 153ms 324ms 413ms
sphinx 274ms 795ms 394ms

After this PR merges I hope to release pipx 0.16.0.0!

60+ packages (pipx slow tests) pass fine
https://github.com/itsayellow/pipx/actions/runs/489220534

Test plan

Tested by running

# command(s) to exercise these changes

src/pipx/package_specifier.py Outdated Show resolved Hide resolved
src/pipx/package_specifier.py Outdated Show resolved Hide resolved
src/pipx/venv.py Show resolved Hide resolved
src/pipx/venv_inspect.py Outdated Show resolved Hide resolved
@itsayellow
Copy link
Contributor Author

The "small subprocess code" has grown to be a bit larger than it started. I'm happy to break it out into a separate file if people think that's a good idea. Let me know if people are interested in that.

@uranusjr
Copy link
Member

uranusjr commented Dec 28, 2020

Maybe name the script venv_metadata_inspector.py and restore the mechanism used to call it? :p

@itsayellow
Copy link
Contributor Author

Maybe name the script venv_metadata_inspector.py and restore the mechanism used to call it? :p

Yes I guess so. :) That whole mechanism seemed to so tortured to me, but I can't argue with success.

@itsayellow
Copy link
Contributor Author

@uranusjr I can't tell how serious your request is with the :p emoji. 🙂

Do you think it's ok to keep the command-string inline? I kind of like it as it is because it's so simple and obvious. And I remember having issues finding the old venv_metadata_inspector.py in the code when I was doing experiments freezing the code with PyInstaller.

Plus right now it's not all that complicated code.

@uranusjr
Copy link
Member

uranusjr commented Dec 29, 2020

I think at its current length it's okay to do this either way. If you decide to make this a standalone file, all previous mechanisms should return (of course), but I'd want the file to be named something different; venv_metadata_inspector.py is pretty vague and arguably does not reflect what your current script actually does.

@itsayellow
Copy link
Contributor Author

Ah ok, then I'm totally in agreement. I think since the code right now is about one function in length, I like leaving it where it is.

@itsayellow
Copy link
Contributor Author

@pipxproject/maintainers , I'm probably going to merge this in the next day or two unless there are any objections. Directly afterwards I'll release 0.16.0.0!

Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@itsayellow itsayellow merged commit 271baaa into pypa:master Jan 16, 2021
@itsayellow itsayellow deleted the native_venv_inspection branch April 13, 2021 22:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants