rasa icon indicating copy to clipboard operation
rasa copied to clipboard

Support transports other than HTTP(S)

Open veigaribo opened this issue 3 years ago • 4 comments

This implements a solution for the issue #7789.

What I did was the second idea I proposed, where basically we create an intermediary method setup (which I referred to as register in the issue) to actually register the Channel blueprint to the app. (I give my reasons for it below, but, please, bring it up if you feel like another approach would be better)

What that allows us to do is basically access both the Sanic app and the on_new_message callback, which makes it possible for us to register a listener on the app and use the on_new_message only when it is ready.

Suppose we have an InputChannel that is supposed to receive it's messages using AMQP:

class AmqpInput(InputChannel):
    # ...

    # override setup
    def setup(self, on_new_message, app, options):
        # we may still register the blueprint for health-check, etc
        super().setup(on_new_message, app, options)

        # defer actual setup for when the server has started
        app.register_listener(
            # when this lambda executes, we may be sure on_new_message is ready to be called
            # so we may start consuming immediately
            lambda _, loop: self._setup_connection(on_new_message, loop),
            "after_server_start",
        )

(https://github.com/Veigaribo/example-rasa-amqp-connector/blob/second-approach/connectors/amqp.py)

Also, I created a class (that could very well be a dataclass) to hold extra options, just in case we need to add more. Then we may do it in a backwards-compatible way.


The reasons why I chose this approach over the other one (creating a on_agent_ready method to run specifically when the server has started) are:

  1. It just seems much easier to implement
  2. I imagine it will be easier to use this solution to solve #7525 (more below)
  3. Basically, compared to the other approach, this seems more future-proof, in the sense that it gives more power to the end-user to do what they want.

Regarding the issue #7525 (and PR #7526), the improvement I imagine this would enable would be to do something like:

class SlackInput(InputChannel):
    # ...

    # override setup
    def setup(self, on_new_message, app, options):
        # do not register the blueprint at first

        # by deferring the setup to just before the server starts, we are able to get access to the event loop
        app.register_listener(
            # then we may give the would-be-blueprint method access to it
            lambda _, loop: self.blueprint_with_loop(on_new_message, loop),
            "before_server_start",
        )

    def blueprint_with_loop(on_new_message, loop):
        # ...

        loop.create_task(on_new_message(user_msg))

Questions

  • Are there any tests I need to change / add? From what I looked, the tests/core/test_channels.py, tests/core/test_run.py and the ones in tests/core/channels passed. Does that mean it's ok?

Status:

  • [ ] added some tests for the functionality
  • [ ] updated the documentation
  • [ ] updated the changelog (please check changelog for instructions)
  • [ ] reformat files using black (please check Readme for instructions)

veigaribo avatar Feb 10 '21 05:02 veigaribo

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Feb 10 '21 05:02 CLAassistant

Thanks for submitting a pull request 🚀 @alopez will take a look at it as soon as possible ✨

sara-tagger avatar Feb 10 '21 07:02 sara-tagger

I'm working on another approach at #8031, I suppose only one of the pull requests should be accepted.

veigaribo avatar Feb 24 '21 00:02 veigaribo

This PR has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Apr 16 '22 07:04 stale[bot]