Simplify BaseLogger attach APIs

[erip]

Fixes #980

Description: Adds new interface, docstring, and implementations for simplified base_logger API.

Check list:

• New tests are added (if a new feature is added)
• New doc strings: description and/or example code are in RST format
• Documentation is updated (if required)

[erip]

I think the failure is unrelated. I still need to write some tests, though.

[sdesrozis]

@erip Thanks :)

The file ignite/contrib/engines/common.py should be refactored to avoid import all loggers module (see setup_any_logging function).

What do you think to add methods for BaseWeightsScalarHandler and BaseWeightsHistHandler too ?

[erip]

Thanks for the comment @sdesrozis. I was thinking about the setup_any_logger code while doing this and I this it's possible to clean it up. I'm wondering if we should deprecate it or just completely remove it. It's technically part of the public API, but doesn't really serve any use outside of a private method... WDYT?

[sdesrozis]

Thanks for the comment @sdesrozis. I was thinking about the setup_any_logger code while doing this and I this it's possible to clean it up. I'm wondering if we should deprecate it or just completely remove it. It's technically part of the public API, but doesn't really serve any use outside of a private method... WDYT?

I suppose setup_any_logger is private. If it’s useless, we could remove it. We should just provide the setup of concrete loggers on public API. I don’t think it’s worth depreciating.

@vfdev-5 you agree ?

[vfdev-5]

Technically setup_any_logger is in v0.3.0 as a public API method (but, yes I agree that it is more private than public and not documented). So, technically we should not just remove it without any suggestions on how to use new API. At least we can raise an exception with appropriate message.

[vfdev-5]

ERROR OF SDESROZIS! USED MY PHONE AND EDIT THE POST :( Sorry :( The code is correct - sdesrozis

class BaseOutputHandler:

    def __call__(self, x):
        print("BaseOutputHandler", x)


class Engine:
    def __init__(self):
        self.handlers = []

    def __call__(self, x):
        for h in self.handlers:
            h(x)


class BaseLogger:
    output_handler_cls = BaseOutputHandler

    def attach_output(self, e):
        e.handlers.append(self.output_handler_cls())


class TbOutputHandler:
    def __call__(self, x):
        print("TbOutputHandler", x ** 2)


class TbLogger(BaseLogger):
    output_handler_cls = TbOutputHandler


engine = Engine()

BaseLogger().attach_output(engine)
TbLogger().attach_output(engine)

engine(2)

> python test.py
> BaseOutputHandler 2
> TbOutputHandler 4

[erip]

I was just thinking that approach myself. 😆 I think it's reasonable as long as we mark the Base...Handlers as ABCMeta to force them to be abstract.

[vfdev-5]

@erip please go ahead with that approach :)

[sdesrozis]

Here I would prefer use interface to provide contract to implement for developers of logger. It's more or less the same but more constrained.

What do you think about that ?

from abc import ABCMeta, abstractmethod


class BaseOutputHandler:

    def __call__(self, x):
        print("BaseOutputHandler", x)


class Engine:
    def __init__(self):
        self.handlers = []

    def __call__(self, x):
        for h in self.handlers:
            h(x)


class BaseLogger(metaclass=ABCMeta):

    # should not provide an implementation
    @abstractmethod
    def output_handler(self):
        pass

    def attach_output(self, e):
        e.handlers.append(self.output_handler())


class TbOutputHandler:
    def __call__(self, x):
        print("TbOutputHandler", x ** 2)


class TbLogger(BaseLogger):

    def output_handler(self):
        return TbOutputHandler()


engine = Engine()

# It don't work, BaseLogger is for inheritance
# BaseLogger().attach_output(engine)
TbLogger().attach_output(engine)

engine(2)

> python test.py
> TbOutputHandler 4

[erip]

@sdesrozis agreed. Have a look at this - hopefully this fits your model.

[sdesrozis]

I should have defined output_handler and output_opt_handler as abstract function

In BaseLogger

@abstract
def output_handler(self) -> BaseOutputHandler
    pass

And in concrete logger

class MyOutputHandler(BaseOutputHandler):
    ....

class MyLogger(BaseLogger):
    def output_handler(self) -> BaseOutputHandler
        return MyOutputHandler

I think my typehints is wrong : the good type is “class of BaseOutputHandler “

[vfdev-5]

@sdesrozis or even more proper solution would be to dispatch those creators

class BaseLogger:
    @abstractmethod
    def create_output_handler(self, *args, **kwargs) -> BaseOutputHandler
        pass

    def attach_output_handler(self, engine: Engine, event_name: str, *args: Any, **kwargs: Mapping):
        engine.add_event_handler(event_name, self.create_output_handler(*args, **kwargs))

class MyOutputHandler(BaseOutputHandler):
    ....

class MyLogger(BaseLogger):
    def create_output_handler(self, ) -> MyOutputHandler
        return MyOutputHandler(*args, **kwargs)

But a solution with class attributes is shorter :)

EDIT: I think this is what was suggested by @sisp here : #901 (comment)

[sdesrozis]

That looks like the perfect pattern here!

[sdesrozis]

Abstract method is a guideline for developer. I prefer that but @erip chooses at the end 😊 I talk and I do nothing 😅 (except edit messages 😱)

[vfdev-5]

@erip I added the code with implementation of _create_output_handler and _create_opt_param_handler, added docstrings... Attaching the way it is done, it should fix failed tests.
Remains helper methods setup_*_logging for other loggers.

[erip]

I think I've got the setup... helpers wrapped up. Over to @sdesrozis for review. 🥳

[sdesrozis]

Nice work 👍🏻

[vfdev-5]

I was thinking about those methods and loggers that need **kwargs to initialize.
Either we need to pass those things or directly work on the instance of such logger...

[erip]

I was thinking about this, too. I think the tricky thing is that the setup_X_logger by convention passes in default kwargs. Adding *logger_constructor_args and **logger_constructor_kwargs isn't easy because of that. I think there are some clever things we could possibly do... will try to draft a gist quickly.

[erip]

See here

[sdesrozis]

Like here ?

ignite/ignite/contrib/engines/common.py

Line 296 in 6624337

logger = VisdomLogger(**kwargs)

[erip]

I think that only works if it's a **kwargs-only constructor.

[sdesrozis]

I think that only works if it's a **kwargs-only constructor.

Can’t test, I just have my phone 😅

[erip]

Ah, I was wrong. As long as we can guarantee the user specifies all constructor args they can pass it in as a dict at the end like in VisdomLogger even for positional args.

[sdesrozis]

So let’s define defaults and that will be ok 👌🏻

Sorry I was too prompt to merge.

[erip]

Have #1015 out :-)

[vfdev-5]

Seems like I was late before it waas merged :(

[vfdev-5]

@erip or @sdesrozis could you add that in another PR please