[WIP] Attachable Mixin

[ericspod]

Fixes #874 (partially) and PR 644 (partially)

Description:

This adds a new mixin Attachable for defining a dictionary relating events to (callable, args, kwargs) tuples. The generic attach method will add these as event handlers to the given Engine object. This should act as a generalization of other attach methods that currently exist. The advantage is that internal code or external users can cause a class inheriting this mixin to be attached to an engine with arbitrarily-defined handlers for events and are not limited to the ones pre-defined by the given attach methods.

There are a few problems with this approach however. The Timer handler still has to have its own attach method since the given argument event names may overlap and the Attachable currently can only associate one handler with an event at a time. The Metric and MetricsLambda classes are not as clean as hoped for since the latter relies on partially attaching metric objects to the engine. MetricsLambda instances only attach an EPOCH_COMPLETED handler and not the others, they are considered attached to the engine at ths point and the is_attached method checks for only this. Child metric objects do not attach EPOCH_COMPLETED but do for other events, these are considered not attached. The check for attachedness for the metrics thus has to take this into consideration, so currently this PR doesn't remove as many overridden methods in Metric or MetricsLambda as I'd have liked.

Is this a useful extension, as implemented or just the concept behind the mixin? Can we tweak it to improve the code quality and functionality?

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)

[sdesrozis]

Tests should be added about Attachable.

[sdesrozis]

Why do you remove event if handler is None ?

[ericspod]

This was a way of combining the operation of adding or removing a handler into one method. It might be better to introduce a remove_handler_mapping method and removal code there, or change line 45 to elif event_name in self.attach_map so that the method silently does nothing when event_name is for an event not attached yet. This could be useful if handler is an argument in the calling routine which is None by default indicating no handler for a particular event, it lets you define a do-nothing default in that case.

[sdesrozis]

The argument engine should be typed as Engine, shouldn’t it?

[ericspod]

The problem is that I would have to import Engine from its module, since that module imports this one this creates a cyclic dependency between them. Engine could be imported within the method and type check done there.

[sdesrozis]

Related to comment above. IMO name shouldn’t be None.

[ericspod]

This was to maintain compatibility with the way MetricsLambda attaches only to EPOCH_COMPLETED and checks for partially attached metric objects. It checks that internal metric objects are attached to particular events only. Only the top level object should attach to EPOCH_COMPLETED and all others attached partially. This was all based on my understanding of how this mechanism worked.

[sdesrozis]

Why do not use Attachable here ?

[ericspod]

This was again to do with the way the MetricsLambda class worked, the partial attaching mechanism is what is supported by this way of doing things.

[sdesrozis]

Related to above comment.

[sdesrozis]

I really enjoy this PR. Although we have to take care because Attachable concept is critical. We need test and check if it works with filtered events.

[vfdev-5]

Thanks for the PR @ericspod !
I agree that there are cases where Attachable can not cover perfectly current code and factorize without overhead. Maybe, at first we can try to factorize logger's code : https://github.com/pytorch/ignite/blob/master/ignite/contrib/handlers/base_logger.py#L152 ?

I agree with @sdesrozis this part is rather critical and we need to check very carefully

[ericspod]

I've read through @sdesrozis 's comments which are quite useful and point out some of the issues with my changes. A lot of this I think is to do with the way MetricsLambda does its partial attachment to handle the fact that it wants sub-metrics to calculate values but not compute the final results which it does itself. With any tree structure you have to consider what the behavioural differences are between nodes which are internal versus leaves, if they are doing some function other than storing data it's useful that they should be aware of their tree position. Perhaps we should change Metric to assist MetricsLambda and simplify its update mechanism to be something a little cleaner.

[sdesrozis]

@ericspod I agree with you. MetricLambda is a complex part of general Metric. First, as you mentioned, partial attachment is very specific. You pointed out the second point, graph representation is not easy to handle. Btw Attachable seems to be the good concept. If you have ideas to improve the whole design, please feel free to suggest 😊

As @vfdev-5 suggested, refactor first logger could be a good first step not so hard. And it could help to think about Metric.

@ericspod what do you think?

[ericspod]

The simple refactor for BaseLogger is to inherit from Attachable and change the attach method to use the inherited one as well as a change the attachment map beforehand:

class BaseLogger(Attachable, metaclass=ABCMeta):
    def attach(self, engine, log_handler, event_name):
        """[omitted]
        """
        name = event_name

        if name not in State.event_to_attr:
            raise RuntimeError("Unknown event name '{}'".format(name))

        self.set_handler_mapping(event_name, log_handler)
        super().attach(engine)

This doesn't provide much advantage, it also doesn't allow attaching multiple handlers for the same event but on different engines, so attaching the logger to the trainer and evaluator now is a problem. Attachable made more sense for the Handler classes which get associated with one engine at a time and usually only one method per event, maybe it doesn't make sense here?

Alternatively we could refactor BaseLogger to change the semantics a little bit by removing the attach method and the Engine argument from the attach_output_handler and attach_opt_params_handler methods. After these are called the inherited attach method would then be called to actually perform the attachment:

class BaseLogger(Attachable, metaclass=ABCMeta):
...
    def attach_output_handler(self, event_name: str, *args: Any, **kwargs: Mapping):
        if event_name not in State.event_to_attr:
            raise RuntimeError("Unknown event name '{}'".format(event_name))
            
        self.set_handler_mapping(event_name,self._create_output_handler(*args, **kwargs))

    def attach_opt_params_handler(self, event_name: str, *args: Any, **kwargs: Mapping):
        if event_name not in State.event_to_attr:
            raise RuntimeError("Unknown event name '{}'".format(event_name))
            
        self.set_handler_mapping(event_name,self._create_opt_params_handler(*args, **kwargs))

Attaching to an Engine is one extra method call to attach

vd_logger = VisdomLogger()

# Attach the logger to the trainer to log training loss at each iteration
vd_logger.attach_output_handler(
	event_name=Events.ITERATION_COMPLETED,
	tag="training",
	output_transform=lambda loss: {"loss": loss}
)

vd_logger.attach(trainer)

This is more in line with my use case for Attachable but is now a change for all the loggers and still doesn't allow a logger to be attached to multiple engine objects.

[vfdev-5]

@ericspod I agree refactoring BaseLogger like that, does not provide any advantage...

Removing engine from attach_output_handler and attach_opt_params_handler, IMO, will brake existing attach api idea is to attach to an engine...

Anyway, thanks for working on that, Eric !

[sdesrozis]

@ericspod Thanks for this work!

Did you try to use Attachable not by inheritance but by composition to have multiple attachable objects in an instance ? Maybe it could help here.

[ericspod]

@sdesrozis We could have an Attachable for each engine to attach to, this would address one issue, but at this point it's perhaps adding more complex code to little advantage.

[sdesrozis]

@ericspod I thought about this PR and I didn't find a good way to include it atm :( That does not mean that it won't be included but it's more complicated than expected :( The hard point is attachment is a key component of ignite. We absolutely must take all the necessary precautions not to break anything.

[ericspod]

@sdesrozis I agree this does require more thought still. The Attachable mixin in some form could be added now and used later where appropriate.

[sdesrozis]

@sdesrozis I agree this does require more thought still. The Attachable mixin in some form could be added now and used later where appropriate.

Personally I don't really like the idea of ​​incorporating uncovered code.

[ericspod]

@sdesrozis Fair enough, this PR is something to revisit in the future then. We could address #874 with less general code and merge PR 644 which should be backwards-compatible with a few tweaks.

[sdesrozis]

@ericspod Concerning #874, we just merged the PR #979 with MetricUsage :) I think it covers #644. Could you have a look and give your feedback ?

Thanks for your help !

[ericspod]

It's a different approach but it solves the problems presented in the issue and PR. It looks good and can make the metrics more flexible for different use cases. I would have used an inheritance approach versus a compositional one but it's pretty clean as implemented. If one calls Metric.attach with one usage value the caller has to be sure that Metric.detach is called with same to prevent confusion I suppose.

One suggestion I would make though is have a USAGE_NAME member of EpochWise set to "epoch_wise" and use that in place of the string literals, and similarly BatchWise.USAGE_NAME assigned "batch_wise". I think it's a good idea to have string identifiers stored somewhere users can refer to, this avoids having "magic" literals in code.

[vfdev-5]

One suggestion I would make though is have a USAGE_NAME member of EpochWise set to "epoch_wise" and use that in place of the string literals, and similarly BatchWise.USAGE_NAME assigned "batch_wise". I think it's a good idea to have string identifiers stored somewhere users can refer to, this avoids having "magic" literals in code.

Very good point, thanks @ericspod !