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

[20.0.5+] 280ms overhead on python startup #1682

Closed
asottile opened this issue Mar 1, 2020 · 12 comments
Closed

[20.0.5+] 280ms overhead on python startup #1682

asottile opened this issue Mar 1, 2020 · 12 comments

Comments

@asottile
Copy link
Contributor

@asottile asottile commented Mar 1, 2020

Issue

The changes to site.py introduce an (unacceptable imo) 280ms startup penalty under python 2.x

I created virtualenvs via the following

rm -rf venv venv_20_0_4 20_0_5
virtualenv venv
venv/bin/pip install virtualenv==20.0.4
venv/bin/virtualenv venv_20_0_4 -p python2
venv/bin/pip install virtualenv==20.0.5
venv/bin/virtualenv venv_20_0_5 -p python2

comparing the startup time between the two:

$ time venv_20_0_4/bin/python -c ''

real	0m0.011s
user	0m0.005s
sys	0m0.005s
$ time venv_20_0_5/bin/python -c ''

real	0m0.291s
user	0m0.214s
sys	0m0.027s

this is (almost certainly) due to the import of setuptools in the critical path:

$ time venv_20_0_4/bin/python -c 'import setuptools'

real	0m0.212s
user	0m0.187s
sys	0m0.025s
@asottile asottile added the bug label Mar 1, 2020
@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Mar 1, 2020

Well, unacceptable in your case, but first we need to be correct over fast and without this brew python on macOS does not work correctly. That being said let me think how else can we do this patching in a lazy way.🤔

@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Mar 1, 2020

The path here would be to install an import hook that does the fixup post-import, rather than unconditionally at startup. This could be injected during the site.py import on Python 2... and via a pth file on Python 3.

@asottile

This comment has been minimized.

Copy link
Contributor Author

@asottile asottile commented Mar 1, 2020

can we only put the distutils hack in when it's needed (instead of unconditionally?)

@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Mar 1, 2020

No because most python use setuptools. And not sure how would you put it conditionally, we don't know when will be needed. Setuptools doesn't fallback on distutils but has its own copy of distribution definition vendored 🤷‍♂️

@asottile

This comment has been minimized.

Copy link
Contributor Author

@asottile asottile commented Mar 1, 2020

I mean more like only do this on macos and when the config is detected

@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Mar 1, 2020

That would be a hot patch at best not an actual fix 🤷‍♂️I'm looking here for a permanent solution, so people don't open issues about this.

@asottile

This comment has been minimized.

Copy link
Contributor Author

@asottile asottile commented Mar 1, 2020

I'd like to understand the problem further, seems like we might be "fixing" something unfixable

will spend some time this week doing research -- any pointers would be helpful

@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Mar 1, 2020

I don't think it's unfixable. The issue is that global distutils configuration file values can break tools behaviour in virtualenv, mainly pip, setuptools and distutils related ones.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Mar 1, 2020

One could argue that maybe the tools fix the issue, but than we're just throw the blame around, aka should setuptools support virtualenv or virtualenv support setuptools. As the tools generally don't know about virtual environments, especially on python 2, as there's no pep for it, historically virtualenv supported the tools, and this is what I was also proposing above with the pth file.

We're not trying to fix here something that haven't fixed before. Just virtualenv 16 did the fix differently, in a method that's no longer compatible with venv. So now I'm proposing a solution that would do so, and also work on python 2. I agree importing the elements to patch is expensive, hence thinking of using the import hook... But need to play around with it to see it's viability.

@asottile

This comment has been minimized.

Copy link
Contributor Author

@asottile asottile commented Mar 2, 2020

cool, yeah I want to understand the problem more:

  • what was the solution in the 16.x era
  • what (if anything) is being done to fix this in venv
  • does this affect all framework builds or just the ones that ship with brew? (brew also removed python@2 so the "fix" might not be fixing anything any more)
@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Mar 2, 2020

It's not macOS only related. Can happen just as well with any other Python. It just happens that macOS brew always ships a distutils configuration that manifest this issue, while other platforms do not. Virtualenv 16 worked around by shipping it's own distutils copy, I really want to avoid this. This also no longer works on python 3 style venv, where it's no longer possible to override standard library modules due to how the venv is created. I'm not aware upstream trying to fix this yet. but I'd like to have a fix within virtualenv, as we had one before the rewrite.

@gaborbernat

This comment has been minimized.

Copy link
Contributor

@gaborbernat gaborbernat commented Mar 4, 2020

Hello, a fix for this issue has been released via virtualenv 20.0.8; see https://pypi.org/project/virtualenv/20.0.8/ (https://virtualenv.pypa.io/en/latest/changelog.html#v20-0-8-2020-03-04). Please give a try and report back if your issue has not been addressed; if not, please comment here, and we'll reopen the ticket. We want to apologize for the inconvenience this has caused you and say thanks for having patience while we resolve the unexpected bugs with this new major release.
thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

2 participants
You can’t perform that action at this time.