Skip to content
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

feat(converse): converse api #194

Merged
merged 22 commits into from
Oct 18, 2021
Merged

feat(converse): converse api #194

merged 22 commits into from
Oct 18, 2021

Conversation

samuelmasse
Copy link
Contributor

@samuelmasse samuelmasse commented Oct 13, 2021

Adds a collector to the message api to replicate behavior similar to the converse api

Closes DEV-1909
Closes DEV-1612

Requires to be on this branch when testing: botpress/botpress#5568

@linear
Copy link

linear bot commented Oct 13, 2021

@samuelmasse samuelmasse marked this pull request as ready for review October 13, 2021 21:10
packages/server/src/messages/api.ts Show resolved Hide resolved
packages/server/src/converse/service.ts Outdated Show resolved Hide resolved
packages/server/src/converse/service.ts Outdated Show resolved Hide resolved
Co-authored-by: Laurent Leclerc Poulin <laurentlp@users.noreply.github.com>
@allardy
Copy link
Member

allardy commented Oct 14, 2021

What do you think of using messaging as a simple proxy for converse ?

Right now, when you call converse, we start a timeout of 5 sec before considering the request as completed and sending back a response to the user. This delay is paused as long as an action is running, and the timeout is cleared as soon as the conversation is completed, so there is no "random" delay (like your 3 secs).

Basically, the route could be User -> Messaging -> Call /converse on runtime, wait for response, then -> User ?

@samuelmasse
Copy link
Contributor Author

@allardy I don't think messaging should depend on the runtime. What we'll do to make this a bit better is remove the await for the typing indicators delay, so that the runtime sends all of its messages one after the other. To avoid all the messages being sent at the same time, we'll put the messages in a queue in the channel code, so they are sent sequentially and they wait for each typing indicator.

We'll still need a magic number for how much we wait, but it'll be much smaller and it won't depend on the amount of time a typing indicator lasts on a certain message.

Copy link
Contributor

@laurentlp laurentlp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will pre-approve this, but we'll merge it once you have made the fix for the typing indicators.

@allardy
Copy link
Member

allardy commented Oct 14, 2021

@samuelmasse The runtime will not be exposed to the outside world, so communications must go through messaging. Is this a good replacement for the converse api, and can we use it to benchmark messaging with botpress efficiently ?

@samuelmasse samuelmasse changed the base branch from master to sm-message-queue October 15, 2021 03:39
Base automatically changed from sm-message-queue to master October 18, 2021 21:49
@samuelmasse
Copy link
Contributor Author

Merging this right now. I'll make the final improvements in another PR

@samuelmasse samuelmasse merged commit f4ecbca into master Oct 18, 2021
@samuelmasse samuelmasse deleted the sm-converse branch October 18, 2021 22:14
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.

None yet

3 participants