-
-
Notifications
You must be signed in to change notification settings - Fork 604
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Attachable Mixin #1018
[WIP] Attachable Mixin #1018
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests should be added about Attachable.
ignite/base/mixins.py
Outdated
yield event, method | ||
|
||
def set_handler_mapping(self, event_name: Any, handler: Callable, *args, **kwargs) -> None: | ||
if handler is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you remove event if handler is None ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ignite/base/mixins.py
Outdated
else: | ||
del self.attach_map[event_name] | ||
|
||
def attach(self, engine: Any) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The argument engine should be typed as Engine, shouldn’t it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ignite/metrics/metric.py
Outdated
@@ -148,7 +155,7 @@ def completed(self, engine: Engine, name: str) -> None: | |||
|
|||
engine.state.metrics[name] = result | |||
|
|||
def attach(self, engine: Engine, name: str) -> None: | |||
def attach(self, engine: Engine, name: Optional[str] = None) -> None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to comment above. IMO name shouldn’t be None.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ignite/metrics/metric.py
Outdated
engine.add_event_handler(Events.EPOCH_STARTED, self.started) | ||
if not engine.has_event_handler(self.iteration_completed, Events.ITERATION_COMPLETED): | ||
engine.add_event_handler(Events.ITERATION_COMPLETED, self.iteration_completed) | ||
if name is not None: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do not use Attachable here ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
ignite/metrics/metric.py
Outdated
if engine.has_event_handler(self.iteration_completed, Events.ITERATION_COMPLETED): | ||
engine.remove_event_handler(self.iteration_completed, Events.ITERATION_COMPLETED) | ||
|
||
super().detach(engine) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to above comment.
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. |
Thanks for the PR @ericspod ! I agree with @sdesrozis this part is rather critical and we need to check very carefully |
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 |
@ericspod I agree with you. As @vfdev-5 suggested, refactor first @ericspod what do you think? |
The simple refactor for 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. Alternatively we could refactor 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 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 |
@ericspod I agree refactoring Removing engine from Anyway, thanks for working on that, Eric ! |
@ericspod Thanks for this work! Did you try to use |
@sdesrozis We could have an |
@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. |
@sdesrozis I agree this does require more thought still. The |
Personally I don't really like the idea of incorporating uncovered code. |
@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. |
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 One suggestion I would make though is have a |
Very good point, thanks @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 genericattach
method will add these as event handlers to the givenEngine
object. This should act as a generalization of otherattach
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 givenattach
methods.There are a few problems with this approach however. The
Timer
handler still has to have its ownattach
method since the given argument event names may overlap and theAttachable
currently can only associate one handler with an event at a time. TheMetric
andMetricsLambda
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 theis_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 inMetric
orMetricsLambda
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: