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

Don't import pretty_errors #2543

Merged
merged 2 commits into from
May 17, 2024
Merged

Conversation

kalekundert
Copy link
Contributor

@kalekundert kalekundert commented May 15, 2024

It's really inappropriate for a low-level library like this to be making global changes to the way stack traces are rendered. Especially since the "pretty" stack traces don't include file names or line numbers!

This is just crazy. All I wanted to do was import lightning, and I had to spend a couple hours tracking down why my stack traces were suddenly useless. If users want this behavior, they should knowingly opt-in themselves. They shouldn't be forced in by some dependency of a dependency of a dependency.


📚 Documentation preview 📚: https://torchmetrics--2543.org.readthedocs.build/en/2543/

@Borda
Copy link
Member

Borda commented May 15, 2024

Thank you for sharing your opinion; how/why do you install debug dependency in the first place? 🤔
ref: https://pypi.org/project/torchmetrics/1.4.0.post0 and #2527

@kalekundert
Copy link
Contributor Author

I just installed lightning, yesterday:

$ python -m venv venv
$ . venv/bin/activate
$ pip install torchmetrics==1.4.0
...
$ pip freeze | grep pretty
pretty-errors==1.2.25

It's good that torchmetrics doesn't break stack traces by default as of 1.4.0.post0, but there's still no reason for this library to have the potential to do so. If a user want to use pretty_errors, they should modify site_customize.py or alias python, as recommended by pretty_errors itself.

src/torchmetrics/__init__.py Show resolved Hide resolved
Copy link

codecov bot commented May 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69%. Comparing base (bfcd4b0) to head (1b3fe93).
Report is 102 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master   #2543   +/-   ##
======================================
  Coverage      69%     69%           
======================================
  Files         313     313           
  Lines       17617   17615    -2     
======================================
- Hits        12162   12161    -1     
+ Misses       5455    5454    -1     

@mergify mergify bot added the ready label May 15, 2024
@stancld stancld enabled auto-merge (squash) May 17, 2024 06:52
@stancld stancld merged commit 4c2a143 into Lightning-AI:master May 17, 2024
69 checks passed
Borda pushed a commit that referenced this pull request Aug 2, 2024
* fix: don't import `pretty_errors`

* fix: remove `pretty_errors` as a debug requirement

(cherry picked from commit 4c2a143)
Borda pushed a commit that referenced this pull request Aug 2, 2024
* fix: don't import `pretty_errors`

* fix: remove `pretty_errors` as a debug requirement

(cherry picked from commit 4c2a143)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants