feat: Stop event

[chrisk314]

Summary

This PR closes #55 by adding a SystemEvent abstract base class for system events and a StopEvent concrete class which signals termination of event driven models. The abstract base class Component defines an event handler function for the system StopEvent which is inherited by all child classes. On receiving the StopEvent, each component will now propagate a new StopEvent and then shutdown its IOController, ultimately resulting in termination of the Component.run loop.

To support the inheritence of the StopEvent handler behaviour in child Component classes, some changes have been made to the setup process for the IOController class used within Component classes: on defining a new Component child class, IOController input and output fields and events are now inherited from parent class IOControllers. Rather than simply checking if a Component child class defines an IOController instance, the presence of additional input or output fields or events, above and beyond those defined in the abstract base class Component is now checked.

Changes

• Adds child Event class SystemEvent abstract base class to be used for all plugboard builtin system events.
• Adds child SystemEvent class StopEvent used to signal termination of plugboard event driven models.
• Updates Component abstract base class to include a handler for StopEvent which triggers termination of component.
• Modifies the logic surrounding setup of IOControllers on declaration of Component child classes to support inheriting input and output fields and events in child Component classes.

[toby-coleman]

Had a look through here - added a few comments.

[chrisk314]

There appears to be an error occurring in the teardown of some tests related to the event loop being closed whilst trying to read data from a channel. Despite this the pipeline still passes... I will look into this and make sure everything is getting handled gracefully.

[chrisk314]

@toby-coleman I'm wondering about the changes just merged for fixing the Ray issues in #92. In particular the parts which skips Component.__init_subclass__ may well interfere with the approach to IOController inheritance introduced here. The tests are passing so either: this is not problematic somehow; or the tests aren't catching a potential issue there.

My IOController inheritance test doesn't cover the Ray case so perhaps if I set that to run on Ray it could expose a problem. Also, the StopEvent test is not currently running on Ray. I'll update that which would also hang without the IOController inheritance. Let's discuss tomorrow.

[toby-coleman]

@toby-coleman I'm wondering about the changes just merged for fixing the Ray issues in #92. In particular the parts which skips Component.__init_subclass__ may well interfere with the approach to IOController inheritance introduced here. The tests are passing so either: this is not problematic somehow; or the tests aren't catching a potential issue there.

My IOController inheritance test doesn't cover the Ray case so perhaps if I set that to run on Ray it could expose a problem. Also, the StopEvent test is not currently running on Ray. I'll update that which would also hang without the IOController inheritance. Let's discuss tomorrow.

Hmm yeah let's chat tomorrow. If there's any way you can avoid running that on Ray it would help. For example skipping hasattr(cls, "io") is fine because it will get run on the driver node before the process is run and is just a validation check.

[chrisk314]

@toby-coleman I've resolved the issue with IO inheritance on Ray following the approach we discussed. Also fixed up a couple of other issues with test cases and serialisation.

I've created a github issue to track the state of some gaps / incompatibilities in pubsub / event support. Mostly this affects models running in Ray. I believe the implementation before the stop event and pubsub changes also had limitations for correctly running models in Ray, in particular in the more general multi-host context. If you're happy to proceed on the basis of the known issues mentioned in #101 which we can address in future PRs then I believe this is ready for a final review and merge.

[toby-coleman]

@toby-coleman I've resolved the issue with IO inheritance on Ray following the approach we discussed. Also fixed up a couple of other issues with test cases and serialisation.

I've created a github issue to track the state of some gaps / incompatibilities in pubsub / event support. Mostly this affects models running in Ray. I believe the implementation before the stop event and pubsub changes also had limitations for correctly running models in Ray, in particular in the more general multi-host context. If you're happy to proceed on the basis of the known issues mentioned in #101 which we can address in future PRs then I believe this is ready for a final review and merge.

Sounds good to me - will have another look through the PR this evening.

[toby-coleman]

Ready to go, I think!