Initial rough framework for plugins #952
Conversation
temporalio/client.py
Outdated
|
|
||
| # Iterate over interceptors in reverse building the impl | ||
| self._impl: OutboundInterceptor = _ClientImpl(self) | ||
| for interceptor in reversed(list(interceptors)): |
There was a problem hiding this comment.
This interceptor list now needs to be pulled off the self._config. Don't use any more parameters after on_create_client is called.
There was a problem hiding this comment.
Good call. I also need to go through and make a good set of tests around this sort of thing.
tests/worker/test_worker.py
Outdated
| @@ -1184,3 +1189,46 @@ def shutdown(self) -> None: | |||
| if self.next_exception_task: | |||
| self.next_exception_task.cancel() | |||
| setattr(self.worker._bridge_worker, self.attr, self.orig_poll_call) | |||
|
|
|||
|
|
|||
| class MyCombinedPlugin(temporalio.client.Plugin, temporalio.worker.Plugin): | |||
There was a problem hiding this comment.
What is the value to users of this multiple inheritance approach? It seems to be a discoverability loss--knowing you can add a second base class is not a leap I'd expect most people to make.
I'm also wondering how a plugin author will register the name/version of their plugin in this scheme.
There was a problem hiding this comment.
I would say a few things:
- It does inform a user of the plugin where it is intended to be used. This could be handled via documentation, but so can the discoverability loss.
- It's consistent with our existing layout for Interceptors. Should there also only be one kind of intercepter which you use everywhere? So that you can discover what is interceptable. If you think so, is it worth differing our design between them since that ship has sailed.
@cretz probably has thoughts here.
Name/version aren't present, but easy to add a function(s) to the Plugin classes which has to be implemented. I'll bring this up to the team as well.
There was a problem hiding this comment.
Basically what @tconley1428 said. The main benefit is separating what you're plugging into, same as interceptors (and has consistency with interceptors on this front for those familiar). But it's not a big deal, we can make temporalio.common.Plugin that combines client and worker concerns if we want, we've just traditionally tried to separate client and worker abstractions. We don't do worker stuff outside of the worker module for example, which is a logical approach.
There was a problem hiding this comment.
It does inform a user of the plugin where it is intended to be used.
This is redundantly shown by the names of the methods and the types of those methods, e.g. on_create_worker.
It's consistent with our existing layout for Interceptors
The purpose of a plugin is different from an interceptor. The whole point of a plugin is to bring together disparate things (interceptors, activities, etc) under one roof. The two purposes of this are (a) to help users easily add and (b) to give a roadmap for framework integrators to know what they can/need to do to. It's counterproductive to try to separate them out at this layer.
But it's not a big deal, we can make temporalio.common.Plugin that combines client and worker concerns if we want
👍, I would be in favor of temporalio.common.Plugin. If you want to make that multiply inherit, I don't mind if there's a good reason and as long as the samples all use the common.Plugin.
The main benefit is separating what you're plugging into
I'm not following the root reason here.
What was changed
Why?
Checklist
Closes
How was this tested: