-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
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. |
Yea, and it's much worse without the trimming. From #10423's first post:
|
With this PR:
It's not as bad as the uncompressed numbers look. |
Do we care strongly about the size increase? |
I do care, not strongly but it does matter to me (less about the pure size, more about the incremental extra time involved in 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... |
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 |
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 This is a separate thing, though, so I'll leave it to a separate PR if I decide to do anything about it. |
Hey, zipimport is a thing. :) |
Exactly! (I was there when it was invented 🙂) |
@pradyunsg @pfmoore consider just storing wheels in the vendor folder then |
Wheels are not meant to be used as importable artifacts. Plus, not all of our dependencies have wheels on PyPI. |
@pfmoore Assuming I pursue dropping chardet along with this (albeit, in a separate PR), would you be fine with this? |
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...) |
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!
|
Oh, and here's how it looks for me on a modern terminal: (spinner) output.mp4(progress bar) output2.mp4 |
One thing at a time, but we'll get there soon-enough, I hope. :) |
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 Looking forward to seeing this land. |
Whelp, rich depends on |
This was wrong. Turns out, it's a single Python file. 🤷🏽 |
Oh, you gotta be kidding me. They literally enable LongPath on all Windows VMs since, like "initial commit". |
It's only 248 characters as well. That's close, but not "too long" by my reckoning... |
Tried out long paths on GitHub Action's Python installations and... AHAAHAAHAH. Will be "fixed" by #10653, by dropping 3.9 testing on Windows -- in the spirit of testing on boundary versions for Windows. |
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. |
Alright, filed actions/setup-python#267. And... #10653 is green. |
Alright, so now I know that it's 3.9 specific in our CI runs too. |
Rebased, and this should work? |
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.
Rebased, with the following commit squashed into the first one here -- 5739d1c -- and an upgrade to rich for better file:// URL presentation. |
Let's do this! |
Toward #10461
rich
and dependencies.pygments
.commonmark
andrich.markdown
.