-
-
Notifications
You must be signed in to change notification settings - Fork 656
Use events list for logger handlers #1544
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
Conversation
ydcjeff
left a comment
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.
LGTM, Thank you!
| :class:`~ignite.engine.RemovableEventHandle`, which can be used to remove the handler. | ||
| """ | ||
| name = event_name | ||
| if isinstance(event_name, EventsList): |
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.
I think it would be better to use return engine.add_event_handler(event_name, log_handler, self, event_name) for any type of events. Below for loop if considered without check if name not in State.event_to_attr is exactly what is done by Engine.add_event_handler.
I think we can simply generalize event checking like
event_to_check = event_name if isinstance(event_name, EventsList) else [event_name, ]
for name in event_to_check:
if name not in State.event_to_attr:
raise RuntimeError(f"Unknown event name '{name}'")
return engine.add_event_handler(event_name, log_handler, self, event_name)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.
That does not work.
The last argument of the method add_event_handler is a list of events. Internally in engine, there is a loop where the handler is attached at every event in the list. But the list is forwarded to each handler while just each event should be.
If there was no argument to the method, it would work.
Thoughts ?
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.
Yes, you are right. In this case, my suggestion is not valid. OK, let's do it as you proposed.
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.
Thanks for the PR @sdesrozis !
I let you merge it.
Fixes #1543
Description:
Allow
EventsListinBaseLoggerattachment. The key point is to attach each event independently as it is done inEngine.Check list: