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

Has setup_logging not override already setup loging level. #132

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Carreau
Copy link
Contributor

@Carreau Carreau commented Aug 26, 2024

Add a verbosity=None (and make it the default) for verbosity. If one has already increase the logging level, the default value of verbose should not change it. This is achieved with a default value of None.

Technically I guess we should have a context manager to restaure previous value if True/False is passed, but that will be for another time.

I also think that if verbosity is an int, we should set the lgging level to that int instead of havin an arbitrary verbosity of 0/1/2; but that's also another discussion.

I'm doing this as I'd like adding some logger.debug, but I don't want to pass verbose=True everywhere.

micropip/logging.py Outdated Show resolved Hide resolved
@hoodmane
Copy link
Member

I'm not sure I like the fact that this makes the logging state use the level from the previous call. I think maybe setup_logging could return a manager object with methods to restore the log level at the end of the function and around await. And add a set_log_level method. Something like this:
b877ee2

WDYT?

@Carreau
Copy link
Contributor Author

Carreau commented Aug 26, 2024

Yeah, if you want to push further improvement I'm all for it. feel free to push on my branch.

Alternative is to have set_log_level unconditionally a context manager.

@hoodmane
Copy link
Member

Okay I pushed my changes here.

@hoodmane
Copy link
Member

So now I think we should expose micropip.set_log_level and deprecate the logging argument to micopip.install and micropip.uninstall.

@Carreau
Copy link
Contributor Author

Carreau commented Aug 27, 2024

argument to micropip.install and micropip.uninstall

I see how those can be useful as long as they temporarily increase/decrease level and the default is to no change. IMHO verbose=None|False|True is ment for end user, while setting to level like DEBUG is really for developers.

Regardless it's up to you.

def setup_logging(verbosity: int | bool | None) -> LoggerWrapper:
_set_formatter_once()
assert _logger
result = LoggerWrapper(_logger)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is still wrong, as this changes the logging level permanently for a logger wrapper even if passed None.

I think the conflation of getting a logger and setting the level is creating issues.

Add a verbosity=None (and make it the default) for verbosity.
If one has already increase the logging level, the default value of
verbose should not change it. This is achieved with a default value of
None.

Technically I guess we should have a context manager to restaure
previous value if True/False is passed, but that will be for another
time.

I also think that if verbosity is an int, we should set the lgging level
to that int instead of havin an arbitrary verbosity of 0/1/2; but that's
also another discussion.

I'm doing this as I'd like adding some logger.debug, but I don't want to
pass verbose=True everywhere.

Restore logger level between calls

forward getattr/setattr

unconditional context manager
@Carreau
Copy link
Contributor Author

Carreau commented Sep 13, 2024

Thanks for your patience, and sorry I was on PTO.

Back at this again;

as setup_logging is used only in 2 places I think it's ok to have a bigger change on API.

setup_logging() now takes no arguments, and now return a loggerwrapper with .level(None|True|False|0|1|2) context manager that restore to previous level on exit.

All two usages are now using this new API, this so install and uninstall should behave the same, except if the level is changed outside of those calls, it stay changed.

(still waiting for the tests to pass, and strongly suggest looking at diff with whitespace removed from the GitHub option, as large changes are just indent of function bodies.

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