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 sender_id argument shell and interactive command line tools #3975

Closed
3 tasks
djwessel opened this issue Jul 10, 2019 · 22 comments
Closed
3 tasks

Add sender_id argument shell and interactive command line tools #3975

djwessel opened this issue Jul 10, 2019 · 22 comments
Assignees
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR

Comments

@djwessel
Copy link

Description of Problem:

Both rasa shell and rasa interactive do not support the ability to set the sender_id field. Instead for shell, the sender_id is just set to the default value of default, where as for interactive, the field is set to a uuid. To test stories/run interactive learning with custom actions that rely on the sender_id field value, the current tools lack the ability to customize this field.

Overview of the Solution:

A straightforward solution would be to add a --sender_id command line argument to rasa shell and rasa interactive. This argument would then be passed to the respective record_messages function, which already supports the sender_id parameter.

Furthermore, the default value for this argument should not change the functionality of the current implementation (shell defaults to default and interactive defaults to a uuid).

Examples (if relevant):

An example of a custom action that would rely on the value of the sender_id is as follows.

import logging

from rasa_sdk import Action

logger = logging.getLogger(__name__)

class ActionLogSenderId(Action):

  def name(self):
    return "action_log_sender_id"

  def run(self, dispatcher, tracker, domain):
    logger.info("Sender id: {}".format(tracker.sender_id))

Currently, when running rasa shell, the logged value would be Sender id: default, whereas when running rasa interactive, the logged value would be Sender id: {some random uuid.uuid4().hex}.

With the proposed change, when the user runs rasa shell --sender_id unique123 or rasa interactive --sender_id unique123, the logged value would be Sender id: unique123

Blockers (if relevant):

None

Definition of Done:

  • Tests are implemented for shell and interactive.
  • The --sender_id argument is implemented and meets tests.
  • The --sender_id argument is added to the Command Line Interface docs for both shell and interactive.
@djwessel djwessel added the type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR label Jul 10, 2019
@akelad
Copy link
Contributor

akelad commented Jul 10, 2019

Can I ask why you would need this though? What would you need to test with respect to the sender id in interactive learning or when just chatting to the bot?

@akelad akelad added the status:more-details-needed Waiting for the user to provide more details / stacktraces / answer a question label Jul 10, 2019
@djwessel
Copy link
Author

Hey, similar to the custom FetchProfileAction action in the docs here, we use the sender_id to fetch information about the user, which then sets a slot which is used to inform the following actions to take. In order to use interactive learning, or even just chat with the bot using the shell, we need to be able to customize the sender_id.

@no-response no-response bot removed the status:more-details-needed Waiting for the user to provide more details / stacktraces / answer a question label Jul 11, 2019
@akelad
Copy link
Contributor

akelad commented Jul 12, 2019

You could probably use some dummy information for the default sender id for testing. I'll discuss this enhancement with our team though and let you know whether we think it makes sense to add

@akelad akelad added the type:discussion 👨‍👧‍👦 Early stage of an idea or validation of thoughts. Should NOT be closed by PR. label Jul 12, 2019
@sjtilney
Copy link

Not having the ability to set the sender_id at run time has also caused a lot of headache for our team.
Our Rasa actions interact with many different REST services. Many of those services require a valid user id (sender_id) in order to get a successful response.
Furthermore, we expect different responses depending on the region that the user is in or preferences they may have set. It's impossible to test these cases without being able to change the sender_id on the go.

For example, if someone asks a bot, "when is the next holiday?" we would expect a different response for a user in Germany versus a user in the United States. The only way to test that these cases are working properly during interactive training is to change the sender_id to one associated with a user in the various countries.

@wochinge
Copy link
Contributor

wochinge commented Jul 24, 2019

Ok, we discussed this in the team and we think that would be a great contribution 👍
We discussed also different ways of doing it, and doing command line arguments for both, the shell and the interactive, seem to make the most sense.

@sjtilney @djwessel Would you be up for doing a pull request on this?

@wochinge wochinge added area:rasa-oss 🎡 Anything related to the open source Rasa framework and removed type:discussion 👨‍👧‍👦 Early stage of an idea or validation of thoughts. Should NOT be closed by PR. labels Jul 24, 2019
@djwessel
Copy link
Author

djwessel commented Jul 25, 2019

@wochinge Great! Thanks for the feedback. I should be able to set up a pull request in the next few days.

@wochinge
Copy link
Contributor

@djwessel That's amazing! Thanks for proposing this, discussing and contributing 💯 Let me know if you need any help on the way (you can also tag me here and I can have a look at an early draft 👍 )

@msamogh msamogh added type:discussion 👨‍👧‍👦 Early stage of an idea or validation of thoughts. Should NOT be closed by PR. and removed type:discussion 👨‍👧‍👦 Early stage of an idea or validation of thoughts. Should NOT be closed by PR. labels Aug 6, 2019
@DiegoProtec
Copy link

DiegoProtec commented Sep 2, 2019

I need to pass sender_id to cmdline channel. It would be possible to adapt the param sender_id on console.py as optional to receive the sender from credentials.yml?

@wochinge
Copy link
Contributor

@DiegoProtec Sorry, for the late response. I was on vacation.

It would be possible to adapt the param sender_id on console.py as optional to receive the sender from credentials.yml?

Yes, that we would definitely be possible, but as I discussed with the team, the preferred solution would be to pass this information via the command line. Hence, I would suggest to add a parameter sender_id to configure_app and serve_application and then use this one in record_messages. Do you have any more thoughts on that @tabergma ?

@DiegoProtec
Copy link

DiegoProtec commented Sep 13, 2019 via email

@tabergma
Copy link
Contributor

Adding a new parameter to the command line sound good 👍 However, I would name it --sender-id instead of --sender_id due to naming convention. Seems like a straight forward solution.

@wochinge
Copy link
Contributor

@DiegoProtec Could you please clarify your solution a bit or share your pr? I think that would make it easier to discuss your changes :-)

@wochinge
Copy link
Contributor

@DiegoProtec How is it going? Is there anything I can help you with?

@wochinge
Copy link
Contributor

@DiegoProtec What's your current state? can I help you?

@DiegoProtec
Copy link

@wochinge Thanks for your attention, I decided to add the sender_id parameter when it is development environment in custom tracker store.

if sender_id and environment == 'dev':
            self.local_sender_id = sender_id

@wochinge
Copy link
Contributor

hm, do you have a draft pr where I can have a look at the proposed changes? I think that would be easier for giving feedback.

@wochinge
Copy link
Contributor

@DiegoProtec How is it going? If you don't have time, I'd assign it to one of my colleagues

@wochinge
Copy link
Contributor

@ChristinaKoss When you start with it, I'd split in two PRs

  • add sender id to rasa interactive
  • add sender id to rasa shell

@wochinge
Copy link
Contributor

wochinge commented Feb 3, 2020

@tmbo Should we use conversation-id or sender-id as parameter (in relation to this issue #4683) ? I'd opt for sender-id since it's shorter

@tmbo
Copy link
Member

tmbo commented Feb 3, 2020

as far as I know we always expose it as sender id in our storage formats / events / apis, so should stick with that (even though nowadays conversation-id is probably the more descriptive name...)

@wochinge
Copy link
Contributor

The rasa shell part is handled by #5117

@wochinge wochinge mentioned this issue Feb 14, 2020
4 tasks
@chkoss
Copy link
Contributor

chkoss commented Feb 18, 2020

The rasa interactive part is done now (#5243)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework type:enhancement ✨ Additions of new features or changes to existing ones, should be doable in a single PR
Projects
None yet
Development

No branches or pull requests

9 participants