Make sure metric handlers are fired first

[beyondyoni]

Hi,

I noticed that since event handlers are invoked at the order of their insertion, there's a potential bug- if a metric handler is inserted after a TensorBoard handler, the metric value will not be ready at the time of writing.

While this has only minor effects (one epoch shift in the results), I think it can be easily solved by one of two approaches:

1. separating handlers list to two: one for metrics, and the other handler types.
2. More generic and scalable- allowing users to decide the order of execution by adding priority argument to add_event_handler (default would be that metric handlers are inserted with higher priority) and then sort the list when run() is invoked.

Let me know if you like any of the ideas, and I'd be happy to draft a PR

[vfdev-5]

@beyondyoni thanks for the ideas. IMO, the order of handlers is up to user to respect. So, if user adds at first the loggers and after that handlers providing the info, it is his/her resposibility.

However, I think adding some argument to setup a sort of priority of handlers can be an interesting enhancement ! For instance, I do not know how can we provide such option. If you would like to draft some ideas, please do not hesitate to draft a PR and we can reiterate over.

[beyondyoni]

I agree with your opinion of order of execution being the user's responsibility but I think the current situation isn't clear enough (I actually came across this bug because a co-worker of asked for my help to solve it).

I made a new push. Let's discuss it there

[vfdev-5]

Okay, I see. Well, let's try to fix this behaviour :)

I made a new push. Let's discuss it there

Could you please make a separate PR for this. Thanks

[vfdev-5]

@justusschock what do you think about this ?

[justusschock]

I think this is a general question. We should decide, whether we want priorities/sorting of handlers or not (but definitely not support both versions).

If we implement it, it should always be used, but this may also introduce unwanted behaviour if some handlers are for example added by an internal function with a different priority, this may also confuse the user, since he might expect some other behaviour.

I think, I slightly prefer the way including priorities, but this has to be carefully implemented.
On the implementation side, I would prefer to have the priority option not in the function adding the handler but as an attribute of the handler (comparable to the logging level in python's internal logging module).

[beyondyoni]

I think the current design implicitly supports priorities, and is, therefore, a source for (potential) confusion, so I'm glad to see you're leaning towards changing that.

If I understand correctly, your implementation approach requires introducing a new Handler class right? I think it's a good idea, as it will also improve areas in the code such as the following:

        for e in events:
            for _, h, _, _ in self._event_handlers[e]:
                if h == handler:
                    return True
        return False

[justusschock]

Yes it requires a new handler class but also some code that allows all current handler implementations to continue working by providing good defaults and wrapping possibilities