Switch to auto-updating spinners wholesale - #14238
Conversation
These are easier to work with and are more flexible since they don't have a spin() method that needs to be called constantly. One such auto-updating spinner, _PipRichSpinner, was already added for the inprocess build dependency installer. This commit extends this concept for the rest of the codebase. Please note the new spinner interface is a bit weird. For one, spinners can be marked as finished multiple times with only the first call taking effect. This is necessary to ensure robust error handling in case the spinner's caller doesn't actively manage the spinner itself (which is optional). Furthermore, the final status message is printed even if the spinner was never started. This is carried over from the old spinners. The spinners are interesting since they do two jobs: showing a spinner while pip does work and displaying the final status of said work. Finally, the spinners don't necessarily auto-start. The backend hook caller has some extra code to decide whether to spin the spinner, so providing for manual start control was needed. The overall changes look like this: - Add a new non-interactive spinner that uses a background thread to schedule keep-live "still working" messages - Add a new no-op spinner for when no status/spinner output is desired. Previously this was achieved by relying on the non-interactive spinner's use of logger.info() which will be hidden automagically. This is clever, but an explicit no-op spinner is IMO cleaner. - Simplify the rich spinner a fair bit - Add more documentation, comments, and tests for future maintainability and test the (previously?) spinner selection logic. - ... and finally, delete the old spinners and switch to the new spinners everywhere This migration has the extra benefit of bringing the inprocess build dependency installer's output handling closer to the subprocess installer. I'd never wired a non-interactive spinner for the inprocess installer originally, oops!
It's only used by the raw progress bars now.
Otherwise the build backend/build dep installer tests fail while setting up their spinner.
|
I was hoping this would result in an overall codesize reduction, but unfortunately the additional code documentation and tests made this PR much bigger. I still think this is worth it though as it simplifies the actual business logic. Exchanging logic for more tests and documentation is a good trade, IMO. $ pip diff --stat main... -- src/
src/pip/_internal/build_env/installer.py | 6 +--
src/pip/_internal/cli/progress_bars.py | 16 ++++++-
src/pip/_internal/cli/spinners.py | 292 +++++++++++++++++++++++++++++++++++++++++++++++---------------------------------------------------------------------------------
src/pip/_internal/utils/subprocess.py | 9 ++--
4 files changed, 129 insertions(+), 194 deletions(-) |
sepehr-rs
left a comment
There was a problem hiding this comment.
Thanks for working on this, seems like a nice improvement! I have left a couple of inline comments as someone less familiar with this part of the codebase:
| assert start.called is autostart | ||
|
|
||
|
|
||
| def test_noninteractive_spinner_lifecycle( |
There was a problem hiding this comment.
I may be missing some context, but do we have a test that covers the RichSpinner lifecycle?
There was a problem hiding this comment.
Testing the rich spinner is quite difficult since the logic is largely delegated to the rich library and its spinner implementation. The finish part of the rich spinner lifecycle is already tested separately, so I decided it wasn't worth adding more tests.
OTOH, the non-interactive spinner is entirely custom (before and after this PR) so we ought to test it.
There was a problem hiding this comment.
Oh, thanks for the context! That makes sense.
|
Does this change the look and feel? And if so do you have screenshots or gifs? |
No, it shouldn't. I've spent a fair bit of time ensuring the look and feel would be the same. The one behavioural change I know of is with the inprocess build dependency installer. It's a simple bugfix where it simply never had a non-interactive spinner (unlike the rest of the codebase) leading to inconsistent status reporting. This was mentioned in the PR description at the end. |
Correct news entry
These are easier to work with and are more flexible since they don't have a
spin()method that needs to be called constantly. One such auto-updating spinner,_PipRichSpinner, was already added for the inprocess build dependency installer. This commit extends this concept for the rest of the codebase.Please note the new spinner interface is a bit weird. For one, spinners can be marked as finished multiple times with only the first call taking effect. This is necessary to ensure robust error handling in case the spinner's caller doesn't actively manage the spinner itself (which is optional).
Furthermore, the final status message is printed even if the spinner was never started. This is carried over from the old spinners. The spinners are interesting since they do two jobs: showing a spinner while pip does work and displaying the final status of said work.
Finally, the spinners don't necessarily auto-start. The backend hook caller has some extra code to decide whether to spin the spinner, so providing for manual start control was needed.
The overall changes look like this:
Add a new non-interactive spinner that uses a background thread to schedule keep-live "still working" messages
Add a new no-op spinner for when no status/spinner output is desired. Previously this was achieved by relying on the non-interactive spinner's use of
logger.info()which will be hidden automagically. This is clever, but an explicit no-op spinner is IMO cleaner.Simplify the rich spinner a fair bit
Add more documentation, comments, and tests for future maintainability and test the (previously untested?) spinner selection logic.
... and finally, delete the old spinners and switch to the new spinners everywhere
This migration has the extra benefit of bringing the inprocess build dependency installer's output handling closer to the subprocess installer. I'd never wired a non-interactive spinner for the inprocess installer originally, oops!
Towards #9081, and supersedes and closes #14026.
Tip
This is easier to review in the split diff viewer.