BUG: allows logging config under distributed key

[deniederhut]

The logging documentation specifies a configuration path to
logging info like config['distributed']['logging'], but the
config module looks in config['logging']. This commit allows
both by looking first in config['distributed'], then falling
back to config.

Closes #2937

[jakirkham]

@mrocklin, would you be able to review this?

[mrocklin]

I don't think anyone knows much about the logging code here (it may have been Antoine's). @jcrist might be the best person to think about how we can improve the logging/config situation. Perhaps we can ask him?

[mrocklin]

poke @jcrist , if you have a minute

(sorry for letting this linger @deniederhut )

[deniederhut]

No worries - thanks for circling back to it!

…

On Mon, Sep 9, 2019, 18:14 Matthew Rocklin ***@***.***> wrote: poke @jcrist <https://github.com/jcrist> , if you have a minute (sorry for letting this linger @deniederhut <https://github.com/deniederhut> ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2952?email_source=notifications&email_token=ABUM3LBN67WBWOHPCIO4NITQI3KGZA5CNFSM4ILGQ5O2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6JJ5MQ#issuecomment-529702578>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABUM3LB7W7NY4C6R4DQR7W3QI3KGZANCNFSM4ILGQ5OQ> .

[jcrist]

Thanks for the fix @deniederhut. Overall this seems fine to me. Could you add a test that logging configuration is picked up under distributed.logging? The test would go in https://github.com/dask/distributed/blob/master/distributed/tests/test_config.py.

I'm also wondering if we should deprecate/remove the top-level location. It's not clear to me if this was a bug, or legacy from the old config system. My preference would be to at least deprecate it (this could be done in a separate PR), as all distributed config should really be under the distributed key only.

[mrocklin]

It's old/legacy. I'd be happy to see it removed.

…

On Thu, Sep 12, 2019 at 10:18 AM Jim Crist ***@***.***> wrote: Thanks for the fix @deniederhut <https://github.com/deniederhut>. Overall this seems fine to me. Could you add a test that logging configuration is picked up under distributed.logging? The test would go in https://github.com/dask/distributed/blob/master/distributed/tests/test_config.py . I'm also wondering if we should deprecate/remove the top-level location. It's not clear to me if this was a bug, or legacy from the old config system. My preference would be to at least deprecate it (this could be done in a separate PR), as all distributed config should really be under the distributed key only. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2952?email_source=notifications&email_token=AACKZTBRRY7WIFYV2AJZCU3QJJ2WLA5CNFSM4ILGQ5O2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD6STSFY#issuecomment-530921751>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AACKZTG6U7WM45L2YOM2S7TQJJ2WLANCNFSM4ILGQ5OQ> .

[deniederhut]

Okay, test in place. Should I open an issue for deprecating the older logging config placement? Or just open a second PR that addresses this?

[mrocklin]

cc @quasiben for review

[mrocklin]

@jcrist would you be willing to review/manage this PR?

[mrocklin]

Ping @jcrist , who we recently realized wasn't getting github notifications.

@jcrist no pressure (I know I just pinged you on other things as well), but if you have time it would be great to have someone take a long look at how we do logging configuration.

[mrocklin]

@jcrist are you planning to look at this? If no, that's fine, but it'd be good to get a response if you're able.

[deniederhut]

Hey @mrocklin, have we found someone to review this PR?

[mrocklin]

I personally haven't done anything here, no. Is that what you were asking?

[deniederhut]

No worries! Things have been quiet for a while so I just wanted to check in

[quasiben]

@deniederhut thanks again for this PR. I merged with master and I hope to get this in today. I think I only have a few questions. Currently, when setting logging config from the CLI the user does something like the following:

❯ DASK_LOGGING__DISTRIBUTED=debug python -c "import dask, distributed; print(dask.config.get('logging'))"
{'distributed': 'debug'}

I was expecting with this PR to be able to be able to use DASK__DISTRIBUTED_LOGGING=debug but this does not seem to work:

❯ DASK__DISTRIBUTED_LOGGING=debug python -c "import dask, distributed; print(dask.config.get('distributed.logging'))"
{'distributed': 'info'}

[jcrist]

@quasiben, I think you meant:

$ DASK_DISTRIBUTED__LOGGING=debug python -c "import dask, distributed; print(dask.config.get('distributed.logging'))"
debug

which works fine (no __ after DASK is needed, but __ from then on indicate . in the key).

[jcrist]

Apologies for letting this sit so long. I've kicked off a new build (travis ci only, just to check that things haven't broken while idle). Will merge on tests pass.

[quasiben]

@jcrist thanks for the correction

[TomAugspurger]

All green, merging.

Thanks for sticking with this @deniederhut. Sorry for the delay!