refactor setting loggers

[janosh]

Summary

Closes #242. Turned into a little bit more of a refactor than I had intended but should make the logger more user accessible.

[janosh]

CI as well as intensive tests running locally pass. Should we add additional logger tests?

[ardunn]

@janosh thanks for the contribution!

What additional tests did you have in mind? we already have some in utils test_log but I'd be open to adding some more, provided they add some utility.

I will have to review this pr a bit more before merging but all looks good on first glance.

Also ill link #204 here as well since those issues might be related

[janosh]

Ah, I'd only seen test_LoggableMixin in tests/test_base.py. You're right, utils/tests/test_log.py covers much more. The only thing, I think, one could add is testing AMM's logging utils in conjunction with the classes that use them. I.e. actually call the loggers from MatPipe, etc. in a test. But that doesn't have to be in this PR.

[ardunn]

@janosh that is a good idea, I think that can be done relatively easy in test_pipeline!

Regarding this PR though, all seems good except the assert statement which is triggering codacy. If possible I'm trying to avoid isinstance type checking as much as possible (there are still a few sections of code in AMM which use it unfortunately) but if we do have to use it, we should just use an if block and raise TypeError afterwards.

[janosh]

If possible I'm trying to avoid isinstance type checking as much as possible...

That's interesting. I'd be curious to know why.

Also, if we use type instead, note that it doesn't cover subclasses. I.e. if someone created their own logger class by subclassing logging.Logger, that would raise a (probably) unwarranted error.

Just to be clear, you're suggesting something like:

import logging

if type(new_logger) != logging.Logger:
    raise TypeError("Received invalid logger")

[ardunn]

@janosh no I am not suggesting that. sorry I should have been clearer.

I mean no type checking at all if we can avoid it (sometimes we can't). My order of preferences is type < isinstance < no type checking. The most Pythonic thing to do is treat the object by the methods it implements (i.e., if it walks like a logger and talks like a logger, its a logger) rather than include or exclude it based on its class or parent class(es).

So for this time it's ok to isinstance check if there is no way around it:

if not isinstance(new_logger, logging.Logger):
    raise TypeError("Logger argument must be a logging.Logger object or subclass!")

But in general type checking is not good practice (I am still trying to undo many isinstance related cryptic errors, including some in this very project). In this case though, not a big deal :)

[ardunn]

I'm just going to merge for now

[janosh]

Ah, now I see. I would have certainly happy to leave out the type checking completely. Sorry for my confusion.