AlexandreDecan / sismic

Sismic Interactive Statechart Model Interpreter and Checker http://sismic.readthedocs.io/
GNU Lesser General Public License v3.0
140 stars 25 forks source link

Refactor of Bind mechanism to introduce concept of Bind_Interface #86

Closed trulede closed 5 years ago

trulede commented 5 years ago

I'm trying to reconcile the recent changes to Bind(), along with my Adapter class for Distributed Statecharts, and the previously mentioned idea of creating a Proxy for the existing Bind Mechanism. Out of this analysis came a few ideas, which when combined, change the nature of the Bind Mechanism.

This is a further extension of the recent changes with an intention to create a flexible and stable interface. What I would really like to do is make the concept of Binding two Statecharts generic and have the parameters of the Bind() determine how the Bind is realised. Possibly also, the Bind() could be delegated to a Class- i.e. bind this Statechart to the Interpreter using this Bind_Class.

This is no different to the latest implementation, just further along.

What I would like to suggest is to turn the Bind Mechanism into something which represents a more generic subscription model. An Object, which implements the abstract Bind_Interface(), is able to be bound to an Interpreter, and in the process subscribes to the various meta-data-interfaces. The subscription can either be manually specified within the call to Bind(), or, specified by a Helper Class which implements the Bind_Interface and provides the predefined set of Subscriptions.

As a short conceptual example:

class BindInterface():
    subscribes = []
    execute():
        pass
    @property
    final():
       return foo
    queue():
        pass

class PropertyBind(BindInterface):
    subscribes = ['Meta', 'Execute']
   # and so on ...

class LocalBind(BindInterface):
    subscribes = ['Internal']
   # and so on ...

it.bind(LocalBind(some_other_chart))
it.bind(some_other_chart_interpreter) #if the Interpreter class inherits LocalBind (by default), then this works too!
it.bind(RemoteBind(some_other_chart)) # the Remote bind should override the LocalBind IIRC
it.bind(PropertyBind(import_from_yaml('foo')) # simple Propertychart

There is more to it that that :wink: however at least that was one area which I can explain at least a bit. Much of the thinking behind this is that a bound Statechart and bound Property Statechart are actually the same thing, and so, by making the interface more generic, we get the chance to introduce new kinds of Binds ... which could be very interesting.

Further along, the actual implementation of the Bind (sending events, calling execute) could also be pushed into the HelperClass, allowing for even more possibilities. Then, the Interpreter need only call a method on each Bound Statechart, and have the Helper do the actual work. That might be the point where a remote Statechart could be bound ... i.e. where the transport/connectivity gets implemented.

In my distributed case, I might have:

it.bind(RedisBind(import_from_yaml('remote_foo'))
it.bind(PropertyBind(RedisBind(import_from_yaml('remote_foo'))) # perhaps even that will work, remote Property charts ...

I think it will be easier to implement than explain ... 😄 ... assuming things remain calm, I hope to spend some time this week on the concept. Again, there are several drivers here; distributed Statecharts, more generic and flexible bind() interface, and also looking at additional language support.

trulede commented 5 years ago

Enhancement, Refactoring

AlexandreDecan commented 5 years ago

I'm not sure I fully understand what's the point with this ;-) Currently, the bind mechanism already allows what you suggest:

I however see two points that are not directly covered by the current implementation:

The main drawback of the current implementation (at least to me) is the specific behaviour we have in bind when an Interpreter instance is provided. A naive solution could be to suggest users to bind another statechart using it.bind(other_interpreter.queue) instead of it.bind(other_interpreter) (or in other words, we remove the "magics" around Interpreter instances passed to bind), or to define a BoundListenerInterface with a listen method, and:

In that case, bind should simply distinguish between instances of BoundListenerInterface and other callables (I still want to support callables as well). However, (1) I'm not convinced that introducing more interfaces/abstraction layers is the best idea, (2) I don't like the special case around Interpreter instances we currently have, and (3) forcing users to explicitly pass other_interpreter.queue is a breaking change. So none of the three solutions (current implementation ; listener interface ; removing transparent support for Interpreter) currently convinces me...

AlexandreDecan commented 5 years ago

After a careful re-read of your initial message, I think we're on the same line :-) Let's confirm this, and then let's discuss on how we can (in a backwards compatible way) make the current binding mechanism even more generic without introducing too many abstraction layers if possible.

AlexandreDecan commented 5 years ago

Another thing to consider is whether internal events have to be notified "separately". I mean that the binding mechanism could be restricted to meta-events only, as internal events can be obtained based on the "event sent" meta-event. If we go for a more general binding mechanism, then we can implement the "traditional binding" for statecharts based on meta-events: when a meta-event eis received, and is "event sent", then create and queue Event(e.event.name, **e.event.data). If we go for a BoundListenerInterface, a SentEventListener could implement this behaviour and Interpreter could inherit from it.

AlexandreDecan commented 5 years ago

What about the following:

  1. Create a new class BoundListener that defines an abstract method listen. This method accepts MetaEvent instances, and returns nothing.
  2. Create concrete implementations of BoundListener:
    • A InternalEventListener(callable) that filters "event sent" meta-events, and calls given callable with Event(e.event.name, **e.event.data).
    • A MetaEventListener(callable) that calls given callable with all meta-events that are received.
    • A PropertyStatechartListener(statechart, *, interpreter_klass) that listens to meta-events, queues them to the interpreter created with interpreter_klass(statechart, clock=...) (as currently implemented), executes this interpreter and checks for final.
  3. Make Interpreter inherits from InternalEventListener initialized with SentEventListener.__init__(self.queue) or, another possibility, make Interpreter inherits from BoundListener and implement the behaviour of filtering/queuing internal events in its listen method (I still need to see if there are other ways).
  4. To preserve backwards compatibility, we cannot change the bind method which, by default, only listen for internal events. I suggest then to create a new bind_listener method that accepts either a BoundListener instance or a callable. If a callable is provided, it is wrapped around a MetaEventListener. This bind_listener method should explicitly state that it notifies for meta-events. This method could possibly have a internal_only=False parameter. If set, wrap the callable with InternalEventListener instead of MetaEventListener.
  5. Make bind(interpreter_or_callable) relies on bind_listener (if an interpreter is provided, it is a BoundListener instance so it's ok. If a callable is provided, to keep backwards compatibility, wrap it with InternalEventListener). The bind method will be deprecated
  6. Make bind_property_statechart(statechart, *, interpreter_klass) relies on bind_listener. Wrap given parameters with PropertyStatechartListener.
trulede commented 5 years ago

You are half way there! I think we can manage the backwards compatibility too.

Think more general. I want to Bind two Statecharts, and I would like the bind mechanism to be transparent to both the Interpreter and the Statechart - that is to say, represented by a third Object/Class, which sits in between. How it does not matter right now, could be inherited, mixedin or a parameter.

If we do that, then you can implement a Listener, Proxy, Adapter, PropertyChart, Validator with the Bind mechanism ... and anything else which comes to mind. Perhaps some of those are new concepts for you? But I think we can do something that works for each case, which will be interesting.

I've run out of time, but will get some more feedback tomorrow.

AlexandreDecan commented 5 years ago

I (think I) got it :)

But I would at least like to keep the (current) API simple for these two uses cases:

The bind_listener as proposed here above could be the "low-level interface" of the binding mechanism, and we can keep the current bind and bind_property_statechart methods as "high-level interface", wrappers around the low-level one. It will be easier to keep backwards compatibility as well. Perhaps the low-level interface could be as simple as Interpreter.register(callable) (instead of bind_listener to avoid confusion). I really would like to keep the API as simple as possible, while still allowing to have more advanced usages (that's partly the philosophy behind Sismic actually).

It is also compatible with your idea of having a third "in-between" class to orchestrate any kind of communication based on meta-events (including internal events, as they can be obtained from the corresponding meta-events). Then we can provide any kind of callable, e.g. listener, proxy, adapter, etc. to support the specific use-cases (internal events for other interpreters, internal events for some callable, meta-events for property statecharts, meta-events for some other monitoring mechanism, ...).

Am I right?

trulede commented 5 years ago

Yes, I would hope we could do this without API changes.

Let me think about the rest tomorrow :wink: But I think you have it, now need to sleep on it and see if I still have it tomorrow. 🤣

trulede commented 5 years ago

Yes, so the idea is to rework the Bind Mechanism into a class which sits between two Interpreters. That should then open up the Bind Mechanism to support specific uses-cases which are outside of the scope of the core Sismic implementation. And make the interface more stable for those outside use-cases.

I would propose to submit a pull request with my ideas on the implementation, and from there we see how to reconcile that into the Sismic codebase. My impression is that we should be able to extend the bind() interface to do this without breaking existing code, and keep the existing behavior of the core Sismic implementation.

AlexandreDecan commented 5 years ago

Could you elaborate a little bit (or provide a toy example) the "new class that sits between interpreters"?

AlexandreDecan commented 5 years ago

I pushed a new branch named "binding" (currently even with master). Consider PR against this branch. I hope to find some time today to work on this as well.

AlexandreDecan commented 5 years ago

Let's discuss in #88