Skip to content
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

Rationals for the commit "Huge blob commit to drive reactor forward" #34

Open
jbjuin opened this issue Jan 12, 2022 · 14 comments
Open

Rationals for the commit "Huge blob commit to drive reactor forward" #34

jbjuin opened this issue Jan 12, 2022 · 14 comments

Comments

@jbjuin
Copy link
Collaborator

jbjuin commented Jan 12, 2022

Hi @edelvalle, glad to see that you came back to this project !

Do you have any rationals about your complete rewrite of reactor ?

  • transpilation in python,
  • pydantic serialization,
  • more...

I remember that you wanted to avoid external dependencies and pydantic is a huge one.
Is this for speed reason ?
Cheers,

@edelvalle
Copy link
Owner

Hey @jbjuin, nice to have you here!

The answer is "it's complicated"

I had parked reactor for a year or so, not giving it too much attention and focused on experimenting with HTMX, look at djhtmx. It is kind of the same but with HTMX.

I started djhtmx from reactor, is basically a fork using HTMX, and had been using it for more than a year in production (that's why this both projects don't have tests, because the tests are tests of my production code, if I break something on this projects I will notice immediately).
Now what's happening is that a year of learning in djhtmx is coming back to reactor.

During this year I noticed that:

  • using Pydantic offered me a standard way of de/serializing the components, that was documented somewhere else. Allowed me to serialize also more complex times and convert them back correctly.- also Pydantic offers me some level of runtime type checking for function calls and proper parsing of some types out of the box.

Other changes were:- I had to separate the initialization state from the state of the component, because I noticed they are not the same, and learned this from ReactJS and MithrilJS.

  • Had to move the transpilation to python because calculating and applying diffs had an overhead when the client had transpilation of the event binding DSL to JS.
  • I needed caching in some components so added a way for you to generate a caching key and retrieve the rendered component without touching the database in most cases.
  • Also in the last year I bumped up my JavaScript game and went from CoffeeScript to pure JS.- Learned (from your code) that I could de/serialize model instances (thanks)
  • Made a more transparent and simple model mutation broadcasting, where I send you the model instance and some description of what happened, so you don't have to reload it from the database.
    Basically these are the reasons you are seeing all these changes in reactor.

Do you have any ideas, opinions, questions? Just let me know...

Have a great start of the year!
Eddy

@jbjuin
Copy link
Collaborator Author

jbjuin commented Jan 13, 2022

Hi @edelvalle thanks for all those explanations.

I didn't try djhtmx although I should since it looks great as well.

I use reactor in some tools but on an old version: the one before the commit changing the syntax for the event handlers (when you removed the prefix receive_ and switched to self._subscribe instead of self.subscribe).

I don't really know what to do about that. I built a bunch of tooling around it and I may continue with it and make a fork from the version I use. I never had any problems with serialization (nor JS transpilation overhead being a problem) or the other needs that you mention: cache and sending the instance in the broadcast. This being said I do have questions about this version:

  • I would be curious to see how you implemented the cache and how to invalidate it ? Do you have examples of this ?
  • Also I'm not sure that I understand how you switched to "declarative" subscriptions ? And related to this: how do you receive the instance sent in the broadcast ? Since in the previous version the broadcast just triggered a complete re-rendering of subscribed components. Do you have an example code of the new subscription mechanism ?
  • Do you plan to work on it again ? Do you still use it in your company ?

The thing I want to do is test reactor under some parametrized load (number of sessions basically):

  • estimate of the memory consumption since the app state (or a part of it) is kept on the server, how does that scale with sessions ?
  • How does channels behave with 10/100 web sockets opened, etc ?
  • Do you have some results on that point ?

I've planned some time in the coming weeks to clean my code and publish it so that you can have a look.

Anyway it's a joy to develop with reactor. You did a great work on that !

@edelvalle
Copy link
Owner

edelvalle commented Jan 14, 2022

I don't really know what to do about that

I'm so sorry to hear this, but... I think I took some decisions were not the best and this are amendments.

I built a bunch of tooling around it and I may continue with it and make a fork from the version I use.

Seriously? I would had never done such drastic changes if I knew this library was actually being used by other people other than me. I assumed unicorn is way more popular, well documented, tested and so on... this library release it here as a byproduct of my actual work.

Regarding your questions:

I would be curious to see how you implemented the cache and how to invalidate it ? Do you have examples of this ?

Before calling render, it will check if you have _cache_key set and will try to retrieve the key from the cache, refresh the cache if configured to do so and use that HTML as the rendered version. There is no way to invalidate the cache for a specific key, just generate a different one. Also the cache expires at some point. In a component this are the parameters that control the cache configuration and key:

    # Cache: the render of the component can be cached if you define a cache key
    _cache_key: str = None
    # expiration time of the cache
    _cache_time = 300
    # if True will refresh the cache on each render
    _cache_touch = True

Also I'm not sure that I understand how you switched to "declarative" subscriptions?

In my mental model is easier, I just state what I want to be subscribed to without doing it in an imperative way.
Because then you have also to keep track of what you need to unsubscribe from, so is better if the library does that for you and just say what you are interested in.

And related to this: how do you receive the instance sent in the broadcast ? Since in the previous version the broadcast just triggered a complete re-rendering of subscribed components. Do you have an example code of the new subscription mechanism ?

Before I the broadcast would be something like model.1234 changed, and you go and figure out what happened, for that you have to touch the database to check what changed, or if it was deleted or what.

Now the change comes the channel name you subscribed to model.1234, the instance of the model (so if you are using it in your state you can replace it with the incoming one without touching the database), and a description of the action that happened:

class Action:
    # Model action
    UPDATED = "UPDATED"
    DELETED = "DELETED"
    CREATED = "CREATED"

    # M2M actions
    ADDED = "ADDED"
    REMOVED = "REMOVED"
    CLEARED = "CLEARED"

If you have a TODO list and you have a component per item in the list you can do:

class ItemComponent(Component):
    item = Model[Item]
    
    @property
    def subscriptions(self):
        return f"item.{self.item.id}"

    def mutation(self, channel, instance, action):
        self.item = instance
        if action == Action.DELETED:
            self.destroy()

If you are just rendering the self.item.name for example you could have a key for that as:

@property
def _cache_key(self):
    return self.item.name

Do you plan to work on it again ? Do you still use it in your company ?

We are evaluating to use this, why? because we could send cross-components signal to update components that do not have a direct relation.

The thing I want to do is test reactor under some parametrized load

Had not done that, would be very interesting!

Anyway it's a joy to develop with reactor. You did a great work on that !

Than you very much and sorry for the disturbance I caused... 😸

@jbjuin
Copy link
Collaborator Author

jbjuin commented Jan 14, 2022

I don't really know what to do about that

I'm so sorry to hear this, but... I think I took some decisions were not the best and this are amendments.

Ha ha don't worry ! We are still evaluating reactor for a coming project and we may switch to this new version. Maybe we could integrate some of the modifications we did. I will have to read this new version more deeply.

I built a bunch of tooling around it and I may continue with it and make a fork from the version I use.

Seriously? I would had never done such drastic changes if I knew this library was actually being used by other people other than me. I assumed unicorn is way more popular, well documented, tested and so on... this library release it here as a byproduct of my actual work.

Well Unicorn looks good as well but with more magic (my feeling). I also need "backend push" features that seems to lack in Unicorn .

The code of reactor is small, simple and even though it's not fully tested it is small enough to be easily understood and fixed if needed. Plus, I know it quite well now :)

Regarding your questions:

I would be curious to see how you implemented the cache and how to invalidate it ? Do you have examples of this ?

Before calling render, it will check if you have _cache_key set and will try to retrieve the key from the cache, refresh the cache if configured to do so and use that HTML as the rendered version. There is no way to invalidate the cache for a specific key, just generate a different one. Also the cache expires at some point. In a component this are the parameters that control the cache configuration and key:

    # Cache: the render of the component can be cached if you define a cache key
    _cache_key: str = None
    # expiration time of the cache
    _cache_time = 300
    # if True will refresh the cache on each render
    _cache_touch = True

I see. This may be useful indeed with some "flat" data where you can easily know when something changed or not, so that you can derive the key from that "something".

Also I'm not sure that I understand how you switched to "declarative" subscriptions?

In my mental model is easier, I just state what I want to be subscribed to without doing it in an imperative way. Because then you have also to keep track of what you need to unsubscribe from, so is better if the library does that for you and just say what you are interested in.

And related to this: how do you receive the instance sent in the broadcast ? Since in the previous version the broadcast just triggered a complete re-rendering of subscribed components. Do you have an example code of the new subscription mechanism ?

Before I the broadcast would be something like model.1234 changed, and you go and figure out what happened, for that you have to touch the database to check what changed, or if it was deleted or what.

Now the change comes the channel name you subscribed to model.1234, the instance of the model (so if you are using it in your state you can replace it with the incoming one without touching the database), and a description of the action that happened:

class Action:
    # Model action
    UPDATED = "UPDATED"
    DELETED = "DELETED"
    CREATED = "CREATED"

    # M2M actions
    ADDED = "ADDED"
    REMOVED = "REMOVED"
    CLEARED = "CLEARED"

If you have a TODO list and you have a component per item in the list you can do:

class ItemComponent(Component):
    item = Model[Item]
    
    @property
    def subscriptions(self):
        return f"item.{self.item.id}"

    def mutation(self, channel, instance, action):
        self.item = instance
        if action == Action.DELETED:
            self.destroy()

OK so you introduced a "mutation" handler. That is interesting as well. I ended up using a different approach with my "store" companion app, re-using the classical events handlers (receive_blabla) and sending message to components so that they can receive the new value and update when the store state changed.

If you are just rendering the self.item.name for example you could have a key for that as:

@property
def _cache_key(self):
    return self.item.name

Ok so this is the caching mechanism ! Great... this is how I understood it: the key is the name so you do not hit the DB except if the name changes. Cool !

Do you plan to work on it again ? Do you still use it in your company ?

We are evaluating to use this, why? because we could send cross-components signal to update components that do not have a direct relation.

Yeah this was one of my need as well (hence the reactor "store" companion app I ended writing). So with this mechanism in place we don't need it anymore ! Great !

The thing I want to do is test reactor under some parametrized load

Had not done that, would be very interesting!

Yes. I'm still not sure how to do that properly since I will have to use some headless browser client. This is not a small 1 hour task.

Anyway it's a joy to develop with reactor. You did a great work on that !

Than you very much and sorry for the disturbance I caused... smile_cat

Again it was not against your choices, this work is truly amazing: simple and efficient. Love it !
So I'm wondering now how we could coordinate the work effort ?

  • The first thing I will do is pull the new version and understand it more deeply.
  • I will test your new mechanisms: declarative subscriptions & cache.

If it fits our needs I will

  • try to port the Jinja2 companion app to it (this is an absolute requirement for us) and publish it,
  • work on stress testing so that we can confidently know how reactor (channels basically) react to high load and quantify the number of session versus load relation.

When this is done I will probably propose a pull request to go back to the receive_ handler syntax :) (I find it more explicit and less confusing when one look at its component).

Any other idea to coordinate efforts on that ?

Thanks again for your explanations and I hope you will keep working on that !
Cheers,

Edit: just saw that the README is updated with those new mechanism ! Sorry for the questions, last time I checked it was not updated...

@edelvalle
Copy link
Owner

edelvalle commented Jan 15, 2022

The code of reactor is small, simple and even though it's not fully tested it is small enough to be easily understood and fixed if needed. Plus, I know it quite well now :)

I try to keep the levels of magic to the minimum, just magic to make things comfortable but no more.

Yeah this was one of my need as well (hence the reactor "store" companion app I ended writing). So with this mechanism in place we don't need it anymore ! Great !

I will probably add a mechanism to send events cross components, and will look something like a pubsub pattern. Where a component subscribes to a channel and other publishes events with arguments and stuff and the first components receives the whole thing and decides what to do then.

Again it was not against your choices, this work is truly amazing: simple and efficient. Love it !

I'm just lazy

  • try to port the Jinja2 companion app to it (this is an absolute requirement for us) and publish it

What!? Do you have a jinja2 companion app? I'm looking forward to look into that and make the template system configurable and flexible, so you can tell the whole reactor or a single component to use Jinja. Nice!

When this is done I will probably propose a pull request to go back to the receive_ handler syntax :) (I find it more explicit and less confusing when one look at its component).

Would you mind having something shorter like recv_ or any other suggestion? I will call this "the receiver prefix".

To do this this things would have to be modified:

  • Modify ComponentRepository.dispatch_event to prepend the receiver prefix to the event name.
  • Modify Component.__init_subclass__, to just wrap in validate_arguments, attributes that start with the receiver prefix, to just type check and convert incoming calls and avoid checking internal calls of the component.
  • Modify ComponentMeta.__get_context to exclude the event handlers from the rendering context.
  • Modify the on template tag to prefix the event handler with the receiver prefix.

I can do it for you if you tell me what prefix do you like...

Regarding stress testing and so on, do you wanna have a call about that? Check my email on the commits and send me an email and we can talk any time. I'm in Berlin, Germany (CET zone)

@jbjuin
Copy link
Collaborator Author

jbjuin commented Jan 20, 2022

Hi @edelvalle,

I just started trying reactor 3 and here is a first remark about the new subscription system.

In reactor 2 we had the broadcast("progressing") command that could be used in conjunction with the self.subscribe("progressing") to send arbitrary events between components (or from a Celery Task for instance).

  • The drawback was that there was no payload with those events. The subscribing component was re-rendered and it had to fetch data again, etc.
  • The advantage was that it was not tied to django models.

In reactor 3, broadcast is now tied to models.

  • The drawback is that we cannot send arbitrary signal to update some component.
  • The advantage is that some data are passed with it (instance and action) and there is a handler on the receiving component to handle properly the incoming data.

My questions & remarks are:

  • is there a reason to abandon a "generic" broadcast system not tied to django models ?
  • would it be possible to have a generic broadcast("my-message", {"msg": "hello world"}) so that components subscribing to "my-message" will receive the payload in the "mutation" handler ?

This generic broadcasting system would not be tied to django models and would answer your need of a "pub/sub" pattern. The "model actions broadcasting" system could use it to send the instance & action.

Does it makes sense ? What do you think ?

I continue my exploration of the new features and will report my remarks here.

About you previous questions:

  • yes I do have a very simple Jinja companion app that just redefine the various template tags as Jinja filters & functions. And a jinja environment to plug in the templates settings,
  • ok for recv_ for the receiver prefix. Sounds great.

Cheers !

@edelvalle
Copy link
Owner

Hi,

I wanted to add this generic broadcasting and have a way for the component to receive those message. I will make a PR and you check my proposal, and I will ask you several stuff in that PR so we design in code and not talking on the vacuum.

Then I will do the changes for the recv_ prefix.

Have a great time and thanks a lot!

@edelvalle
Copy link
Owner

edelvalle commented Jan 20, 2022

@jbjuin check the arbitrary broadcast #35

@jbjuin
Copy link
Collaborator Author

jbjuin commented Jan 21, 2022

Hi @edelvalle
I just tested a POC for jinja integration and it works quite well. It consists in a single file jinja2_env.py which redefines the template tags and custom filters of reactor.

Do you prefer to integrate it in reactor 3 or make it available through a companion app ?

@edelvalle
Copy link
Owner

Nice, let's do it as part of the app.
Do you wanna show me what you have so we discus how do we make it configurable?

@jbjuin
Copy link
Collaborator Author

jbjuin commented Jan 25, 2022

Hey @edelvalle,

I have a question about the integration in reactor 3 of one of my companion app I wrote (for reactor 2): "reactor store".
In reactor store one defines a Store that keep an in-memory global (to the session) shared state.
Components can interact with the global store to mutate, get and watch the shared state.

Here is a usage example:

# this is a mutation
def set_key(state, new_value):
    state["my-key"] = new_value
    return state


class MyStore(ReactorStore):

    # the shared state
    STATE = {"my_key": "A value"}
    # all mutations
    MUTATIONS = {"set_key": set_key}


class MyComponent(Component, MyStore):

    def mount(self, **kwargs):
        # initialize the store
        self.store_init()
        self.value = self.store_get("my_key")
        self.store_watch("my_key")
    
    # this handler will be called when "my_key" is modified by the mutation "set_key".
    def receive_set_key(self, my_key=None):
        print("my_key was changed by the set_key mutation")

Here are the few methods defined:

  • self.store_commit("set_key", "new_value") to "mutate" the state,
  • self.store_get("my_key") to get the shared value for that key,
  • self.store_watch("my_key") to receive the new value in a handler when "my-key" is mutated, etc.

Practically the state is a simple dict that is attached to the _root_component of each component which want to access the global state. The _root_component is unique per session (aka websocket connection) so the state can be shared by accessing that object.

I'm now wondering how I could do that same pattern in reactor 3 since there is no _root_component in components anymore.

Would it be possible, on component creation to add a reference to the ComponentRepository so that reactor store can keep the state on it ?
Can you see another way to reproduce that pattern or even improve it ? It is a very practical and simple pattern to develop apps where a page has multiple siblings components that needs to communicate. I could use the new pub/sub pattern to make communication possible between them but it is not exactly the same usage.

Cheers !

PS: Just thinking out loud: maybe an invisible "store" component included in the page could store the state and then mutations are just wrappers around the pub/sub pattern. Seems that it may possible this way...

@edelvalle
Copy link
Owner

One of the reasons I removed the root component is that there was a reference cycle between all components and the root component, which could cause memory leaks due the the reference counting. This is something I avoided in this new design.

That's why you see that the components do not have direct access to the repository and the ComponentMeta does not has access the repo or the component, at least not a persistent access, just over function calls.

Doing the invisible store component was my first idea, since it fits in the model and isolates the state like in an actor model. But if you need to read the state to take a decision you will end up with two functions. One to request state and other to analyze it. That could be complicated and ugly.

I have no clear answer right now. If you come up with an idea make a proposal.

@jbjuin
Copy link
Collaborator Author

jbjuin commented Jan 28, 2022

Hi @edelvalle,

just to keep you informed that I have a draft working for the reactor 3 "store" pattern.

So it basically expose a "get" and a "subscribe" API to components. It's a simple shared in-memory KV store. Components can get value from it, subscribe to key changes and change values through fixed mutations.

The "commit" and "subscribe" use the notifications + broadcast system that you added. Subscriptions to store keys is done by decorating the _subscriptions property. This is needed to keep the user defined subscriptions and extend them with the custom store keys channels (where the store publish the changes) - there may be other option to implement this if needed.

As you mentioned the tricky point was the "get" operation. Since I want it to be "synchronous", ie value = store_get("my_key"), I relied on a dirty hack. Indeed to perform this we need a way to directly access the store component instance.

One possible solution is:

def store_get(Store, keys):
    """Get values from the store Store keys."""
    # the hack: it works... but feels hacky :)
    import gc
    for obj in gc.get_objects():
        if isinstance(obj, Store):
            # here we return at the first found instance of the Store: those store components must be singles !
            return [obj.store_state[key] for key in keys]
    raise Exception("No key found")

In my implementation multiple Store can be used but a single component of each store can be instantiated at the same time.

Instead of relying on the gc I could have used some weakref on instances by overloading init or the classmethod new but I couldn't get it to work. Well, all this being said, this is a way to do it.

The API of this store proposal is not optimal, there is a lot of edge cases and no error handling for now.

What I can do is make a gist with the code + example usage so that you can try it ?

Cheers

PS: Maybe a more formal way of building Reactor App could be done, like Vue is doing it, with an "app" component on which we could add plugins (stores, a router, etc) that would be injected on all components instances.

@edelvalle
Copy link
Owner

I will think about this, but I want to have a clear use case to implement something like this.

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

No branches or pull requests

2 participants