Skip to content
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

Use rich for logging, tracebacks, printing, progressbars #2350

Merged
merged 3 commits into from
Feb 16, 2023

Conversation

tomaarsen
Copy link
Contributor

@tomaarsen tomaarsen commented Feb 15, 2023

Closes #1843

Hello!

Pull Request overview

  • Use rich for logging, tracebacks, printing and progressbars.
  • Add rich as a dependency.
  • Remove loguru as a dependency and remove all mentions of it in the codebase.
  • Simplify logging configuration according to the logging documentation.
  • Update logging tests.

Before & After

rich is a large Python library for very colorful formatting in the terminal. Most importantly (in my opinion), it improves the readability of logs and tracebacks. Let's go over some before-and-afters:

Printing, Logging & Progressbars

Before:

image

After:

image

Note that for the logs, the repeated information on the left side is removed. Beyond that, the file location from which the log originates is moved to the right side. Beyond that, the progressbar has been updated, ahd the URL in the printed output has been highlighted automatically.

Tracebacks

Before:

image

After:

image


Before:

image

After:

image


Before

image

After

image

Notes

Note that there are some changes in the logging configuration. Most of all, it has been simplified according to the note from here. In my changes, I only attach our handler to the root logger and let propagation take care of the rest.

Beyond that, I've set rich to 13.0.1 as newer versions experience a StopIteration error like discussed here.

I've replaced tqdm with rich Progressbar when logging. However, I've kept the tqdm progressbar for the Weak Labeling for now.

One difference between the old situation and now is that all of the logs are displayed during pytest under "live log call" (so, including expected errors), while earlier only warnings were shown.

What to review?

Please do the following when reviewing:

  1. Ensuring that rich is correctly set to be installed whenever someone installs argilla. I always set my dependencies explicitly in setup.py like here or here, but the one for argilla is empty, and pyproject.toml is used instead. I'd like for someone to look this over.
  2. Fetch this branch and run some arbitrary code. Load some data, log some data, crash some programs, and get an idea of the changes. Especially changes to loggers and tracebacks can be a bit personal, so I'd like to get people on board with this. Otherwise we can scrap it or find a compromise. After all, this is also a design PR.
  3. Please have a look at my discussion points below.

Discussion

rich is quite configurable, so there's some changes that we can make still.

  1. The RichHandler logging handler can be modified to e.g. include rich tracebacks in their logs as discussed here. Are we interested in this?

  2. The rich traceback handler can be set up to include local variables in its traceback:

    Click to see a rich traceback with local variables

    image

    Are we interested in this? I think this is a bit overkill in my opinion.
  3. We can suppress frames from certain Python modules to exclude them from the rich tracebacks. Are we interested in this?

  4. The default rich traceback shows a maximum of 100 frames, which is a lot. Are we interested in reducing this to only show the first and last X?

  5. The progress bar doesn't automatically stretch to fill the full available width, while tqdm does. If we want, we can set expand=True and it'll also expand to the entire width. Are we interested in this?

  6. The progress "bar" does not need to be a bar, we can also use e.g. a spinner animation. See some more info here. Are we interested in this?


Type of change

  • Refactor (change restructuring the codebase without changing functionality)

How Has This Been Tested

I've updated the tests according to my changes.

Checklist

  • I have merged the original branch into my forked branch

  • I added relevant documentation

  • follows the style guidelines of this project

  • I did a self-review of my code

  • I added comments to my code

  • I made corresponding changes to the documentation

  • My changes generate no new warnings

  • I have added tests that prove my fix is effective or that my feature works

  • Tom Aarsen

And move away from loguru. Note that rich is locked to 13.0.1 as the newest version experiences a StopIteration error
@tomaarsen tomaarsen added the type: enhancement Indicates new feature requests label Feb 15, 2023
@tomaarsen
Copy link
Contributor Author

It seems to have some issues installing rich. In particular, I think it's struggling to read the __version__ of argilla without triggering errors that rich hasn't been installed yet. This issue persists regardless of whether rich==13.0.1 is in the - pip: section of the environment_dev.yml or not.

It'll have to be investigated some more.

In order to allow rich to be installed, I had to:
* Allow argilla.__version__ to be accessed without rich installed by hiding the imports under try-except ModuleNotFound errors.
* Update spacy from 3.1.0 to 3.5.0 due to a dependency conflict between spacy==3.1.0 and rich==13.0.1 over the typing-extensions version
@codecov
Copy link

codecov bot commented Feb 16, 2023

Codecov Report

Base: 91.86% // Head: 91.82% // Decreases project coverage by -0.04% ⚠️

Coverage data is based on head (a3495e5) compared to base (c83ec9e).
Patch coverage: 85.71% of modified lines in pull request are covered.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2350      +/-   ##
===========================================
- Coverage    91.86%   91.82%   -0.04%     
===========================================
  Files          161      161              
  Lines         7892     7869      -23     
===========================================
- Hits          7250     7226      -24     
- Misses         642      643       +1     
Flag Coverage Δ
pytest 91.82% <85.71%> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/argilla/__init__.py 86.66% <60.00%> (-13.34%) ⬇️
src/argilla/logging.py 88.46% <80.00%> (+1.18%) ⬆️
src/argilla/client/client.py 87.55% <100.00%> (+0.05%) ⬆️
...gilla/labeling/text_classification/label_errors.py 86.58% <0.00%> (-3.66%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tomaarsen
Copy link
Contributor Author

tomaarsen commented Feb 16, 2023

In order to allow rich to be installed, I had to:

  • Allow argilla.__version__ to be accessed without having rich installed by hiding the imports under try-except ModuleNotFound.
  • Update spacy from 3.1.0 to 3.5.0 due to a dependency conflict between spacy==3.1.0 and rich==13.0.1 over the typing-extensions version.

I'll solve the merge conflicts.

This is ready for review.

Copy link
Member

@frascuchon frascuchon left a comment

Choose a reason for hiding this comment

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

So nice work!

Thanks @tomaarsen

@frascuchon frascuchon merged commit 4c5f513 into argilla-io:develop Feb 16, 2023
@tomaarsen tomaarsen deleted the enhancement/rich branch February 16, 2023 13:45
tomaarsen added a commit that referenced this pull request Jul 14, 2023
Closes #3407

Hello!

# Description

List version restriction of `rich`. This restriction of `<= 13.0.1` was
introduced in #2350 due to a
[bug](Textualize/rich#2800 (comment))
in the version that was most recent back then: version 13.1.0.
However, the issue has been resolved as of the current most recent
version: 13.4.2.
Additionally, #3407 suggests allowing at least up to 13.3.1.

I think the best solution is just to let go of the version restriction
and let pip/conda/poetry install the most recent version.

**Type of change**

(Please delete options that are not relevant. Remember to title the PR
according to the type of change)

- [x] Bug fix

**How Has This Been Tested**

I installed the most recent `rich` and experimented a bit with some
scripts by making them crash etc.

**Checklist**

- [ ] I added relevant documentation
- [ ] follows the style guidelines of this project
- [x] I did a self-review of my code
- [ ] I made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] I filled out [the contributor form](https://tally.so/r/n9XrxK)
(see text above)
- [ ] I have added relevant notes to the CHANGELOG.md file (See
https://keepachangelog.com/)

I'm of the opinion that this is not worthy of a changelog entry.

---

- Tom Aarsen
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement Indicates new feature requests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use Rich for logging, stack traces, and progress bars
2 participants