Skip to content

Add rich based spinner #13451

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

Merged
merged 2 commits into from
Jul 17, 2025
Merged

Add rich based spinner #13451

merged 2 commits into from
Jul 17, 2025

Conversation

ichard26
Copy link
Member

@ichard26 ichard26 commented Jun 27, 2025

Broken out of #13450. Our legacy spinners can't be used for that PR because they require that their spin() method is called manually (and thus are only really functional with the subprocess utility). Instead, let rich handle the updates in the background for us.

Can be tested manually like so:

import time
import logging.config  # this is a bug with pip's logging stack
from pip._internal.cli.spinners import open_rich_spinner
from pip._internal.utils.logging import setup_logging

setup_logging(0, no_color=False, user_log_file=None)
with open_rich_spinner("doing work"):
		time.sleep(2)
try:
		with open_rich_spinner("doing work"):
				1/0
except BaseException:
		pass
try:
		with open_rich_spinner("doing work"):
				raise KeyboardInterrupt
except BaseException:
		pass

@ichard26 ichard26 added skip news Does not need a NEWS file entry (eg: trivial changes) C: output Related to what pip prints labels Jun 27, 2025
The benefit of this spinner over our legacy spinners is that Rich will
update and render the spinner automatically for us. This is much nicer
than having to call .spin() manually.
Copy link
Member

@notatallshaw notatallshaw left a comment

Choose a reason for hiding this comment

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

This looks good.

I would be tempted to rename PipRichSpinner to _PipRichSpinner to make it clear it shouldn't be accessed externally.

Also because the PipRichSpinner is running in a thread, if somehow something other than Live called render or finish there could be thread safety issues, but because the context manager doesn't even return the Live object I think this should never happen.

@ichard26 ichard26 enabled auto-merge (squash) July 17, 2025 17:37
@ichard26
Copy link
Member Author

There are two ways thread safety issues can arise:

  • Multiple threads are calling render() or finish() directly, as you've mentioned, the API makes this impossible
  • Multiple threads are writing to the terminal at once, leading to interwoven output. This can be avoided as long as the same console instance is used globally. This is easy to achieve using the get_console() helper that I added in PR Installation progress bar ✨ #13220.

In addition, this code is going to be rolled out in a opt-in basis so there are unexpected issues, they can be addressed at a later point.

Thanks for the review!

@ichard26 ichard26 merged commit f07a4cd into pypa:main Jul 17, 2025
29 checks passed
@ichard26 ichard26 deleted the feat/rich-spinners branch July 17, 2025 18:03
sepehr-rs pushed a commit to sepehr-rs/pip that referenced this pull request Jul 19, 2025
The benefit of this spinner over our legacy spinners is that Rich will
update and render the spinner automatically for us. This is much nicer
than having to call .spin() manually.
pelson pushed a commit to pelson/pip that referenced this pull request Jul 20, 2025
The benefit of this spinner over our legacy spinners is that Rich will
update and render the spinner automatically for us. This is much nicer
than having to call .spin() manually.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: output Related to what pip prints skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants