Skip to content

Conversation

peterjdolan
Copy link
Contributor

@peterjdolan peterjdolan commented Mar 26, 2025

Description

Many long-running commands produce little or no feedback in the terminal to indicate that they're progressing, and none of them provide estimates of how long the operation will run. This change introduces the enlighten python package, which displays progress bars akin to TQDM below the existing terminal output. This PR does not modify the import workflow, rather only modifies several built-in plugins. Modifying the import workflow to display a progress bar will be done in a follow-up PR.

See tracking issue

To Do

  • Documentation. (If you've added a new command-line flag, for example, find the appropriate page under docs/ to describe it.)
  • Changelog. (Add an entry to docs/changelog.rst to the bottom of one of the lists near the top of the document.)
  • Tests. (Very much encouraged but not strictly required.)

Example screenshot

Screenshot 2025-04-10 103457

Copy link

Thank you for the PR! The changelog has not been updated, so here is a friendly reminder to check if you need to add an entry.

@peterjdolan peterjdolan changed the title Add progress bars to several commands. Feature: Add progress bars Apr 7, 2025
@peterjdolan
Copy link
Contributor Author

Import operation partially implemented; still needs some work. Example screenshot above.

@snejus snejus requested review from snejus and Copilot and removed request for snejus April 14, 2025 01:04
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 24 out of 25 changed files in this pull request and generated 2 comments.

Files not reviewed (1)
  • docs/changelog.rst: Language not supported

@peterjdolan peterjdolan force-pushed the tqdm branch 3 times, most recently from bc02408 to c64e577 Compare April 14, 2025 18:52
@peterjdolan peterjdolan requested a review from Copilot April 17, 2025 03:10
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 24 out of 25 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • docs/changelog.rst: Language not supported
Comments suppressed due to low confidence (1)

beetsplug/replaygain.py:1560

  • Removal of the SystemExit and KeyboardInterrupt exception handling in the replaygain command may affect how user interruptions are managed. Please confirm that propagating these exceptions, rather than silencing them, is the intended behavior.
finally:

@peterjdolan
Copy link
Contributor Author

@snejus I'm not sure why the latest push is failing the automatic lint and formatting; it passes 'poetry run poe lint && poetry run poe format' on my machine just fine. Any suggestions? I've already sync'd my fork to beets master and rebased.

@wisp3rwind
Copy link
Member

wisp3rwind commented Apr 22, 2025

This seems to be related to some combination of (although I'm not seeing the full picture yet)

Someone should probably open a new PR to fix all TC006 warnings, include TC in our ruff rule selection instead of TCH, and probably fix ruff to a major version (a 0.X.*). Since major ruff updates might contain breaking rule/style changes, I'd guess that we'd only want to bump it manually.

I do get the same TC006 error if I locally run the system-installed ruff 0.11.0.

@semohr
Copy link
Contributor

semohr commented Sep 15, 2025

Is there a way to disable the progress bar?

We’re running in a containerized environment without a cli UI, where all stdout and stderr are redirected into log files. In similar cases, progress libraries (e.g. tqdm) tend to generate a lot of noisy log output, as each terminal update is written as a new line. I’m not sure how enlighten handles this and was not able to derive this from their docs.

Just wanted to mention this here to spare us some trouble down the line. Do you see any other problems regarding this use case?

@jackwilsdon
Copy link
Member

It looks like this needs to replicate the check enlighten.get_manager does - disabling the manager if the output is not a TTY.

You might be best actually just using enlighten.get_manager and passing enabled to that, as it will only enable the manager if the provided enable value was true and the stream is a TTY.

@peterjdolan
Copy link
Contributor Author

It looks like this needs to replicate the check enlighten.get_manager does - disabling the manager if the output is not a TTY.

You might be best actually just using enlighten.get_manager and passing enabled to that, as it will only enable the manager if the provided enable value was true and the stream is a TTY.

Great points, thank you. I'll add that condition as well.

@peterjdolan peterjdolan force-pushed the tqdm branch 4 times, most recently from c698875 to 83155c0 Compare September 25, 2025 00:44
@peterjdolan peterjdolan requested a review from Copilot September 25, 2025 17:05
@peterjdolan
Copy link
Contributor Author

This is now ready for review. I'm not certain how to request a review, though. Anybody have pointers?

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Copilot reviewed 20 out of 21 changed files in this pull request and generated 5 comments.


Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@peterjdolan peterjdolan force-pushed the tqdm branch 3 times, most recently from 37ab9c0 to 3f46e56 Compare September 26, 2025 20:03
Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

New security issues found

)
try:
output = check_output(cmd, stderr=STDOUT)
output: bytes = check_output(cmd, stderr=STDOUT)
Copy link
Contributor

Choose a reason for hiding this comment

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

security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'check_output' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.

Source: opengrep

Many long-running commands produce little or no feedback in the terminal to
indicate that they're progressing, and none of them provide estimates of how
long the operation will run. This change introduces the `enlighten` python
package, which displays progress bars akin to TQDM below the existing terminal
output.

To support consistent use and presentation of the progress bars, and to
allow for future modification, we introduce a method to beets.ui -
beets.ui.iprogress_bar - which can be used by Beets' core commands and all
Beets plugins. Example usage is provided in the methods' documentation
and in a new 'further reading' doc for developers.

The Enlighten library does not work as well in Windows PowerShell as it
does in a linux terminal (manually tested in Zsh), so the progress bars
are disabled in Windows environments. Resolving these issues and enabling
them in Windows is left as future work.

Progress Bars are disabled also when not in a TTY, to prevent
interference with logging when running as a web server, for example.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants