perf: Use Rich Status for batched spinner rendering instead of per-line updates#14026
perf: Use Rich Status for batched spinner rendering instead of per-line updates#14026KRRT7 wants to merge 41 commits into
Conversation
Move spinners.py and progress_bars.py functionality into a new unified ui.py module. This consolidates UI concerns and prepares for switching from manual spinner management to Rich's Status context manager for proper buffering and rate-limited rendering.
Add _detect_no_color() function to check --no-color flag and environment variables. In setup_logging(), detect if stderr is a TTY and auto-disable colors when writing to non-TTY streams (like in tests). Set force_terminal only when stderr is actually a TTY, preventing ANSI codes from appearing in test output while maintaining proper terminal rendering in interactive use.
Spinners are now managed through Rich's Status context manager rather than being passed to call_subprocess. Remove the spinner parameter and related code that updated the spinner on each line. This eliminates per-line spinner updates and lets Rich's Status handle rendering with proper batching and rate limiting, addressing the syscall and terminal write overhead of the old implementation.
Replace imports of spinners.py and progress_bars.py with ui module. Update build_env.py and subprocess.py to use status() context manager instead of open_spinner/open_rich_spinner. Add logger.info() call to status() function to ensure status messages (like 'Installing build dependencies') appear in output through the logging system rather than transient spinner display.
Remove the old spinners.py and progress_bars.py files now that their functionality has been consolidated into ui.py. Update test imports to use the new ui module instead of spinners module.
|
Hi @KRRT7, thank you for your contribution. Please let me know if you need any help fixing the failing tests. Also, please note that pip is a volunteer-driven project and reviewer capacity is limited, so it may take some time before a maintainer can take a look at your PR. |
|
Hey @sepehr-rs I'll open an issue shortly, though from what I saw, there's a few open PRs I could just link to that this closes. |
|
closes #14028 |
Great! Please update your news file to reflect the issue number you're closing. (e.g. |
|
@KRRT7 The news entry filename should be |
sepehr-rs
left a comment
There was a problem hiding this comment.
Hi, I benchmarked the spinner path under a TTY using runner_with_spinner_message() and a subprocess producing output for ~5 seconds.
In my measurements:
- main: 434 stdout writes, ~2 KB terminal output
- PR: 671 stdout writes, ~20 KB terminal output
- wall-clock time was effectively unchanged
Based on these results, I wasn't able to reproduce the performance improvements described in the PR description. Could you share the benchmark used to support those claims, or clarify whether they refer to a different code path (for example download progress bars rather than subprocess spinners)?
Separately, the PR appears to include a number of changes that don't seem directly related to the spinner/status refactor. Please consider reducing the scope or splitting out unrelated changes, as that would make the review process much easier.
|
those numbers don't make sense to me, what do your benchmarks look like? I'll share mine in a bit |
Sure. I benchmarked the I compared main and your PR using: strace -f -e write -o main.trace python benchmark.py
strace -f -e write -o pr.trace python benchmark.pyand then: grep 'write(1' main.trace | wc -l
grep 'write(1' pr.trace | wc -l
grep 'write(1' main.trace | awk -F',' '{print $3}' | awk '{sum += $1} END {print sum}'
grep 'write(1' pr.trace | awk -F',' '{print $3}' | awk '{sum += $1} END {print sum}'Results Results (after a re-run with a ~2.5s workload):
Wall-clock time was essentially identical (~2.7s). So for this workload I wasn't able to reproduce the performance improvements described in the PR. It's possible we're measuring different code paths, though, so I'd be interested in seeing the benchmark you used and comparing methodologies. For reference, here's the benchmark script I used: |
|
that'd be why: you're not running it for a long enough time where it gets worse |
|
the workloads is closer to a longer-running install path where subprocess output stays active for much longer, and that's important bc he cost I’m trying to reduce is cumulative, The benchmarks are useful for isolating progress-rendering behavior, which is where the issue surfaces, rich internally has a more efficient way of doing it. |
|
Thanks for sharing the benchmark suite. I was able to run it and I do see improvements in the progress-bar benchmarks (~20% in the download-related cases). My earlier measurements were focused on the One question I still have is about the "long-running cumulative cost" explanation. The benchmark workload itself is relatively small (20 KB / 20 chunks per run), so it's not obvious to me how closely it reflects the longer-running install scenarios described in the PR discussion. Do you have additional benchmarks covering those cases? Overall, given that the PR description discusses both subprocess output handling and progress rendering, it may be helpful to distinguish which performance claims apply to which code paths. |
yes that's on purpose, it's not necessary to have a big benchmark suite to show what I'm trying to show, this, imo, is enough to make my point, plus the numbers there, including iterations and Kbs are configurable. |
# Conflicts: # src/pip/_internal/build_env/installer.py
# Conflicts: # src/pip/_internal/build_env/installer.py
|
Before this PR, subprocess execution and spinner rendering were coupled. On The spinner code itself lived in After this PR, The new Why this matters: |
|
@willmcgugan for your review! |
|
If I've understood correctly. Pip has written a custom wrapper for the progress bars, rather than using using Rich's builtin manager. And this bypasses the logic that Rich's version does to minimize updates. For context, Rich limits display the progress bar to N updates a second. Rather than rendering on every update. It can be a big win depending on the granularity of writes. @KRRT7 I think you've spotted a real issue. But I suspect the pip folks are hesitant because they would be exchanging known code that works, with unknown code that may come with its own issues. They have to be very conservative given how pip is essential infrastructure. Can't speak for the core devs, but maybe break this PR into smaller more digestible pieces. And possibly add a benchmark tool. This kind of thing can vary massively depending on software and hardware combinations. |
|
Thanks, this is helpful. Yes, that’s the issue I was trying to address: pip’s custom spinner/wrapper code meant we weren’t really letting Rich handle update throttling/rendering itself. As for the PR itself, I’ve now reduced it to a much smaller, API-preserving version that keeps the existing module boundaries and subprocess interfaces, and just swaps the spinner implementation underneath while preserving the legacy non-interactive output and tests. For now I’ve taken that route in the hope that it makes the change easier to review and merge. As for a benchmark tool, it’s something I’ve been considering as well. pip’s project/test setup makes it a bit awkward to turn into a proper reusable benchmark harness, but I’d still like to look into it when I have a moment. |
|
What I still don't understand how our pre-existing Rich based spinner is slow: class _PipRichSpinner:
"""
Custom rich spinner that matches the style of the legacy spinners.
(*) Updates will be handled in a background thread by a rich live panel
which will call render() automatically at the appropriate time.
"""
def __init__(self, label: str) -> None:
self.label = label
self._spin_cycle = itertools.cycle(SPINNER_CHARS)
self._spinner_text = ""
self._finished = False
self._indent = get_indentation() * " "
def __rich_console__(
self, console: Console, options: ConsoleOptions
) -> RenderResult:
yield self.render()
def __rich_measure__(
self, console: Console, options: ConsoleOptions
) -> Measurement:
text = self.render()
return Measurement.get(console, options, text)
def render(self) -> RenderableType:
if not self._finished:
self._spinner_text = next(self._spin_cycle)
return Text.assemble(self._indent, self.label, " ... ", self._spinner_text)
def finish(self, status: str) -> None:
"""Stop spinning and set a final status message."""
self._spinner_text = status
self._finished = True
@contextlib.contextmanager
def open_rich_spinner(label: str, console: Console | None = None) -> Generator[None]:
if not logger.isEnabledFor(logging.INFO):
# Don't show spinner if --quiet is given.
yield
return
console = console or get_console()
spinner = _PipRichSpinner(label)
with Live(spinner, refresh_per_second=SPINS_PER_SECOND, console=console):
try:
yield
except KeyboardInterrupt:
spinner.finish("canceled")
raise
except Exception:
spinner.finish("error")
raise
else:
spinner.finish("done")This explicitly defers the
I'll admit that I don't understand what exactly optimizations rich's own spinner is doing, but the refresh rate ought to be the most important part, no? Sure, there may be a little bit more optimization you can achieve if you also know what characters you're writing, but how significant is that? You're correct to point out that we don't use this spinner everywhere, but that's just a matter of we haven't gotten around to it. I don't see the need to throw out what already works wholesale. |
|
I added an ASV benchmark for the actual It benchmarks the interactive spinner hot loop directly, with a virtual clock so it measures spinner overhead rather than sleep time. Running: uv run --group benchmark python -m asv run --bench spinner "main^!"
uv run --group benchmark python -m asv run --bench spinner "HEAD^!"
uv run --group benchmark python -m asv compare main HEADgives:
and for the larger case:
So this shows the scaling issue more directly: on |
|
This PR introduces a behavior change that the tests don't identify: on an interactive tty the new spinner erases its own line when it finishes, since Status is a transient Live. So the "Preparing metadata ... done" and "Building wheel for X ... done" lines that main leaves in the scrollback are gone after the build. Repro under a pty, run once against main and once against this PR: erase_repro.py (PEP 723;
|
in
pip/_internal/utils/subprocess.pypip currently reads the child process’s stdout one line at a time (proc.stdout.readline() inside a while True loop).Every call to readline() triggers a system call and a Python‑level buffer check. When the subprocess produces many small lines (e.g., downloading a large wheel), the loop runs thousands of times
The cumulative overhead of those syscalls and Python function calls dominates the wall‑clock time, even though the actual work (downloading files) is I/O‑bound.
The legacy open_spinner / InteractiveSpinner implementation writes raw back‑space characters to the terminal and rewrites the whole line for every spinner tick.
When showing_subprocess is false (the usual case for quiet installs), the spinner updates on every line of subprocess output, not on a timed interval. That means dozens or hundreds of terminal writes per second, each of which forces the terminal emulator to repaint the line. The repeated cursor‑hiding/showing logic (hidden_cursor) adds extra writes.
The original code used a custom _PipRichSpinner that wrapped Rich’s Live panel. It still performed the low‑level terminal writes itself rather than letting Rich’s own rendering engine batch updates. This prevented Rich from applying its own optimisations (e.g., double‑buffered drawing, minimal refresh rates).