Fix user ID always default on every shell session. #5117

VictorCoCo · 2020-01-23T11:23:47Z

Proposed changes:

It is hard to check the bot's functionality after making updates and/or changes, especially when
there is a need to maintain a session for a user.

For example:
When using a CoCo component, a session identifier needs to be maintained throughout and when the user ID
doesn't change from session to session, the conversation maintains the context from the previous shell session.

...

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)

claassistantio · 2020-01-23T11:24:00Z

All committers have signed the CLA.

akelad · 2020-01-23T15:14:14Z

Thanks for submitting this PR, we'll give it a review as soon as possible.

ricwo

I think the change makes sense in general - please have a look at the comments.

Would you mind adding a changelog entry? You can find the instructions in the README (you'll have to create an issue for reference in the changelog entry).

Thanks a lot for your contribution! 🎉

ricwo · 2020-01-24T21:41:07Z

rasa/core/channels/console.py

@@ -22,6 +22,8 @@

 DEFAULT_STREAM_READING_TIMEOUT_IN_SECONDS = 10

+CONSOLE_SESSION_USER_ID = str(uuid.uuid4())


we use the hex version of UUID4 elsewhere - it would be good to keep that consistent

Suggested change

CONSOLE_SESSION_USER_ID = str(uuid.uuid4())

CONSOLE_SESSION_USER_ID = uuid.uuid4().hex

rasa/core/channels/console.py

Co-Authored-By: ricwo <ric.wkr@gmail.com>

ricwo

Thanks a lot for the changes 👍

changelog/5117.improvement.rst

Co-Authored-By: ricwo <ric.wkr@gmail.com>

ricwo · 2020-01-30T11:38:00Z

@VictorCoCo can you please make sure test_console_input() passes?

erohmensing · 2020-01-31T16:00:21Z

@ricwo do you think it is worth it to be able to configure the id being set the same every time (or set a specific id, with default random)? I'm wondering if there is anyone who is currently banking on pulling the tracker from "default" every time who will have trouble in the future with an unpredictable conversation ID, especially in a script to pull trackers.

VictorCoCo · 2020-02-02T15:56:57Z

@ricwo do you think it is worth it to be able to configure the id being set the same every time (or set a specific id, with default random)? I'm wondering if there is anyone who is currently banking on pulling the tracker from "default" every time who will have trouble in the future with an unpredictable conversation ID, especially in a script to pull trackers.

@erohmensing Do you think passing the sender_id to the shell command as an optional argument would help?

ricwo · 2020-02-02T16:34:05Z

yes @erohmensing @VictorCoCo I think that is reasonable - as you suggested the easiest thing would be to add a command-line argument to rasa shell (--conversation-id)

VictorCoCo · 2020-02-03T14:26:48Z

@erohmensing Added

--conversation-id

missed your comment, will change --sender-id to --conversation-id

ricwo · 2020-02-04T18:57:24Z

@VictorCoCo can you please make sure all tests pass? feel free to request a review from me again afterwards

Co-Authored-By: ricwo <ric.wkr@gmail.com>

ricwo

Looks great, thanks for your changes 👍

changelog/5117.improvement.rst

Co-Authored-By: ricwo <ric.wkr@gmail.com>

ricwo · 2020-02-14T08:35:01Z

@VictorCoCo would you mind fixing the failing build?

VictorCoCo · 2020-02-17T08:05:26Z

@VictorCoCo would you mind fixing the failing build?

I see only an issue on code climate, an unrelated code issue. Maybe a permission issue, Do you want me to squash it?

chkoss · 2020-02-18T12:57:14Z

rasa/constants.py

@@ -59,3 +59,5 @@

 DEFAULT_SESSION_EXPIRATION_TIME_IN_MINUTES = 60
 DEFAULT_CARRY_OVER_SLOTS_TO_NEW_SESSION = True
+
+DEFAULT_SENDER_ID = "default"


@ricwo Would it be better to default to generating a random ID when rasa shell is called instead of fixing it here as a constant?

The reason for this suggestion is: If you have a persistent tracker store and run rasa shell, you would want to start from scratch with a new conversation id and not at whichever state the last session with a default id was in.
(We were also discussing that in the PR that adds such an argument for rasa interactive: #5243.)

I agree, It was the way I first changed it. @ricwo @erohmensing I am ok with changing it, What is your opinion?

@chkoss yes but this is a separate functionality and should probably be controlled with a different command-line argument, something like --assign-random-conversation-id

@VictorCoCo your initial implementation assigned a random user ID with every new shell session, and made it impossible to use a deterministic one. being able to define a conversation ID as a command-line argument and having the script choose a random one are two separate issues. whatever the default behaviour, there needs to remain the possibility of a deterministic conversation ID

tmbo · 2020-02-21T10:10:40Z

@VictorCoCo we'd love to make this part of the next release next week. Regarding the discussion, I think we should be consistent. By default we should generate a random id. If the user wants to use a specifc one, they can use --conversation-id to set it. (https://github.com/RasaHQ/rasa/pull/5243/files).

Do you mind changing that? Otherwise the PR looks great and we are ready to merge 🚀

inconsistend API, needs to be fixed before merge

VictorCoCo · 2020-02-21T10:24:35Z

@VictorCoCo we'd love to make this part of the next release next week. Regarding the discussion, I think we should be consistent. By default we should generate a random id. If the user wants to use a specifc one, they can use --conversation-id to set it. (https://github.com/RasaHQ/rasa/pull/5243/files).

Do you mind changing that? Otherwise the PR looks great and we are ready to merge 🚀

Great decision. I will make the changes until Monday.

tmbo · 2020-02-21T10:27:00Z

Perfect - we'll cut the release on Tuesday so Monday sounds great. Please shoot me a message once you are ready 🙂

tmbo

great work @VictorCoCo thanks a lot for jumping on this and getting the change ready to be merged 🚀

Fix user ID always default on every shell session.

08eda89

VictorCoCo requested a review from erohmensing January 23, 2020 11:24

akelad requested review from ricwo and removed request for erohmensing January 23, 2020 15:14

ricwo suggested changes Jan 24, 2020

View reviewed changes

VictorCoCo and others added 4 commits January 26, 2020 10:38

add changelog record.

4eda4ed

Update rasa/core/channels/console.py

7d093a8

Co-Authored-By: ricwo <ric.wkr@gmail.com>

Update rasa/core/channels/console.py

984c702

Co-Authored-By: ricwo <ric.wkr@gmail.com>

Update rasa/core/channels/console.py

1622793

Co-Authored-By: ricwo <ric.wkr@gmail.com>

ricwo approved these changes Jan 26, 2020

View reviewed changes

changelog/5117.improvement.rst Outdated Show resolved Hide resolved

changelog/5117.improvement.rst Outdated Show resolved Hide resolved

VictorCoCo and others added 6 commits January 26, 2020 12:22

Update changelog/5117.improvement.rst

0034e33

Co-Authored-By: ricwo <ric.wkr@gmail.com>

Update changelog/5117.improvement.rst

a5b185c

Co-Authored-By: ricwo <ric.wkr@gmail.com>

Merge branch 'master' into fix-user-id-always-default-on-shell-session

3dee8fd

Merge branch 'master' into fix-user-id-always-default-on-shell-session

88a3c09

Merge branch 'master' into fix-user-id-always-default-on-shell-session

cc2f1cc

Merge branch 'master' into fix-user-id-always-default-on-shell-session

bf9945e

Fix rasa core channels test.

a1351f5

Add --sender-id argument to rasa shell.

529c2e9

VictorCoCo requested a review from ricwo February 3, 2020 14:25

Change sell --sender-id argument name to --conversation-id

6c93b3f

VictorCoCo and others added 2 commits February 6, 2020 13:08

Adjust rasa shell help test to changes.

af2b561

Merge branch 'master' into fix-user-id-always-default-on-shell-session

2f40740

VictorCoCo and others added 6 commits February 10, 2020 10:50

Update rasa/core/run.py

b7ab04d

Co-Authored-By: ricwo <ric.wkr@gmail.com>

Update rasa/core/run.py

cc5e7b7

Co-Authored-By: ricwo <ric.wkr@gmail.com>

Update rasa/core/run.py

af501c7

Co-Authored-By: ricwo <ric.wkr@gmail.com>

Update rasa/core/run.py

f01166c

Co-Authored-By: ricwo <ric.wkr@gmail.com>

Update rasa/core/channels/console.py

2104ec3

Co-Authored-By: ricwo <ric.wkr@gmail.com>

Fix unresolved reference for UserMessage.

63fcf89

VictorCoCo requested a review from ricwo February 10, 2020 17:37

ricwo previously approved these changes Feb 11, 2020

View reviewed changes

changelog/5117.improvement.rst Show resolved Hide resolved

changelog/5117.improvement.rst Outdated Show resolved Hide resolved

ricwo and others added 3 commits February 11, 2020 08:19

Merge branch 'master' into fix-user-id-always-default-on-shell-session

823379a

Update changelog/5117.improvement.rst

d2454b0

Co-Authored-By: ricwo <ric.wkr@gmail.com>

Update changelog/5117.improvement.rst

927a272

Co-Authored-By: ricwo <ric.wkr@gmail.com>

wochinge mentioned this pull request Feb 11, 2020

Add sender_id argument shell and interactive command line tools #3975

Closed

3 tasks

Merge branch 'master' into fix-user-id-always-default-on-shell-session

bd38b69

erohmensing mentioned this pull request Feb 17, 2020

Issue 3975 #5243

Merged

4 tasks

chkoss reviewed Feb 18, 2020

View reviewed changes

Merge branch 'master' into fix-user-id-always-default-on-shell-session

f4cd9e8

degiz added this to the Rasa 1.8 milestone Feb 20, 2020

Merge branch 'master' into fix-user-id-always-default-on-shell-session

32be2b3

VictorCoCo added 3 commits February 23, 2020 15:48

Change --coversation-id to be random by default.

ecf01b6

Add conversation_id arg to interactive test.

ea75052

Add default conversation id value on core.run functions.

b1d32cb

VictorCoCo requested a review from tmbo February 23, 2020 17:17

tmbo approved these changes Feb 23, 2020

View reviewed changes

tmbo merged commit 1240405 into RasaHQ:master Feb 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix user ID always default on every shell session. #5117

Fix user ID always default on every shell session. #5117

VictorCoCo commented Jan 23, 2020 •

edited by tmbo

Loading

claassistantio commented Jan 23, 2020 •

edited

Loading

akelad commented Jan 23, 2020

ricwo left a comment

ricwo Jan 24, 2020

ricwo left a comment

ricwo commented Jan 30, 2020 •

edited

Loading

erohmensing commented Jan 31, 2020 •

edited

Loading

VictorCoCo commented Feb 2, 2020 •

edited

Loading

ricwo commented Feb 2, 2020

VictorCoCo commented Feb 3, 2020

ricwo commented Feb 4, 2020

ricwo left a comment

ricwo commented Feb 14, 2020

VictorCoCo commented Feb 17, 2020

chkoss Feb 18, 2020

VictorCoCo Feb 18, 2020

ricwo Feb 18, 2020

tmbo commented Feb 21, 2020

VictorCoCo commented Feb 21, 2020

tmbo commented Feb 21, 2020

tmbo left a comment

		@@ -22,6 +22,8 @@

		DEFAULT_STREAM_READING_TIMEOUT_IN_SECONDS = 10

		CONSOLE_SESSION_USER_ID = str(uuid.uuid4())

	CONSOLE_SESSION_USER_ID = str(uuid.uuid4())
	CONSOLE_SESSION_USER_ID = uuid.uuid4().hex

Fix user ID always default on every shell session. #5117

Fix user ID always default on every shell session. #5117

Conversation

VictorCoCo commented Jan 23, 2020 • edited by tmbo Loading

claassistantio commented Jan 23, 2020 • edited Loading

akelad commented Jan 23, 2020

ricwo left a comment

Choose a reason for hiding this comment

ricwo Jan 24, 2020

Choose a reason for hiding this comment

ricwo left a comment

Choose a reason for hiding this comment

ricwo commented Jan 30, 2020 • edited Loading

erohmensing commented Jan 31, 2020 • edited Loading

VictorCoCo commented Feb 2, 2020 • edited Loading

ricwo commented Feb 2, 2020

VictorCoCo commented Feb 3, 2020

ricwo commented Feb 4, 2020

ricwo left a comment

Choose a reason for hiding this comment

ricwo commented Feb 14, 2020

VictorCoCo commented Feb 17, 2020

chkoss Feb 18, 2020

Choose a reason for hiding this comment

VictorCoCo Feb 18, 2020

Choose a reason for hiding this comment

ricwo Feb 18, 2020

Choose a reason for hiding this comment

tmbo commented Feb 21, 2020

VictorCoCo commented Feb 21, 2020

tmbo commented Feb 21, 2020

tmbo left a comment

Choose a reason for hiding this comment

VictorCoCo commented Jan 23, 2020 •

edited by tmbo

Loading

claassistantio commented Jan 23, 2020 •

edited

Loading

ricwo commented Jan 30, 2020 •

edited

Loading

erohmensing commented Jan 31, 2020 •

edited

Loading

VictorCoCo commented Feb 2, 2020 •

edited

Loading