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, stack traces, and progress bars #1843

Closed
dcfidalgo opened this issue Mar 31, 2022 · 5 comments · Fixed by #2350
Closed

Use Rich for logging, stack traces, and progress bars #1843

dcfidalgo opened this issue Mar 31, 2022 · 5 comments · Fixed by #2350
Labels
area: server Indicates that an issue or pull request is related to the server type: enhancement Indicates new feature requests
Milestone

Comments

@dcfidalgo
Copy link
Contributor

I propose to substitute tqdm and loguru with the rich library:

  • use rich to format the stack traces
  • use rich for the progress bars
  • use rich to format our logging
  • (optional) add a specific logging handler for notebooks
  • (optional) hide certain stack traces in the notebook
@dcfidalgo dcfidalgo self-assigned this Mar 31, 2022
@dcfidalgo dcfidalgo changed the title [Logging] Use rich for logging, stack traces, and progress bars [Rich] Use rich for logging, stack traces, and progress bars Mar 31, 2022
@dcfidalgo dcfidalgo added the area: server Indicates that an issue or pull request is related to the server label Mar 31, 2022
@frascuchon frascuchon transferred this issue from argilla-io/argilla May 26, 2022
@dvsrepo dvsrepo changed the title [Rich] Use rich for logging, stack traces, and progress bars Use Rich for logging, stack traces, and progress bars Nov 4, 2022
@frascuchon frascuchon transferred this issue from another repository Nov 7, 2022
@frascuchon frascuchon added the type: enhancement Indicates new feature requests label Nov 7, 2022
@frascuchon frascuchon added this to the 2023 Q3 milestone Nov 11, 2022
@frascuchon frascuchon moved this to 2023 Q3 in 🚲 Argilla Roadmap Nov 11, 2022
@github-actions
Copy link

This issue is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the status: stale Indicates that there is no activity on an issue or pull request label Dec 27, 2022
@frascuchon frascuchon removed the status: stale Indicates that there is no activity on an issue or pull request label Jan 9, 2023
@tomaarsen
Copy link
Contributor

tomaarsen commented Feb 14, 2023

Some sample outputs when Rich is used:

Loading and logging gutenberg_spacy-ner

Rich:

image

Current (with Loguru & tqdm):

image

Providing an incorrect value for initialising a Record

Rich:

image

Current:

image

I'd love to hear your thoughts on the before and after.

@davidberenstein1957
Copy link
Member

For me, rich is more readable in both cases.

@dvsrepo
Copy link
Member

dvsrepo commented Feb 14, 2023

Same for me!

@tomaarsen
Copy link
Contributor

Alright, wonderful. I'll prepare a PR.

frascuchon pushed a commit that referenced this issue Feb 16, 2023
Closes #1843

Hello!

## Pull Request overview
* Use [`rich`](https://github.com/Textualize/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`](https://github.com/Textualize/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:
<details><summary>Printing, Logging & Progressbars</summary>

### Before:

![image](https://user-images.githubusercontent.com/37621491/219089678-e57906d3-568d-480e-88a4-9240397f5229.png)

### After:

![image](https://user-images.githubusercontent.com/37621491/219089826-646d57a6-7e5b-426f-9ab1-d6d6317ec885.png)

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.

</details>

<details><summary>Tracebacks</summary>

### Before:

![image](https://user-images.githubusercontent.com/37621491/219090868-42cfe128-fd98-47ec-9d38-6f6f52a21373.png)

### After:

![image](https://user-images.githubusercontent.com/37621491/219090903-86f1fe11-d509-440d-8a6a-2833c344707b.png)

---

### Before:

![image](https://user-images.githubusercontent.com/37621491/219091343-96bae874-a673-4281-80c5-caebb67e348e.png)

### After:

![image](https://user-images.githubusercontent.com/37621491/219091193-d4cb1f64-11a7-4783-a9b2-0aec1abb8eb7.png)

---

### Before

![image](https://user-images.githubusercontent.com/37621491/219091791-aa8969a1-e0c1-4708-a23d-38d22c2406f2.png)

### After

![image](https://user-images.githubusercontent.com/37621491/219091878-e24c1f6b-83fa-4fed-9705-ede522faee82.png)

</details>

## Notes
Note that there are some changes in the logging configuration. Most of
all, it has been simplified according to the note from
[here](https://docs.python.org/3/library/logging.html#logging.Logger.propagate).
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](Textualize/rich#2800 (comment)).

I've replaced `tqdm` with `rich` Progressbar when logging. However, I've
kept the `tqdm` progressbar for the [Weak
Labeling](https://github.com/argilla-io/argilla/blob/develop/src/argilla/labeling/text_classification/weak_labels.py)
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](https://github.com/nltk/nltk/blob/develop/setup.py#L115) or
[here](https://github.com/huggingface/setfit/blob/78851287535305ef32f789c7a87004628172b5b6/setup.py#L47-L48),
but the one for `argilla` is
[empty](https://github.com/argilla-io/argilla/blob/develop/setup.py),
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](https://rich.readthedocs.io/en/latest/logging.html#handle-exceptions).
Are we interested in this?
2. The `rich` traceback handler can be set up to include local variables
in its traceback:
<details><summary>Click to see a rich traceback with local
variables</summary>


![image](https://user-images.githubusercontent.com/37621491/219096029-796b57ee-2f1b-485f-af35-c3effd44200b.png)
    </details>
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](https://rich.readthedocs.io/en/latest/progress.html#columns). Are
we interested in this?

---

**Type of change**

- [x] Refactor (change restructuring the codebase without changing
functionality)

**How Has This Been Tested**

I've updated the tests according to my changes.

**Checklist**

- [x] I have merged the original branch into my forked branch
- [ ] I added relevant documentation
- [x] follows the style guidelines of this project
- [x] I did a self-review of my code
- [x] I added comments to 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

- Tom Aarsen
@frascuchon frascuchon modified the milestones: 2023 Q3, v1.4.0 Feb 21, 2023
@davidberenstein1957 davidberenstein1957 moved this from 2023 Q3 to 2023 Q1 in 🚲 Argilla Roadmap May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: server Indicates that an issue or pull request is related to the server type: enhancement Indicates new feature requests
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

5 participants