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

Vendor rich, use it for progress bars and deprecate alternative progress bar styles #10462

Merged
merged 5 commits into from
Nov 26, 2021

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Sep 11, 2021

Toward #10461

  • Vendor rich and dependencies.
    • trim pygments.
    • drop commonmark and rich.markdown.
  • Plumb it all up, with our current progress bar setup.

@uranusjr
Copy link
Member

uranusjr commented Sep 12, 2021

Wow, that’s 31k lines added, i.e. pip would be about 20% larger after this…

Back of a napkin calculation: 31k * 25 characters per line = 775k. pip is currently 1.6m.

@pradyunsg
Copy link
Member Author

Yea, and it's much worse without the trimming. From #10423's first post:

-rw-r--r--  1 pradyunsg  staff   2.6M  3 Sep 10:44 pip-21.3.dev0+rich.tar.gz
-rw-r--r--  1 pradyunsg  staff   1.5M  3 Sep 10:44 pip-21.3.dev0.tar.gz

@pradyunsg
Copy link
Member Author

With this PR:

-rw-r--r--  1 pradyunsg  staff   1.8M 12 Sep 19:58 pip-21.3.dev0.tar.gz

It's not as bad as the uncompressed numbers look.

@github-actions github-actions bot added the needs rebase or merge PR has conflicts with current master label Oct 5, 2021
@pradyunsg
Copy link
Member Author

Do we care strongly about the size increase?

@pfmoore
Copy link
Member

pfmoore commented Oct 22, 2021

I do care, not strongly but it does matter to me (less about the pure size, more about the incremental extra time involved in py -m venv xxx vs py -m venv --without-pip xxx, where it's copy and unpack time). I don't have time right now to check how much of a difference the extra size makes there. I'll try to do some tests over the weekend.

OTOH, I'm not sure what we do about it, having to vendor everything is just how pip is, and I don't want to start blocking functionality because of code size...

@pradyunsg
Copy link
Member Author

pradyunsg commented Oct 22, 2021

The main reason I ask is because rich makes it really easy to do what I'm trying to do with our error messages, and I'd really like to not redo the work done by the library for transparently degrading to non-color output.

I do think your concerns are well placed though -- since we also zip up pip for isolated build environments.

IIUC, the biggest dependency for us right now is chardet, and with #10291, it should be possible to drop that. The gains there are roughly the exact same as the costs here, so it'll all balance out in the end.

@pfmoore
Copy link
Member

pfmoore commented Oct 22, 2021

Hmm, I wonder whether we could do something fancy like ship (some or all of) our vendored dependencies as a zipfile, which we add to sys.path. Some might not be zip-safe (the certificate file, for example) but get-pip demonstrates that most are, and shipping one compressed zipfile of our vendored code rather than a bunch of files might be a win...

This is a separate thing, though, so I'll leave it to a separate PR if I decide to do anything about it.

@pradyunsg
Copy link
Member Author

Hey, zipimport is a thing. :)

@pfmoore
Copy link
Member

pfmoore commented Oct 22, 2021

Hey, zipimport is a thing

Exactly! (I was there when it was invented 🙂)

@RonnyPfannschmidt
Copy link
Contributor

@pradyunsg @pfmoore consider just storing wheels in the vendor folder then

@pradyunsg
Copy link
Member Author

pradyunsg commented Oct 22, 2021

Wheels are not meant to be used as importable artifacts. Plus, not all of our dependencies have wheels on PyPI.

@pradyunsg
Copy link
Member Author

@pfmoore Assuming I pursue dropping chardet along with this (albeit, in a separate PR), would you be fine with this?

@pfmoore
Copy link
Member

pfmoore commented Nov 3, 2021

Yes, I would. I'd like to see rich included, so as long as we're actively working on containing the problem of bloat caused by our vendoring, that works for me.

(Semi-related, maybe we could ask rich to consider making pygments and commonmark optional dependencies, although that probably means somebody needs to move forward with the "default extra" proposal that's been mentioned on Discourse, because those features should probably remain included by default for rich...)

@pradyunsg pradyunsg changed the title Vendor rich and use it for logging output Vendor rich and use it for progress bars Nov 6, 2021
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Nov 6, 2021
@pradyunsg pradyunsg changed the title Vendor rich and use it for progress bars Vendor rich, use it for progress bars and deprecate alternative progress bar styles Nov 6, 2021
@pradyunsg pradyunsg marked this pull request as ready for review November 6, 2021 09:11
@pradyunsg
Copy link
Member Author

Alright. Assuming the CI passes, this is good to review.

This will be easiest to review commit-by-commit. The main things to look at in the first commit will be 2-4 files at the top of the page and the bottom of the page (rest are the vendored sources).

This PR does a few things that are a bit weird-ish that I wanna check that no one is opposed to. Oh, and pinging @willmcgugan to make sure he's aware of these and OK with this as well!

  • This PR removes rich.markdown, and does not commonmark in pip's vendored packages.
    • commonmark seems to be unnecesssarily bloated at the moment
    • I don't expect us to to rich.markdown now.
    • I'll work with RTD folks to get commonmark in a better spot, when I am able to make time for it.
  • This PR removes all the lexers from pygments, other than what rich directly references (python and cython -- pyrex is an alias to cython).
    • pygments is... big. There's no reason to bundle so much not-gonna-be-used code. :)

@pradyunsg
Copy link
Member Author

Oh, and here's how it looks for me on a modern terminal:

(spinner)

output.mp4

(progress bar)

output2.mp4

@pradyunsg
Copy link
Member Author

One thing at a time, but we'll get there soon-enough, I hope. :)

@willmcgugan
Copy link

Marvellous! @pradyunsg Your approach to vendoring seems sensible.

I would like to offer a lean Rich. I'd be interested in the "default extra" proposal @pfmoore mentioned. I'm imagining something like pip install rich[-pygments]?

Looking forward to seeing this land.

@pradyunsg
Copy link
Member Author

pradyunsg commented Nov 6, 2021

Whelp, rich depends on typing_extensions, and we can't vendor/depend on that.

@pradyunsg
Copy link
Member Author

and we can't vendor/depend on that.

This was wrong. Turns out, it's a single Python file. 🤷🏽

@pradyunsg
Copy link
Member Author

pradyunsg commented Nov 12, 2021

ERROR: Could not install packages due to an OSError: [WinError 206] The filename or extension is too long: 'R:\Temp\pytest-of-unknown\pytest-0\popen-gw0\test_install_no_binary_builds_0\workspace\tmp\pip-install-haks2iva\setuptools_070f768d9950487592ca5a23c8ebc5f1\pkg_resources/tests/data/my-test-package_unpacked-egg/my_test_package-1.0-py3.7.egg/EGG-INFO'

Oh, you gotta be kidding me.

https://github.com/actions/virtual-environments/blob/a64db791f32ffe355d7397c1ac896d42686e9dfa/images/win/scripts/Installers/Initialize-VM.ps1#L70

They literally enable LongPath on all Windows VMs since, like "initial commit".

@pfmoore
Copy link
Member

pfmoore commented Nov 12, 2021

It's only 248 characters as well. That's close, but not "too long" by my reckoning...

@pradyunsg
Copy link
Member Author

Tried out long paths on GitHub Action's Python installations and...

Screenshot 2021-11-12 at 09 16 13

AHAAHAAHAH. Will be "fixed" by #10653, by dropping 3.9 testing on Windows -- in the spirit of testing on boundary versions for Windows.

@pradyunsg
Copy link
Member Author

So... I can't quite figure out what's wrong here -- but something is clearly off. I'll file an issue with the GitHub folks, and in the meantime, we should probably drop 3.9 from the Windows tests anyway.

@pradyunsg
Copy link
Member Author

Alright, filed actions/setup-python#267. And... #10653 is green.

@pradyunsg
Copy link
Member Author

Alright, so now I know that it's 3.9 specific in our CI runs too.

@pradyunsg
Copy link
Member Author

Rebased, and this should work?

@pradyunsg
Copy link
Member Author

Ooops, soo... I've trimmed the lexer mapping from pygments as well, and that needs to be fixed before we merge this. :-)

This will enable building upon these libraries, to improve the
presentation style and output.
Utilise rich's progress bar to present download progress. This has a
subjectively nicer presentation style and should degrade gracefully
without additional effort from our end.
Maintaining these is not particularly useful, since the progress bar
provided by rich is capable of degrading gracefully to ASCII and
alternative styles provided today aren't particularly usable either.
This includes handling of file:// URLs, which is useful for our output.
@pradyunsg
Copy link
Member Author

pradyunsg commented Nov 26, 2021

Rebased, with the following commit squashed into the first one here -- 5739d1c -- and an upgrade to rich for better file:// URL presentation.

@pradyunsg pradyunsg added project: vendored dependency Related to a vendored dependency C: output Related to what pip prints type: enhancement Improvements to functionality labels Nov 26, 2021
@pradyunsg
Copy link
Member Author

Let's do this!

@pradyunsg pradyunsg merged commit a800ae2 into pypa:main Nov 26, 2021
@pradyunsg pradyunsg deleted the rich branch November 26, 2021 14:21
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
C: output Related to what pip prints project: vendored dependency Related to a vendored dependency type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants