Concurrent progress bars

[ibraheemdev]

Summary

Implements concurrent progress bars. Resolves #1209.

Test Plan

progress-bars.mp4

[T-256]

could it be sorted (the largest at top)?

[ibraheemdev]

@T-256 I put the largest at the bottom which looks more natural, in my opinion.

progress-bars.mp4

[charliermarsh]

The code generally looks good (well done) but need to find a bit of time to play with this myself to see if I have any feedback on the UX.

[ibraheemdev]

Just tested this on a large requirements file, I think any downloads <250kb shouldn't be displayed. The output changes too fast for it to be useful.

[codspeed-hq]

CodSpeed Performance Report

Merging #3252 will not alter performance

_{Comparing ibraheemdev:concurrent-progress-bar (0ab84fe) with main (080f095)}

Summary

✅ 13 untouched benchmarks

[ibraheemdev]

We should also probably provide a way to disable this output.

[charliermarsh]

We could consider making it opt-in. (That would also make it less risky to ship.)

[zanieb]

What's the risk, specifically? It seems improbable that this will break people's workflows right?

[zanieb]

We do have a preview option now though, if you really want to gate it. A dedicated setting might make sense too? but opt-in seems wrong for that.

[charliermarsh]

I think the risk is just that it’s overly verbose for most package installs and may require tuning. I would be fine shipping it under preview so we can get feedback on the UI/UX. I haven’t had a chance to play with it myself yet though.

[zanieb]

I don't mind iterating on display ux without opt-in, it feels much safer than other changes we make and we release very often.

[zanieb]

Is this waiting on any feedback?

[charliermarsh]

Yes, from me at least - not sure if others are planning to review.

[zanieb]

I'd love to get this merged, I don't think it's great to have it hanging for weeks. Anything I can do to move this forward?

[charliermarsh]

Ultimately it's on me to review. However you could test and give feedback on the UX -- it would make it higher-confidence for me if we could get more eyes and opinions on it.

[zanieb]

I did test it and was happy with it but I can do more investigation.

@ibraheemdev can you rebase the branch?

[charliermarsh]

I will review and merge today.

[ibraheemdev]

I'll rebase. I am a little concerned about the output when you have a lot of small downloads though, e.g. #2706 (comment). Even kitty was flickering a little. We can probably buffer updates to the terminal a bit.

[zanieb]

If anyone has "good" test cases post 'em here so we have some common ground when considering the behavior.

[T-256]

LGTM and as Charlie said, it could be opt-in via --preview and after stabilization make it default behavior. Also, as Zanie said we can have an option to opt-out from it.

We can probably buffer updates to the terminal a bit.

Let's get some feedback from users, then we can set this interval value with more confidence.

[charliermarsh]

Should we just move behind preview to unblock the decision? @ibraheemdev do you want it to ship to stable?

[ibraheemdev]

I'm fine with merging this behind preview or an opt-in flag, as long as it's accessible because it can be useful for debugging.

[zanieb]

I have a medium preference for shipping it in stable with an opt-out, but that requires a separate mechanism than --preview 🤷‍♀️

[charliermarsh]

Let's just move it to --preview to get it unblocked I think.

[charliermarsh]

I like how pip's progress bars are left-aligned and have a default width -- as-is the display is too wide IMO on my monitor:

How hard is it to achieve something like that?

[charliermarsh]

To achieve that, I guess we'd need to put the package name above the bar, or to the right of it, to achieve consistent alignment?

[charliermarsh]

Or use a fixed size for the package name. I can play with it.

[charliermarsh]

Here's a slightly different layout. The colors are washed out in the video but it's green on dim:

May-23-2024.23-28-19.mp4

[charliermarsh]

Thoughts?

[charliermarsh]

I feel like I'm so allergic to unwrap that I'd probably do something like:

let progress = self
    .reporter
    .as_ref()
    .map(|reporter| (reporter, reporter.on_download_start(dist.name(), size)));

And then use match self.progress everywhere. What's your instinct?

[ibraheemdev]

That is nicer, updated.

[ibraheemdev]

@charliermarsh your design looks quite nice, I would be happy to switch to that.

[charliermarsh]

Sounds good, I'll merge this today. I looked at gating behind preview but honestly looks like kind of a pain haha.

[charliermarsh]

We can add a --no-progress flag?

[itayporezky]

After seeing this in the release notes i was afraid it would cause this
But in my testing it didn't, so that's great :)