Skip to content

Conversation

@jstlaurent
Copy link
Contributor

@jstlaurent jstlaurent commented Jan 27, 2025

Changelogs

  • Use rich to track progress, instead of Halo
  • Use rich for logging, instead of Loguru, to harmonize CLI output
  • Scope logging configuration to the library
  • Remove Loguru, tqdm, and Halo as dependencies

Adresses issue #255

Preview

Screenshot 2025-01-27 at 9 50 57 AM

Checklist:

  • Was this PR discussed in an issue? It is recommended to first discuss a new feature into a GitHub issue before opening a PR.
  • Add tests to cover the fixed bug(s) or the newly introduced feature(s) (if appropriate).
  • Update the API documentation if a new function is added, or an existing one is deleted.
  • Write concise and explanatory changelogs above.
  • If possible, assign one of the following labels to the PR: feature, fix, chore, documentation or test (or ask a maintainer to do it for you).

When used in iPython, Halo raises an exception. The bug has been patched in their repo secven months ago, but no new release has been created since. The last release is more than four years old.

In order to fix this, I elected to use rich, a CLI formatting library that has progress trackers. It is up-to-date and well maintained. I replaced the ProgressIndicator class with a context manager function that wraps a Progress object from rich. In addition to a spinner, we also get progress bars. By using a ContextVar, we can nest multiple track_progress context manager, to track multiple tasks.

In order not to break the visual output in the CLI, I also used the logging handler provided by rich, replacing the Loguru library we had been using. I also took the opportunity to scope the logging configuration to the Polaris library logging only, to avoid changing any config the user might have set up in their code.

@jstlaurent jstlaurent added the fix Annotates any PR that fixes bugs label Jan 27, 2025
@jstlaurent jstlaurent requested a review from cwognum as a code owner January 27, 2025 04:34
@jstlaurent jstlaurent self-assigned this Jan 27, 2025
Copy link
Contributor

@mercuryseries mercuryseries left a comment

Choose a reason for hiding this comment

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

Thanks, @jstlaurent, for tackling this over your busy weekend! 😉
I think this is the right call. I looked into this after issue #255 and was leaning towards Rich as well. Added a few minor comments here and there, nothing major!

Copy link
Collaborator

@cwognum cwognum left a comment

Choose a reason for hiding this comment

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

Thanks @jstlaurent !

Could you show a screenshot of what it now looks like?

jstlaurent and others added 2 commits January 27, 2025 09:40
Co-authored-by: Honoré Hounwanou <mercuryseries@gmail.com>
Co-authored-by: Honoré Hounwanou <mercuryseries@gmail.com>
@cwognum
Copy link
Collaborator

cwognum commented Jan 27, 2025

Thanks for the screenshot @jstlaurent ! Looks great to me!

@jstlaurent jstlaurent merged commit 5520fe8 into main Jan 27, 2025
19 checks passed
@jstlaurent jstlaurent deleted the fix/halo-error branch January 27, 2025 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Annotates any PR that fixes bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants