Fix logging via toml #2593
Fix logging via toml #2593
Conversation
luigi/setup_logging.py
Outdated
| @@ -40,7 +40,7 @@ def _section(cls, opts): | |||
| """Get logging settings from config file section "logging".""" | |||
| try: | |||
| logging_config = cls.config['logging'] | |||
| except (TypeError, KeyError, NoSectionError): | |||
| except (TypeError, KeyError, NoSectionError, AttributeError): | |||
There was a problem hiding this comment.
Do we want the attribute error to allow logging_config = cls.config.options('logging') like @riga suggested? Or is the best course of action to just return False?
There was a problem hiding this comment.
We can't configure logging via section in cfg. It have to be False for LuigiConfigParser.
There was a problem hiding this comment.
For section based logging configuring we need dicts in dists, but cfg format doesn't support it as opposite to toml, yaml, json and other good formats.
There was a problem hiding this comment.
For section based logging configuring we need dicts in dists, but cfg format doesn't support it as opposite to toml, yaml, json and other good formats.
Is there a difference between Python 2 and 3?
There was a problem hiding this comment.
It's particular point of cfg format as is. There can be mistake only if cfg-based config has logging section, but there is no reason to do it.
There was a problem hiding this comment.
Then I maybe just don't get why this block exists at all:
Lines 74 to 78 in 8278773
configured will always be False in 2.7, and True or False in 3.X, depending on whether the logging section exists. That's what I meant with my previous comment.
There was a problem hiding this comment.
It's True for toml-based config with logging section for any Python version.
There was a problem hiding this comment.
Also it will be true for YAML or JSON based configs in future. I hope, we will add it.
There was a problem hiding this comment.
I would also prefer not to catch AttributeError but handle this case explicitly.
|
As I commented #2592 (comment), 2.8.0 didn't have this issue before the bug was introduced in later PR. However we just release 2.8.1 yesterday that that became a broken version. Any progress on this PR? |
|
I've added explicit check for |
|
LGTM. @dlstadther @Tarrasch any thought? I wasn't following the original PR. |
|
@dlstadther please merge and release a hotfix, thanks! |
|
@honnix Done |
|
Awesome. Thanks! |
Description
Wrap AttributeError when try to configure logging via config section. Cfg file doesn't support inheritance enough for logging configuring, so we can skip logging configuring via config section when cfg-based config used.
Motivation and Context
Fix #2592
Have you tested this? If so, how?
Test included. Failed before fix, success after