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 a --use-zipapp option to the test suite #11250

Merged
merged 11 commits into from
Aug 1, 2022
Merged

Conversation

pfmoore
Copy link
Member

@pfmoore pfmoore commented Jul 11, 2022

This adds a new --use-zipapp option to the test suite, which builds pip as a zipapp and uses that when running the integration tests.

The implementation is complete, but I'm leaving this as draft for now as not all of the tests support being run with a zipapp yet.

@uranusjr
Copy link
Member

Wow that slow down the test suite a lot.

@pfmoore
Copy link
Member Author

pfmoore commented Jul 12, 2022

Hmm, it did, didn't it? I mainly wanted to add it to CI to test on a "proper" Linux box, as I was getting a single test failure locally on WSL, which I think is due to it being WSL (test_check_submodule_addition, skipped on Windows, with a comment "TODO(pnasrat) fix all helpers to do right things with paths on windows"). Testing on Windows takes ages, and has a bunch of failures that I don't want to get sucked into now.

It's possible that I've got the zipapp fixture wrong, and it's building the zipapp multiple times. I've no idea how to check that or how to fix it if it is the case. I'd really appreciate someone who understands pytest and our test suite taking a look at that code.

It's also possible that running from a zipapp is just really slow - the zipapp doesn't contain .pyc files, so there's possibly a big chunk of slowdown from that. I'm not sure how we'd address that, if it is the case 🙁

@pfmoore
Copy link
Member Author

pfmoore commented Jul 12, 2022

I'm going to take the "draft" status off this PR, as all the tests are passing so the zipapp approach seems pretty solid. I'm happy to remove zipapp from CI, if the performance is too much of a problem (I'm inclined to think that it is) and it can't be fixed.

I did some checks running various versions of pip, using hyperfine. The basic conclusions were:

  1. Including .pyc files in the zipapp makes little or no difference.
  2. Running the zipapp is about half as fast as running an installed pip (one second vs half a second for pip --version on my PC).
  3. Compressing the zipapp doesn't help.
  4. Running the unpacked zipapp is as fast as running installed pip.
  5. The initial run of the unpacked zipapp is slow, because it's compiling and writing .pyc files.
  6. Running the unpacked zipapp (with no .pyc files) with -B (don't write .pyc files) is in the middle (0.75s).

I'm inclined to think that the overall implication here is that the slowness is down to Python's "zipimport" mechanism, and not something we can address in pip. Which is disappointing, as it may have implications for zipapp as a distribution mechanism for pip. I still think it's a viable approach, and if we add it initially as "experimental", we can get some feedback from users as to whether the performance hit is a problem in real world situations - I suspect that normally, pip's startup time is swamped by things like downloading, builds, etc.

@pfmoore pfmoore marked this pull request as ready for review July 12, 2022 11:48
@uranusjr
Copy link
Member

Hmm, from the latest test run, the duration difference is actually not that much (zipapp took 29m, the second slowest was 26m); maybe it’d a good enough if we simply split that job into two, like we do for tests on Windows.

@pfmoore
Copy link
Member Author

pfmoore commented Jul 28, 2022

Merging with main to test the new __pip-runner__.py approach against a zipapp. I suspect that the dirname(dirname(__file__)) call in that file might fail in that situation.

cc @pradyunsg

@pfmoore
Copy link
Member Author

pfmoore commented Jul 28, 2022

@pfmoore
Copy link
Member Author

pfmoore commented Aug 1, 2022

I’ll leave splitting the ci run into two for —with-zipapp for a follow up PR if anyone thinks it’s worth doing.

@pfmoore pfmoore merged commit f47a204 into pypa:main Aug 1, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 17, 2022
@pfmoore pfmoore deleted the test_zipapp branch April 26, 2023 11:10
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.

2 participants