Skip to content

Conversation

@sdesrozis
Copy link
Contributor

@sdesrozis sdesrozis commented Mar 31, 2020

Fixes #710

Description:

Add an event list class to pack events which could be added with operator | on Events and CallableEventWithFilter

User code is

    def fn(e, b):
        pass

    trainer = Engine(fn)

    @trainer.on(Events.ITERATION_STARTED(once=1) | Events.ITERATION_STARTED(every=3) | Events.COMPLETED)
    def execute_some_handler(engine):
        print(engine.state.iteration)

    trainer.run(range(3), max_epochs=5)

TODO : improve doc

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)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 31, 2020

@sdesrozis I'm not a fan of introducing another public method for that.
Idea of returning RemovableEventHandler is that it can be used as a context manager to remove attached handlers on exit of the context manager. I think we need to keep single method add_event_handler and return something such that we ensure that it could be used as RemovableEventHandler... What do you think ?

@sdesrozis
Copy link
Contributor Author

sdesrozis commented Mar 31, 2020

Now add_event_handler returns RemovableEventHandler compatible with EventsList.

@vfdev-5 Is it better ?

TODO doc

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Mar 31, 2020

Sorry, my bad, the name should remain as previously as RemovableEventHandle and not RemovableEventHandLER.
Yes, now, it looks good for me. Please, add a test with RemovableEventHandle for event list.

TODO doc

  • dosctring of Engine.add_event_handler
  • README.md in "Power of Events & Handlers"

@sdesrozis
Copy link
Contributor Author

Last missing point is

  • README.md in "Power of Events & Handlers"

@sdesrozis
Copy link
Contributor Author

@vfdev-5 you can comment :)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 1, 2020

@sdesrozis looks good to me. For perfect fit, I would put added section just after "Built-in events filtering"...

Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sdesrozis Thanks a lot for the PR !

PS: I changed the order of sections by myself as it can be done very quickly :)

@vfdev-5
Copy link
Collaborator

vfdev-5 commented Apr 1, 2020

@vfdev-5 vfdev-5 merged commit 6a2460e into pytorch:master Apr 1, 2020
@sdesrozis sdesrozis deleted the issue-710 branch April 1, 2020 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Handler Events as flags

2 participants