💥 Plugin Overhaul #1139
💥 Plugin Overhaul #1139
Conversation
| # Activities won't be used by replayer | ||
| register_activities=False, |
There was a problem hiding this comment.
Is this something we expect users to do or should the plugin be self-aware enough to account for this itself?
There was a problem hiding this comment.
That's a fair point. It's all a bit odd with the change to SimplePlugin because the activity won't actually be registered, since they aren't part of ReplayerConfig but they need to be constructed to pass to the SimplePlugin constructor. I'll give it some thought. We could switch to callable use to delay it.
There was a problem hiding this comment.
Note, just because we have SimplePlugin doesn't mean the OpenAIPlugin has to leverage it. And even if it does leverage it, now that SimplePlugin is a class, just because it accepts activities doesn't mean you have to use it, you can just append the activity at worker creation/run time only.
However, what you are facing here is what other plugin developers will face. The benefit of the existing plugin interface is that they must think about this as authors of plugins that need to be properly usable by users. Had you not been familiar with Temporal, you would have been surprised by a user talking about the plugin not properly understanding replayers.
(note this thread is more just chatter, it does not affect my already-set approval)
There was a problem hiding this comment.
Definitely doesn't have to, but it is a good exercise. I considered subclassing instead and went with a callable for now.
There was a problem hiding this comment.
I do think this is a bit complicated compared to just overriding the method. Hopefully users understand that they need to initialize the activity in the callable too, not just return it there.
I saw the decision to use callables was made, this conversation was marked resolved, and the merge was performed. I also saw as part of this we removed the ability to not register activities in OpenAI plugin. Can we make sure we track the effort to allow splitting workflow workers and activity workers with this plugin?
| assert worker.config().get("workflows") == [] | ||
|
|
||
|
|
||
| class MediumPlugin(SimplePlugin): |
|
Beautiful, thanks for adding the test and overall to the team for rallying around this and building something really polished. |
1 similar comment
|
Beautiful, thanks for adding the test and overall to the team for rallying around this and building something really polished. |
5 participants
What was changed
Plugininterfaces to remove initializers and use invocation provided callables instead. This will prevent issues if the same plugin is used across multiple objects: clients, workers, etc.Why?
Checklist
Closes
How was this tested:
New Tests
Any docs updates needed?