-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
[BUG] Rich traceback is not working with @inject from " ets-labs / python-dependency-injector" #2800
Comments
I think I found the issue while I was reading the error messages and I tried to do some modifications inside the In the file def tokens_to_spans() -> Iterable[Tuple[str, Optional[Style]]]:
"""Convert tokens to spans."""
tokens = iter(line_tokenize())
line_no = 0
_line_start = line_start - 1 if line_start else 0
# Skip over tokens until line start
while line_no < _line_start:
try:
current_value = next(tokens)
print(f"-------------------->>>>>>>>>> current value: {current_value}")
_token_type, token = current_value
except StopIteration as err:
print(f"-------------------->>>>>>>>>>>>>Error: err ({err})")
break
yield (token, None)
if token.endswith("\n"):
line_no += 1
# other code is omitted Then in the shell I saw that it was empty: -------------------->>>>>>>>>> current value: (Token.Comment.Single, '# no possibility to compile a first-type citizen coroutine in Cython.')
-------------------->>>>>>>>>> current value: (Token.Text.Whitespace, '\n')
-------------------->>>>>>>>>> current value: (Token.Text.Whitespace, '\n')
-------------------->>>>>>>>>>>>>Error: err ()
-------------------->>>>>>>>>> current value: (Token.Literal.String.Doc, '"""\n')
-------------------->>>>>>>>>> current value: (Token.Literal.String.Doc, '"""')
-------------------->>>>>>>>>> current value: (Token.Text.Whitespace, '\n')
-------------------->>>>>>>>>> current value: (Token.Text.Whitespace, '\n')
-------------------->>>>>>>>>> current value: (Token.Keyword.Namespace, 'from')
-------------------->>>>>>>>>> current value: (Token.Text, ' ')
-------------------->>>>>>>>>> current value: (Token.Name.Namespace, 'uuid')
-------------------->>>>>>>>>> current value: (Token.Text, ' ')
-------------------->>>>>>>>>> current value: (Token.Keyword.Namespace, 'import')
-------------------->>>>>>>>>> current value: (Token.Text, ' ')
-------------------->>>>>>>>>> current value: (Token.Name, 'UUID')
-------------------->>>>>>>>>> current value: (Token.Text.Whitespace, '\n')
-------------------->>>>>>>>>> current value: (Token.Text.Whitespace, '\n')
-------------------->>>>>>>>>> current value: (Token.Keyword.Namespace, 'from') And because of that it was throwing exception from inside the library. |
I am able to reproduce this problem in a different context (a traceback involving
The full error is this:
With 13.0.1 you instead get
Note that since this is converting a caught-and-logged exception to an uncaught exception, it breaks code. |
This looks like it's caused by a change in the read_code function in #2754. Previously, if the file couldn't be read, that would cause an exception inside That said, it seems possible that we can read the file, but the length has changed, so I think catching |
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
@willmcgugan, are you able to take a look at this? It's forcing a bunch of people to stay pinned on an old version of |
PR is always helpful |
Re-open if not fixed in v13.3.2 |
I hope we solved your problem. If you like using Rich, you might also enjoy Textual |
Looks like it works; our regression test passes with the latest version of rich. Thank you for the fix (and for the library in general)! |
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
Describe the bug
Hi I opened the same issue here also ets-labs/python-dependency-injector#667 as I don't know which one must fix this issue.
It happens when I use the @Inject decorator
Platform
Click to expand
What platform (Win/Linux/Mac) are you running on? What terminal software are you using?
I may ask you to copy and paste the output of the following commands. It may save some time if you do it now.
If you're using Rich in a terminal:
pip freeze | grep rich rich==13.3.1
If you're using Rich in a Jupyter Notebook, run the following snippet in a cell
and paste the output in your bug report.
The text was updated successfully, but these errors were encountered: