-
Notifications
You must be signed in to change notification settings - Fork 9
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
Multiple messenger interface support #332
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good.
@@ -62,7 +63,7 @@ class Pipeline: | |||
- key: :py:class:`~dff.script.ActorStage` - Stage in which the handler is called. | |||
- value: List[Callable] - The list of called handlers for each stage. Defaults to an empty `dict`. | |||
|
|||
:param messenger_interface: An `AbsMessagingInterface` instance for this pipeline. | |||
:param messenger_interfaces: An `AbsMessagingInterface` instance for this pipeline. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
todo: fix docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should also add tests for launching pipeline with multiple http interfaces and running them at the same time on different ports.
Could you please remind me our final decision: do we pass interfaces as dict or as array only (with default or user-specified names)? |
The latter. |
interface = CLIMessengerInterface() | ||
self.messenger_interfaces = {interface.name: interface} | ||
else: | ||
self.messenger_interfaces = {iface.name: iface for iface in messenger_interfaces} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should check for duplicates.
@abc.abstractmethod | ||
async def connect(self, pipeline_runner: PipelineRunnerFunction): | ||
async def connect(self, pipeline_runner: PipelineRunnerFunction, iface_id: str): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add this parameter when it is always self.name
?
@@ -252,7 +252,7 @@ class ExtraHandlerRuntimeInfo(BaseModel): | |||
PipelineBuilder: TypeAlias = TypedDict( | |||
"PipelineBuilder", | |||
{ | |||
"messenger_interface": NotRequired[Optional["MessengerInterface"]], | |||
"messenger_interfaces": NotRequired[Optional[Union["MessengerInterface", Iterable["MessengerInterface"], Dict[str, "MessengerInterface"]]]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update this.
|
||
|
||
# %% | ||
telegram_interface = PollingTelegramInterface(token=os.environ["TG_BOT_TOKEN"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This tutorial is currently broken due to calling infinity_polling
in this interface.
#328 also uses self.application.run_polling
to start the bot, so I'm assuming it would also cause issues.
See the tip:
https://docs.python-telegram-bot.org/en/v21.1.1/telegram.ext.application.html#telegram.ext.Application.run_polling
Description
Multiple user interface support added.
Checklist
To Consider
.ignore
files, scripts (such aslint
), distribution manifest (if files are added/deleted)