Issue 3975

[wochinge]

Proposed changes:

• fix rasa interactive part Add sender_id argument shell and interactive command line tools #3975
• rename sender-id to conversation -id

Status (please check what you already did):

• 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)

[erohmensing]

Looks good in general. I'm guessing that although Tom said in the issue that we should expose sender-id, you used conversation-id because that's what the contributor for rasa shell did?

[erohmensing]

Why do we need to set this here as well as here?

[chkoss]

Would be enough to only set it once. The same holds for skip_visualization though and that was set in both places, so we went with the same redundancy for conversation_id to keep it consistent.

Also, I think it makes sense because of this:

• We need the default value in the argument parser because then rasa interactive --help will display it.
• If the code ever changes such that run_interactive_learning will also be called elsewhere than via rasa interactive it is useful to have the default value set also here in the function.

[erohmensing]

We need the default value in the argument parser because then rasa interactive --help will display it.

Hm, this is a good point. But also makes me think that the default might be e.g. 9a1dcae1f3ad41b791eb7a6d9e7a57c7 and i'd expect to be able to query the tracker for that, when in reality by the time i did the actual command, it would have changed.

If the code ever changes such that run_interactive_learning will also be called elsewhere than via rasa interactive it is useful to have the default value set also here in the function.

Yes, definitely agree with this.

[erohmensing]

One thing to add, I don't know if we need to change this functionality here or not, but wanted to point out that the default conversation-id for the other PR is going to be "default", so, not sure if the difference in functionality of the same parameter for the two commands is odd.

[chkoss]

Maybe we should have the default value only in this function then, not in the parser, and for the parser manually add a sentence like "Defaults to a random uuid"?

[erohmensing]

When I run rasa interactive I want to start from scratch and not start at a random state.

Pretty sure it always would, though, or the same issue would happen in shell

But I'm fine either way.

[wochinge]

The same thing happens in shell if you have a persistent tracker store

[erohmensing]

Hm, had no idea. Ok @chkoss let's just go with this then:

I'd keep it in the parser and maybe a sentence to the help message. That should make it clear enough and we don't add unnecessary handling of the None value to the code.

[chkoss]

Done :) Should be ready to be merged now, right?

If the same thing happens in shell with a persistent tracker store, then it would probably be better to make it a random ID there, too, instead of 'default'. Shall I open an issue for that?

[erohmensing]

@chkoss maybe you can comment on the open PR to that effect? #5117

[wochinge]

Tom said in the issue that we should expose sender-id, you used conversation-id because that's what the contributor for rasa shell did?

Yep, there was a slack conversation where we decided to go for conversation-id

[erohmensing]

Btw, in the future it'd be helpful if the name of the PR referenced the changes themselves instead of the issue -- i had trouble finding it in the PR list