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

Improve console logging and tracebacks #105

Merged
merged 8 commits into from
Feb 13, 2024
Merged

Conversation

ibro45
Copy link
Collaborator

@ibro45 ibro45 commented Feb 1, 2024

Description

  • Rich added to our loguru
  • Loguru captures all loggers from other libraries
  • Loguru captures warnings made through warnings standard library
  • Tracebacks improved through Rich, both by looking neater, and by suppressing (not printing the code, only the location) of select modules. Those modules include fire, monai.bundle, PL's Trainer, etc. This should be updated as we notice unnecessary module.

This is what it currently looks like when you try to specify a non-existing class in the config.

image

Related Issue


Type of Change

  • 📚 Examples / docs / tutorials / dependencies update
  • 🔧 Bug fix (non-breaking change which fixes an issue)
  • 🥂 Improvement (non-breaking change which improves an existing feature)
  • 🚀 New feature (non-breaking change which adds functionality)
  • 💥 Breaking change (fix or feature that would cause existing functionality to change)
  • 🔐 Security fix

Checklist

  • I've read the CODE_OF_CONDUCT.md document.
  • I've read the CONTRIBUTING.md guide.
  • I've updated the code style using make codestyle.
  • I've written tests for all new methods and classes that I created.
  • I've written the docstring in Google format for all the methods and classes that I used.

…s in tracebacks. Capture loggers and warnings from other libraries.
@surajpaib
Copy link
Collaborator

Thanks for doing this! I think it looks great overall.

Something more for discussion here - should we suppress all INFO messages by default and provide it only when a verbose flag is set? I feel like on repeated runs, it does not provide any useful information

If Jessica has the time, we should also get her feedback on this logging bit.

@ibro45
Copy link
Collaborator Author

ibro45 commented Feb 1, 2024

should we suppress all INFO messages by default and provide it only when a verbose flag is set?

I'm leaning towards keeping them, as it seems too radical not to, but will need to think about it. Feels like it could bring more confusion than benefit?

If Jessica has the time, we should also get her feedback on this logging bit.

Probably once we merged a version with master

One thing that I'm not happy with is when warnings warnings are called in PyTorch Lightning from a function in their rank_zero module instead of on the spot, so you cannot go directly to the file where it happened to examine it.

image For PL, we can check if it's coming from `rank_zero` module and then just increase the depth in `logger.opt(depth=2).warning` from 2 to 3 to get the module before it.

@ibro45
Copy link
Collaborator Author

ibro45 commented Feb 2, 2024

Not looking good yet actually - opened an issue because I cannot seem to figure out what's going on Textualize/rich#3269

@dosu-bot - can you look into it?

@ibro45
Copy link
Collaborator Author

ibro45 commented Feb 6, 2024

I would merge this as-is right now. Still beneficial to have it on master, even though it should be improved down the road.

To improve:

@ibro45 ibro45 marked this pull request as ready for review February 6, 2024 14:53
@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Feb 6, 2024
@ibro45 ibro45 requested a review from surajpaib February 6, 2024 14:53
@surajpaib
Copy link
Collaborator

@ibro45 Mind if I take a look at the Dataloader logging functionality?

A lot of errors are usually spawned from this and it would be nice to have a more informed logging here.

@surajpaib
Copy link
Collaborator

Everything else LGTM!

@ibro45
Copy link
Collaborator Author

ibro45 commented Feb 6, 2024

@ibro45 Mind if I take a look at the Dataloader logging functionality?

A lot of errors are usually spawned from this and it would be nice to have a more informed logging here.

Oh please do! We can merge it after then

@surajpaib
Copy link
Collaborator

surajpaib commented Feb 13, 2024

Lets merge this for now @ibro45 ? More improvements pending fix of Project-MONAI/MONAI#7451

@surajpaib
Copy link
Collaborator

Also addresses #104 and removes lightly

@surajpaib surajpaib merged commit 72d4c66 into main Feb 13, 2024
7 checks passed
@surajpaib surajpaib deleted the logging_and_traceback branch February 13, 2024 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants