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

Add a local speechbot service #88

Open
wants to merge 30 commits into
base: develop
Choose a base branch
from
Open

Conversation

emilyxzhou
Copy link
Contributor

The Rasa bot currently has 2 default assistants to choose from, both defined in harmoni_bot/src: rasa_example and rasa_greeter. The assistant can be changed in the configuration file by setting the rasa_assistant param value to the name of the Rasa bot's directory; rasa_assistant: "rasa_greeter" would direct the start_rasa_server script to run the server from the rasa_bot/src/rasa_greeter directory.

Currently, unit tests and rostests still need to be improved.

Signed-off-by: Emily Zhou <[email protected]>
Signed-off-by: Emily Zhou <[email protected]>
Signed-off-by: Emily Zhou <[email protected]>
Signed-off-by: Emily Zhou <[email protected]>
Signed-off-by: Emily Zhou <[email protected]>
Signed-off-by: Emily Zhou <[email protected]>
Signed-off-by: Emily Zhou <[email protected]>
…into feature/local-speechbot

# Conflicts:
#	dockerfiles/harmoni/kinetic/base/dockerfile
#	dockerfiles/harmoni/noetic/base/dockerfile

Signed-off-by: Emily Zhou <[email protected]>
Signed-off-by: Emily Zhou <[email protected]>
Signed-off-by: Emily Zhou <[email protected]>
Signed-off-by: Emily Zhou <[email protected]>
@emilyxzhou
Copy link
Contributor Author

Both unit tests and rostests have been updated now, and the Rasa service is fully functional.

Copy link
Collaborator

@micolspitale93 micolspitale93 left a comment

Choose a reason for hiding this comment

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

I have tested the rasa_greeter and it works fine for me.
I think that a brief tutorial on how to create a new rasa agent in the harmoni_bot package could be very helpful.

@@ -0,0 +1 @@
rasa==2.7.1
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why the Rasa version differs between the requirements (2.7.1) file and the docker images (2.4.2)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, the dockerfiles have been updated to 2.7.1 now!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good

Signed-off-by: Emily Zhou <[email protected]>
…into feature/local-speechbot

# Conflicts:
#	dockerfiles/harmoni/kinetic/base/dockerfile
#	dockerfiles/harmoni/noetic/base/dockerfile

Signed-off-by: Emily Zhou <[email protected]>
@emilyxzhou emilyxzhou marked this pull request as ready for review July 21, 2021 21:02
Copy link
Collaborator

@RMichaelSwan RMichaelSwan left a comment

Choose a reason for hiding this comment

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

Great work! Once comments are addressed I'll approve it.

class RasaService(HarmoniServiceManager):
"""This is a class representation of a harmoni_dialogue service
(HarmoniServiceManager). It is essentially an extended combination of the
:class:`harmoni_common_lib.service_server.HarmoniServiceServer` and
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see how this line is true; HarmoniServiceServer is really just an extension of the HarmoniActionServer and you have not overridden any of its functions

harmoni_dialogues/harmoni_bot/scripts/start_rasa_server.sh Outdated Show resolved Hide resolved
package_dir={'': 'src'},
package_dir={
'': 'src',
'harmoni_bot': os.path.join('src', 'harmoni_bot')
Copy link
Collaborator

Choose a reason for hiding this comment

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

As far as I know this line isn't needed. harmoni_common_lib doesn't have it and is imported the same way.

from collections import deque

# Specific Imports
import mock
Copy link
Collaborator

Choose a reason for hiding this comment

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

unused?

Signed-off-by: emilyxzhou <[email protected]>
Comment on lines 6 to 10
if [ "$RASA_ASSISTANT" == "rasa_example" ] || [ "$RASA_ASSISTANT" == "rasa_greeter" ]
then
cd "$RASA_DIR"/src/"$RASA_ASSISTANT"
rasa train && rasa run
else
Copy link
Member

Choose a reason for hiding this comment

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

This check works for now, but it doesn't work with future bots. Can we dynamically support based on what directories exist in the RASA_DIR?

#
# dispatcher.utter_message(text="Hello World!")
#
# return []
Copy link
Member

@chrismbirmingham chrismbirmingham Jul 23, 2021

Choose a reason for hiding this comment

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

We can probably delete this

Copy link
Member

@chrismbirmingham chrismbirmingham left a comment

Choose a reason for hiding this comment

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

I'd like to suggest we drop the rasa greeter from being committed here. I think we should include the example bot so there is an example, but that custom bot's should have their own repositories and be imported separately. @RMichaelSwan @micolspitale93 I'd like your opinions on this idea.

@emilyxzhou
Copy link
Contributor Author

@chrismbirmingham: I've removed rasa_greeter and moved rasa_example to the models directory. The start_server script has also been updated to dynamically check for existing Rasa workspaces.

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.

4 participants