Skip to content

Conversation

@lluisCM
Copy link
Contributor

@lluisCM lluisCM commented Jul 15, 2024

Resolves :

This PR has been tested on :

  • Windows.
  • MacOs.
  • Linux.

Overview

Purpose: Allow event manager to publish remote and local events.

Scope:

  • constants
  • framework-core/event manager
  • All frameowrk /projects because the way that they initialize session and the event manager has changed.

Implementation Details

Approach:

  • When initialising the event manager the user can choose if to allow remote events which by default is true. This means that the event manager will make sure that the session is connected to the event hub and will spread a thread as a listener to wait for the remote events.
  • All the predefined events on the Publish class of the event manager has now the argument remote=True/False to decide whether that specific event should run in remote or local.

Reasoning:
I have removed the “mode” in favor of the allow_remote events for the following reasons:

  • Is in the session and not the event manager where the user decides if to work locally or remotely with the auto_connect_event_hub.
  • The allow_remote argument only makes sure that the session has been connected, but doesn’t mean that when using the event manager all the events from now on will be published remotely.
  • Instead, this ability is on each event, so depending on the needs the user can decide if to run a particular event remotely or not using the argument remote=True of the published event.

In my opinion this gives more flexibility in the framework for advanced users, also allow us to slowly make sure that each event is correctly implemented in booth locally and remotely modes, otherwise we need to make sure that all events in the event manager work in remote mode since the release (And that is a big task to do that I have lowered the priority in order to be able to tackle it after the summer, here is the link to the task: https://dev.ftrackapp.com/#slideEntityId=70c014cf-922c-4680-985f-a9a5bf45d316&slideEnt[…]tityId=dc34b754-79e8-11e3-b4d0-040102b7e101&entityType=show )

Changes:
Trade-offs:

  • Incompatibility with previous versions (That is why framework-core will be 3.0.0) due to the remobal of the event manager argument mode In any case, this argument was all around but never used.
  • Removal of the constants REMOTE_EVENT_MODE = 1and LOCAL_EVENT_MODE = 0 which makes it incompatible with prevoius version that is why updateing to 3.0.0 version.
  • All framework /projects has to be updated and re-released but now is a good moment to do that as we need to re-release all for the publisher and loader.

Testing

Tests Added:
Manual Testing:
Testing example in the documentation: https://developer.ftrack.com/pr/47/integrations/advanced/framework/architecture/framework-core#event-manager

from ftrack_framework_core import event
import ftrack_api

def my_publish_callback(_event):
    '''Event callback printing all new or updated entities.'''
    print(f"callback: {_event}")
def my_subscribe_callback(_event):
    print(f"sub_callback: {_event}")
    return "my-reply"

session = ftrack_api.Session(auto_connect_event_hub=True)
event_manager = event.EventManager(session, allow_remote_events=True)

event_manager.subscribe.host_run_tool_config("my-host_id", my_subscribe_callback)
# Run local event will return the reply from the callback:
reply = event_manager.publish.host_run_tool_config("my-host_id","my-tool-config-reference",{"data":"example"}, my_publish_callback)
print (reply)
# Run remote event will run asynchronous and will print the reply in the callback:
event_manager.publish.host_run_tool_config("my-host_id","my-tool-config-reference",{"data":"example"}, my_publish_callback, remote=True
                                           )

Testing Environment:

Notes for Reviewers

Focus Areas: framework-core/event; EventManager
Dependencies:
Known Issues:

@mlagergren
Copy link
Contributor

First thank you for updating the PR description and clarifying your reasoning, it helps a lot! 🙏

I'd like to dig a little deeper into the changes in order to understand the motivation of changing the event manager. And I'd like to make sure I understand the intend of the mode as it was previously implemented:

  1. local: allows the host and client to "live" in the same process and use the event manager for communication.
  2. remote allows the host and client to "live" in separate processes but communicate over the ftrack server event hub.

Now, from what I understand what we want is to allow the session to be connected to the remote event_hub (session = ftrack_api.Session(auto_connect_event_hub=True)), and that this should be possible regardless of whether the event_manger is running remote or local? Have I understood this part correctly?

And would you say that the purpose of the event manager as it is today (previous to this change) is to manage the client/host communication in an abstracted way. So that Client and Host doesn't need to understand if they work in remote or local.

@lluisCM
Copy link
Contributor Author

lluisCM commented Jul 22, 2024

Now, from what I understand what we want is to allow the session to be connected to the remote event_hub (session = ftrack_api.Session(auto_connect_event_hub=True)), and that this should be possible regardless of whether the event_manger is running remote or local? Have I understood this part correctly?

Yes.

And would you say that the purpose of the event manager as it is today (previous to this change) is to manage the client/host communication in an abstracted way. So that Client and Host doesn't need to understand if they work in remote or local.

Maybe yes, not sure, and not sure if this is good. Why can't I decide in the client when publishing an event if the event should be processed by the remote ftrack event server (asyncrounous) or should it be processed locally (meaning as well a obtaining a direct result from the published event) ?

So, how I see this: As you said, Session can (or not) be connected to the remote event_hub with the auto_connect_event_hub=True using mode in the Event Manager means that all the events published by the event manager will happen locally or remote depending on the mode that the event manager has been initialized.
If the mode is remote, event manager makes sure that the session is connected to the remote event hub and all the events will be processed by the ftrack server event hub. (ALL THE EVENTS, so there is no way to process some events locally to not saturate the ftrack server and to not have to wait for a asyncronous responses and just process the returned value.)

In the proposed method, Session is meant to by default be auto_connect_event_hub=True (Of course it works if its not). Then by default the EventManager process all the pre-defined events locally. But if any event must be processed remotely then it can do it. So the idea behind is that mode doesn't need to be changed, because you decide how the event should be processed when publishing the event and not in the EventManager initialization.

IMPORTANT:
But I just realised that what I'm proposing is wrong, because, what if by default, the user wants all the events to be processed remotely? In that case, yes we also need a default_mode for the event manager which can say that by default all events are processed remote or local.
So yes, sorry, I'm wrong, probably if the event manager is used inside the framework, you never want to publish some events remote and some events local, you always want them all or local or remote, right? (because if this is not true, then we should be able to choose that when publishing the event as in the proposed code).

Then for what it concerns the root cause of this changes, when the framework is working locally, the subscription of any "default" ftrack server event like ftrack.action.discover, can still happen in the event manager but and it only needs to make sure that the seesion has been connected in any case even if the mode is local.

@mlagergren
Copy link
Contributor

mlagergren commented Jul 22, 2024

So yes, sorry, I'm wrong, probably if the event manager is used inside the framework, you never want to publish some events remote and some events local, you always want them all or local or remote, right? (because if this is not true, then we should be able to choose that when publishing the event as in the proposed code).

So my take on this is that with a host and client model that we have, the event manager abstracts the communication between the host and client. It can be through local events in the session, remote events, or in the future it could be through https or some other ways.
With this in mind I'd like to keep the event manager purely dedicated for client/host communication and not involve other communication to the remote events hub like registering actions - which is "similar" on the surface, but really different in the purpose.

@lluisCM lluisCM changed the base branch from main to backlog/create_api_session_util July 24, 2024 10:13
Base automatically changed from backlog/create_api_session_util to main July 25, 2024 07:40
@lluisCM lluisCM merged commit 3a05572 into main Jul 25, 2024
@lluisCM lluisCM deleted the backlog/event-manager-allow-remote-and-local-events branch July 25, 2024 09:22
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.

3 participants