Allow central scheduler logging to be configured based on the logging… #1633
Conversation
….conf file specified in luigi.cfg.
|
What is it that the central scheduler could log that you want to see? (mainly just curious and uninformed) + tests? https://github.com/spotify/luigi/blob/master/test/cmdline_test.py |
|
Haha, I just needed this last Monday! I got surprised nobody had sent a PR yet. You're two days late! :D |
| logging_conf = config.get('core', 'logging_conf_file', None) | ||
| if logging_conf is not None and not os.path.exists(logging_conf): | ||
| logging_conf = None | ||
| if logging_conf is not None: |
There was a problem hiding this comment.
This code block can be simplified I think. Why first logging_conf = None and then if logging_conf is not None. Just introduce the variable in the code path that you use it.
There was a problem hiding this comment.
Since L47-L49 may not execute, the variable logging_conf needs to be defined as something in order to check if it is not None (L50), right? (thus why the variable declaration at L45). At least, that's how i'm thinking through it.
There was a problem hiding this comment.
Eh.. Yea.
Actually I think the server should just plain out die and not start if the condition logging_conf is not None and not os.path.exists(logging_conf) holds true.
There was a problem hiding this comment.
Changed to raise exception if the logging conf file is missing.
|
Also, can you please also add |
|
@dlstadther "What is it that the central scheduler could log that you want to see?" |
|
Added unit tests and print statements. PTAL. Also let me know if you want me to squash the commits. |
|
Looks good to me. I will squash-accept this PR so no need to merge commits. |
I think it makes sense to log less, because currently I think INFO level should not be verbose. In particular "No workers connected" message is verbose and going to be said many more times than it will be actually removed (when used with assistants). Further, I think now is a sane time to change the name of the logger to the "recommended" `getLogger(__name__)` style. Because PR spotify#1633 was just merged and it's less likely that people have relied on the logger name prior to that.
I think it makes sense to log less, because currently I think INFO level should not be verbose. In particular "No workers connected" message is verbose and going to be said many more times than it will be actually removed (when used with assistants). Further, I think now is a sane time to change the name of the logger to the "recommended" `getLogger(__name__)` style. Because PR #1633 was just merged and it's less likely that people have relied on the logger name prior to that.
….conf file specified in luigi.cfg.
Although the luigi configuration file has an option to specify a logging conf file, this option currently only applies to the workers, not the central scheduler, which AFAICT ignores any logging configuration and starts up with a basic configuration. I poked around and didn't see any way to configure the central scheduler's logging other than hacking on its entry point; hence this PR.