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

Feature/issue184 loguru debug #204

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

arkinn47
Copy link

@arkinn47 arkinn47 commented Nov 1, 2024

Description

Summary of changes

  • In the init.py file, in the torchquad folder, line 49, which called the set_log_level function was deleted.
  • On lines 3 and 4, a commented line of explanation and the function "logger.disable("torchquad")" were added respectively.
  • A test file logger_test.py was also added in the main folder, which runs a unit test testing if logger works.

Resolved Issues

  • fixes #Issue 184, where when torchquad is imported logger does not properly log ie. logger.info("..."), does not print anything.

How Has This Been Tested?

  • Test A: The test import torchquad and then attempts to write a log with logger.info and then checks if it was logged.

Related Pull Requests

  • #PR

…tly. Allows logging after importing torchquad.
@gomezzz gomezzz changed the base branch from develop to main November 25, 2024 16:04
@gomezzz gomezzz changed the base branch from main to develop November 25, 2024 16:04
Copy link
Collaborator

@gomezzz gomezzz left a comment

Choose a reason for hiding this comment

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

Hi @arkinn47 !

Thanks for the PR, I think this could work! However, I think the problem is slightly more involved than this. Right now, we are using loguru also to display error messages to user via https://github.com/search?q=repo%3Aesa%2Ftorchquad%20logger.error&type=code and https://github.com/search?q=repo%3Aesa%2Ftorchquad+logger.warning&type=code .

  • With logger.disable these would not be shown anymore, so I think we need to change those occurrences then to use something else?
  • Ideally we would set logger.disable only in release builds since we want logs for debugging. If we can't automate, we should at least mention it in contribute or similar? What do you think?

Thanks!

@arkinn47
Copy link
Author

arkinn47 commented Jan 4, 2025

Hello Gomezzz, and happy new year.
If you remove the logger.disable("torchquad") function from line 4 then the error messages are displayed however, according to "Code Snippets and Recipes for Loguru" page, logger.disable("torchquad") should unconditionally be at the top of init.py to prevent the library's log handlers from also receiving user logs, which according to the website is undesirable. So either we can either simply remove logger.disable("torchquad") or leave it in the release builds and inform users. You mentioned "change those occurrences," however I am not sure what we would change them to. So do you just want to remove logger.disable("torchquad") from line 4 or do something else?
Sincerely, Nathan Arkin

@gomezzz
Copy link
Collaborator

gomezzz commented Jan 11, 2025

Hi @arkinn47 ,
thanks for the update. I think it would make sense to replace the logger.error and logger.warning calls with ones using python's built-in warnings and for the errors just prints? That way users still get errors and warnings and we can just set disable as recommended? What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants