Skip to content

ProgressReporter: align progress bars by largest name length#13266

Merged
konstin merged 5 commits intoastral-sh:mainfrom
eduardorittner:align-prog
May 15, 2025
Merged

ProgressReporter: align progress bars by largest name length#13266
konstin merged 5 commits intoastral-sh:mainfrom
eduardorittner:align-prog

Conversation

@eduardorittner
Copy link
Contributor

Summary

Related to #12492

This change makes all progress bars vertically aligned. This is still a WIP and so is not complete, in the current design I store max_len in BarState and update it on every on_request_start, however this is problematic since order matters, and if the largest name is not sent first, the alignment is not complete. To mitigate this we'd probably have to update all previous bars by "iterating" through the bars field in BarState and update all request bars.

Below is an image of what happens when the largest name (nvidia-cusparselt-cu12) is not the first (in this case, it was the second to last).

2025-05-02T10:56:54-03:00

Test Plan

There are currently no tests, and I'm not sure how to design them since from what I gather the uv_snapshot facilities record the final output, not the intermediate stages.

@zanieb zanieb added the cli Related to the command line interface label May 9, 2025
@zanieb zanieb requested a review from konstin May 9, 2025 14:53
Copy link
Member

@ibraheemdev ibraheemdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just left a couple of nits. I think you're right that we need to update previous bars to keep things aligned, but that shouldn't be too hard.

It might be a little odd if the padding changes for all bars dynamically, maybe we should at least increase the default from 10?

state.sizes.insert(position, size.unwrap_or(0));
state.update_max_len(name.len());

let progress = multi_progress.insert(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right here is where you should be able to iterate through the multi_progress and realign everything.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I'm having a hard time with, indicatif's MultiProgress only has an insert method, with no way of iterating or getting/setting specific values inside it. Which means I should probably be iterating through a field in BarState? But I'm not sure which one, since IDs are only stored in maps, not vectors. Maybe I could figure out from BarState.headers and BarState.id, how many headers are downloads (BarState.id - BarState.headers?) and then iterate from BarState.id - BarState.headers to BarState.id accessing the relevant bar in BarState.bars.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did the dumb thing and iterated through 0..BarState.id and it seems to work, I'm not sure if there's a situation where it breaks or does something unexpected.

@konstin
Copy link
Member

konstin commented May 13, 2025

Hi, and sorry for the late review. Could you change the code so that when we have a new longest progress bar, it updates all running progress bars to the new template with the new length (as you suggested), to avoid a pattern like below?

image

@eduardorittner
Copy link
Contributor Author

Thank you for all the comments and suggestions! I'm having a busy month with school and work but will work on this when I have time 😁

@eduardorittner
Copy link
Contributor Author

I mentioned this in the PR description, but do you have any ideas on how to test this feature? Since we care about the intermediate state while downloads are happening (dynamically adjusting bar length) not only the final one, should we have several snapshots of the output? How can we simulate downloads without actually hitting the network?

@eduardorittner
Copy link
Contributor Author

It might be a little odd if the padding changes for all bars dynamically, maybe we should at least increase the default from 10?

Changed it to 20, let me know what you think!

@eduardorittner eduardorittner force-pushed the align-prog branch 2 times, most recently from f737465 to db56d61 Compare May 14, 2025 20:28
@eduardorittner
Copy link
Contributor Author

Another question I have is if I should write similar logic in on_build_start? If so, any tips on what/how to build with uv? I've only ever downloaded packages with it so I'm unsure

@eduardorittner eduardorittner marked this pull request as ready for review May 14, 2025 20:29
@eduardorittner
Copy link
Contributor Author

Hi, and sorry for the late review. Could you change the code so that when we have a new longest progress bar, it updates all running progress bars to the new template with the new length (as you suggested), to avoid a pattern like below?

image

Also which package are you downloading here? I wanted to test this with a lot of packages but torch is only pulling like 10 dependencies

@konstin
Copy link
Member

konstin commented May 15, 2025

I mentioned this in the PR description, but do you have any ideas on how to test this feature? Since we care about the intermediate state while downloads are happening (dynamically adjusting bar length) not only the final one, should we have several snapshots of the output? How can we simulate downloads without actually hitting the network?

We test this manually part manually, we don't have snapshots for non-static text.

Another question I have is if I should write similar logic in on_build_start? If so, any tips on what/how to build with uv? I've only ever downloaded packages with it so I'm unsure

The builds use a spinner instead of a progress bar (we can't tell how long they will take), so this is only relevant for on_*_progress bars.

Also which package are you downloading here? I wanted to test this with a lot of packages but torch is only pulling like 10 dependencies

I'm using

uv venv -p 3.11 && cargo run pip install -r scripts/requirements/transformers-extras.in

sometimes with --no-cache.

@konstin
Copy link
Member

konstin commented May 15, 2025

I've updated the code to use an enum to only apply this logic to numeric progress while skipping progress spinners, and I've rebased for the merge conflict with main.

Screencast.from.2025-05-15.09-11-13.webm

@konstin konstin merged commit 2d4d93a into astral-sh:main May 15, 2025
86 checks passed
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 18, 2025
## 0.7.5

### Bug fixes

- Support case-sensitive module discovery in the build backend ([#13468](astral-sh/uv#13468))
- Bump Simple cache bucket to v16 ([#13498](astral-sh/uv#13498))
- Don't error when the script is too short for the buffer ([#13488](astral-sh/uv#13488))
- Add missing word in "script not supported" error ([#13483](astral-sh/uv#13483))

## 0.7.4

### Enhancements

- Add more context to external errors ([#13351](astral-sh/uv#13351))
- Align indentation of long arguments ([#13394](astral-sh/uv#13394))
- Preserve order of dependencies which are sorted naively ([#13334](astral-sh/uv#13334))
- Align progress bars by largest name length ([#13266](astral-sh/uv#13266))
- Reinstall local packages in `uv add` ([#13462](astral-sh/uv#13462))
- Rename `--raw-sources` to `--raw` ([#13348](astral-sh/uv#13348))
- Show 'Downgraded' when `self update` is used to install an older version ([#13340](astral-sh/uv#13340))
- Suggest `uv self update` if required uv version is newer ([#13305](astral-sh/uv#13305))
- Add 3.14 beta images to uv Docker images ([#13390](astral-sh/uv#13390))
- Add comma after "i.e." in Conda environment error ([#13423](astral-sh/uv#13423))
- Be more precise in unpinned packages warning ([#13426](astral-sh/uv#13426))
- Fix detection of sorted dependencies when include-group is used ([#13354](astral-sh/uv#13354))
- Fix display of HTTP responses in trace logs for retry of errors ([#13339](astral-sh/uv#13339))
- Log skip reasons during Python installation key interpreter match checks ([#13472](astral-sh/uv#13472))
- Redact credentials when displaying URLs ([#13333](astral-sh/uv#13333))

### Bug fixes

- Avoid erroring on `pylock.toml` dependency entries ([#13384](astral-sh/uv#13384))
- Avoid panics for cannot-be-a-base URLs ([#13406](astral-sh/uv#13406))
- Ensure cached realm credentials are applied if no password is found for index URL ([#13463](astral-sh/uv#13463))
- Fix `.tgz` parsing to respect true extension ([#13382](astral-sh/uv#13382))
- Fix double self-dependency ([#13366](astral-sh/uv#13366))
- Reject `pylock.toml` in `uv add -r` ([#13421](astral-sh/uv#13421))
- Retain dot-separated wheel tags during cache prune ([#13379](astral-sh/uv#13379))
- Retain trailing comments after PEP 723 metadata block ([#13460](astral-sh/uv#13460))

### Documentation

- Use "export" instead of "install" in `uv export` arguments ([#13430](astral-sh/uv#13430))
- Remove extra newline ([#13461](astral-sh/uv#13461))

### Preview features

- Build backend: Normalize glob paths ([#13465](astral-sh/uv#13465))
tmeijn pushed a commit to tmeijn/dotfiles that referenced this pull request May 19, 2025
This MR contains the following updates:

| Package | Update | Change |
|---|---|---|
| [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.7.3` -> `0.7.5` |

MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot).

**Proposed changes to behavior should be submitted there as MRs.**

---

### Release Notes

<details>
<summary>astral-sh/uv (astral-sh/uv)</summary>

### [`v0.7.5`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#075)

[Compare Source](astral-sh/uv@0.7.4...0.7.5)

##### Bug fixes

-   Support case-sensitive module discovery in the build backend ([#&#8203;13468](astral-sh/uv#13468))
-   Bump Simple cache bucket to v16 ([#&#8203;13498](astral-sh/uv#13498))
-   Don't error when the script is too short for the buffer ([#&#8203;13488](astral-sh/uv#13488))
-   Add missing word in "script not supported" error ([#&#8203;13483](astral-sh/uv#13483))

### [`v0.7.4`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#074)

[Compare Source](astral-sh/uv@0.7.3...0.7.4)

##### Enhancements

-   Add more context to external errors ([#&#8203;13351](astral-sh/uv#13351))
-   Align indentation of long arguments ([#&#8203;13394](astral-sh/uv#13394))
-   Preserve order of dependencies which are sorted naively ([#&#8203;13334](astral-sh/uv#13334))
-   Align progress bars by largest name length ([#&#8203;13266](astral-sh/uv#13266))
-   Reinstall local packages in `uv add` ([#&#8203;13462](astral-sh/uv#13462))
-   Rename `--raw-sources` to `--raw` ([#&#8203;13348](astral-sh/uv#13348))
-   Show 'Downgraded' when `self update` is used to install an older version ([#&#8203;13340](astral-sh/uv#13340))
-   Suggest `uv self update` if required uv version is newer ([#&#8203;13305](astral-sh/uv#13305))
-   Add 3.14 beta images to uv Docker images ([#&#8203;13390](astral-sh/uv#13390))
-   Add comma after "i.e." in Conda environment error ([#&#8203;13423](astral-sh/uv#13423))
-   Be more precise in unpinned packages warning ([#&#8203;13426](astral-sh/uv#13426))
-   Fix detection of sorted dependencies when include-group is used ([#&#8203;13354](astral-sh/uv#13354))
-   Fix display of HTTP responses in trace logs for retry of errors ([#&#8203;13339](astral-sh/uv#13339))
-   Log skip reasons during Python installation key interpreter match checks ([#&#8203;13472](astral-sh/uv#13472))
-   Redact credentials when displaying URLs ([#&#8203;13333](astral-sh/uv#13333))

##### Bug fixes

-   Avoid erroring on `pylock.toml` dependency entries ([#&#8203;13384](astral-sh/uv#13384))
-   Avoid panics for cannot-be-a-base URLs ([#&#8203;13406](astral-sh/uv#13406))
-   Ensure cached realm credentials are applied if no password is found for index URL ([#&#8203;13463](astral-sh/uv#13463))
-   Fix `.tgz` parsing to respect true extension ([#&#8203;13382](astral-sh/uv#13382))
-   Fix double self-dependency ([#&#8203;13366](astral-sh/uv#13366))
-   Reject `pylock.toml` in `uv add -r` ([#&#8203;13421](astral-sh/uv#13421))
-   Retain dot-separated wheel tags during cache prune ([#&#8203;13379](astral-sh/uv#13379))
-   Retain trailing comments after PEP 723 metadata block ([#&#8203;13460](astral-sh/uv#13460))

##### Documentation

-   Use "export" instead of "install" in `uv export` arguments ([#&#8203;13430](astral-sh/uv#13430))
-   Remove extra newline ([#&#8203;13461](astral-sh/uv#13461))

##### Preview features

-   Build backend: Normalize glob paths ([#&#8203;13465](astral-sh/uv#13465))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this MR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box

---

This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0MC4xMS4xOSIsInVwZGF0ZWRJblZlciI6IjQwLjEzLjAiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbIlJlbm92YXRlIEJvdCJdfQ==-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cli Related to the command line interface

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants