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

[logger] adds a ctx manager that restores effective level on exit #943

Merged
merged 5 commits into from Jun 18, 2020

Conversation

renefritze
Copy link
Member

@renefritze renefritze commented Jun 5, 2020

No description provided.

@codecov
Copy link

@codecov codecov bot commented Jun 8, 2020

Codecov Report

Merging #943 into master will not change coverage.
The diff coverage is n/a.

@sdrave
Copy link
Member

@sdrave sdrave commented Jun 8, 2020

I have thought some more about this.

I think that scoped_logger is more about setting log levels than about getting a logger. (As in your use case in the hypothesis2 branch.) So maybe it would make more sense to let set_log_levels be a context manager. I think that this should be doable. (I can try if you wish.)

@renefritze
Copy link
Member Author

@renefritze renefritze commented Jun 8, 2020

I actually thought about that approach, but decided to include the getting the logger object bit, since otherwise I would need to pass the logger into the context manager and have more code to write at the usage point if I don't already have the logger.
If you still want to add the set_log_levels manager, one could just use that in the scoped_logger.
Or is it the name you don't like?

@sdrave
Copy link
Member

@sdrave sdrave commented Jun 9, 2020

In the hypothesis2 branch, you use scoped_logger as follows:

    with scoped_logger('pymor.algorithms.gram_schmidt.gram_schmidt_biorth', level='FATAL'):
        A1, A2 = gram_schmidt_biorth(U1, U2, copy=True)

This would simply become

    with set_log_levels({'pymor.algorithms.gram_schmidt.gram_schmidt_biorth': 'FATAL'}):
        A1, A2 = gram_schmidt_biorth(U1, U2, copy=True)

So in this case, you actually have a few characters less.

Using scoped_logger to get a logger with prescribed log level smells like encouraging bad practice to me: in general, you want to control from the outside, what logging information you get from nested function calls (like in your example). Everywhere where you set the log level of your own logger, you break that functionality for everyone calling you.

Do you have a concrete use case in mind, where you would actually want to use the logger returned by scoped_logger?

Or is it the name you don't like?
This as well. Probably confusing for inexperienced programmers.

@renefritze
Copy link
Member Author

@renefritze renefritze commented Jun 9, 2020

So in this case, you actually have a few characters less.

Huh. If that's the only use you found I must've misremembered.
In that case the signature+name change is fine with me.

@renefritze
Copy link
Member Author

@renefritze renefritze commented Jun 9, 2020

set_log_levels is already taken. How about temporary_log_levels?

@sdrave
Copy link
Member

@sdrave sdrave commented Jun 9, 2020

My idea would be to modify set_log_levels to return a context manager which can be optionally used. Similar to the behavior of open.

@renefritze
Copy link
Member Author

@renefritze renefritze commented Jun 12, 2020

My idea would be to modify set_log_levels to return a context manager which can be optionally used. Similar to the behavior of open.

Currently the user expectation for set_log_levels is that settings are permanent though. Not sure how you'd make it clear that's not the case when used as a context manager. I feel a dedicated name/function could be better here.

@sdrave
Copy link
Member

@sdrave sdrave commented Jun 15, 2020

Currently the user expectation for set_log_levels is that settings are permanent though. Not sure how you'd make it clear that's not the case when used as a context manager. I feel a dedicated name/function could be better here.

For me it would be obvious that the change is only temporary when set_log_levels is used as a context manager. If it's a separate function I first to need to know about its existence. If you think otherwise, maybe we should ask the other @pymor/pymor-devs about their opinion.

@pmli
Copy link
Member

@pmli pmli commented Jun 15, 2020

I don't know what is the goal of this PR... There were mentions of the hypothesis branch. Is this something used only in the tests or also by users?

@renefritze
Copy link
Member Author

@renefritze renefritze commented Jun 15, 2020

This would be general user-facing.

@pmli
Copy link
Member

@pmli pmli commented Jun 15, 2020

Ok. And what issue is this PR trying to fix?

@renefritze
Copy link
Member Author

@renefritze renefritze commented Jun 15, 2020

For me: temporarily/localized silencing an algorithms log output in a loop with restoring previous levels after the block.

@pmli
Copy link
Member

@pmli pmli commented Jun 15, 2020

This reminds me of the context manager in warnings for temporarily suppressing warnings. After some googling, I also found a section about context managers in the Logging Cookbook. Is this helpful?

@sdrave
Copy link
Member

@sdrave sdrave commented Jun 15, 2020

@pmli, the question is whether to allow temporary setting of log levels via

with set_log_levels({'pymor.models.iosys': 'WARN'}):
    do_some_stuff()

or if it would be better to use

with set_log_levels_temporarily({'pymor.models.iosys': 'WARN'}):
    do_some_stuff()

as the user might think that the change is not temporary, even though the function is used in a with statement.

@pmli
Copy link
Member

@pmli pmli commented Jun 15, 2020

I agree with René, using set_log_levels for two different things (permanent and temporary setting) might be confusing. On the other hand, using temporarily seems redundant. How about just log_levels:

with log_levels({'pymor.models.iosys': 'WARN'}):
    do_some_stuff()

This then reads like English.

@sdrave
Copy link
Member

@sdrave sdrave commented Jun 16, 2020

I still dislike having two different functions for doing basically the same thing. @renefritze, @pmli, you are aware that

f = open('foo', 'wt')
with f as g:
    pass
f.write('bar')

will give you an exception? Even Python builtins have such behavior. Or do you think this is a different situation?

@renefritze
Copy link
Member Author

@renefritze renefritze commented Jun 17, 2020

open gives you a new object to manipulate and when used as a context manager follows RAII. log_levels changes existing global state and you do not get an object. The actions of __exit__ are restricted to the object you got back for open.

Also closing a file is not restoring its state to how it was before it was opened.

So yes, very different situation.

@pmli
Copy link
Member

@pmli pmli commented Jun 17, 2020

I agree with René again. As far as I see, open is not so special, but the file object it returns is (it has __enter__ and __exit__ methods which make it usable as a context manager). On the other hand, making a setter method like set_log_levels return something doesn't seem right (e.g., when using it interactively, it would print the context manager).

@sdrave
Copy link
Member

@sdrave sdrave commented Jun 17, 2020

With

Even Python builtins have such behavior. Or do you think this is a different situation?

I was meaning that the file object can be either used as a file object or as a context manager giving you a file object (itself). You must admit there is some similarity here.

But I see the argument about global state and that a setter should return nothing. So let's go with a new log_levels function. One could actually think about making it an object storing some log level configuration that also has an apply method. Then set_log_levels(foo) could be a shorthand for log_levels(foo).apply() that maybe gets deprecated in the future so I can be happy again, having only one log level setting related function in the distant future ..

@renefritze
Copy link
Member Author

@renefritze renefritze commented Jun 18, 2020

So I'll merge as is and @sdrave makes an issue for the future to implement his idea?

@sdrave
Copy link
Member

@sdrave sdrave commented Jun 18, 2020

Ok.

@renefritze renefritze self-assigned this Jun 18, 2020
@renefritze renefritze added automerge pr:new-feature labels Jun 18, 2020
@renefritze renefritze added this to the 2020.1 milestone Jun 18, 2020
src/pymortests/core/logger.py Outdated Show resolved Hide resolved
@github-actions github-actions bot merged commit 812a881 into master Jun 18, 2020
8 checks passed
@github-actions github-actions bot deleted the scopedlogger branch Jun 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:new-feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants