Add some typing and support existing handlers

[superdosh]

This makes the logic a bit more complex as we don't just clear all existing handlers, but add handlers if they don't already exist.

Also adds some typing so mypy doesn't complain downstream.

[github-actions]

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

[wpietri]

I like the goals of this, but you have added a fair bit more complexity that is not covered by tests.

Are their normal circumstances where handlers already exist but that we want to format them? If so, I'd say add tests explicitly for those circumstances.

If this is just for pytest, though, I'd say it doesn't matter what formatting is in use, and so you can drop some of the complexity.

[superdosh]

Are their normal circumstances where handlers already exist but that we want to format them? If so, I'd say add tests explicitly for those circumstances.

If this is just for pytest, though, I'd say it doesn't matter what formatting is in use, and so you can drop some of the complexity.

@wpietri -- I think most of the added complexity is the part where we check whether the handlers already exist (not the formatting). I think we could leave that out and assume the user won't call configure_logging twice. In that case, we always add a handler, and things simplify quite a bit.

I do think now that we're not clearing all handlers, applying the formatter to all is good since if there's some third party library that adds something, it would be annoying for there to be two different formats showing up in GCP logs.

What do you think?

e: took a crack at the simplified version in the latest commit. Main difference versus prior is getting rid of the clear + the formatting all handlers.

[wpietri]

@wpietri -- I think most of the added complexity is the part where we check whether the handlers already exist (not the formatting). I think we could leave that out and assume the user won't call configure_logging twice. In that case, we always add a handler, and things simplify quite a bit.

Yeah, I'm fine with assuming that.

I do think now that we're not clearing all handlers, applying the formatter to all is good since if there's some third party library that adds something, it would be annoying for there to be two different formats showing up in GCP logs.

What do you think?

As a general rule I think we should build for known problems rather than speculative problems. I haven't seen this happen, and most libraries respect the existing logging. If it does happen, we can solve it then. But if you have seen it happen, then I'm fine with it as long as you have a test for the use case.