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

No session start when appending events via API #5446

Merged
merged 13 commits into from
Mar 20, 2020

Conversation

wochinge
Copy link
Contributor

@wochinge wochinge commented Mar 19, 2020

Proposed changes:

  • don't append events when using the append endpoint at the API. When you append events event by event, then we should not run / add events automatically and rather take the history as it is
  • remove deprecation for /conversations/<conversation_id>/execute since this part is heavily used by rasa interactive

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)

@wochinge
Copy link
Contributor Author

wochinge commented Mar 19, 2020

@ricwo Could you please have a short look? This will fix some behavior in Rasa X interactive learning (part of https://github.com/RasaHQ/rasa-x/issues/2023 ) .

And another thing: This changes the behavior of rasa interactive - we could also use a query param to decide whether to retrieve an empty tracker or one with session start events I checked it out. The session start events are still correctly attached in rasa interactive

@wochinge wochinge added this to the Rasa 1.9 milestone Mar 19, 2020
@wochinge wochinge requested a review from ricwo March 19, 2020 17:15
Copy link
Contributor

@ricwo ricwo left a comment

Choose a reason for hiding this comment

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

The code looks great so far 👍

I checked it out. The session start events are still correctly attached in rasa interactive

Yes rasa interactive uses GET /tracker which fetches the tracker with the session start sequence

rasa/core/processor.py Outdated Show resolved Hide resolved
rasa/core/processor.py Outdated Show resolved Hide resolved
rasa/core/processor.py Outdated Show resolved Hide resolved
rasa/server.py Outdated Show resolved Hide resolved
rasa/server.py Outdated Show resolved Hide resolved
Comment on lines 716 to 717
def get_tracker(self, conversation_id: Text) -> Optional[DialogueStateTracker]:
"""Get the tracker for a conversation.
Copy link
Contributor

Choose a reason for hiding this comment

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

should we make this a processor method, so the contrast with get_tracker_with_session_start() is more apparent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mhm, it's already a processor method? I could move it up so that they are right next to each other?

@wochinge wochinge marked this pull request as ready for review March 20, 2020 09:10
@wochinge wochinge force-pushed the append-events-without-session-start branch from 00b8a79 to 6ba8d86 Compare March 20, 2020 09:10
@wochinge wochinge requested a review from ricwo March 20, 2020 09:10
Copy link
Contributor

@ricwo ricwo left a comment

Choose a reason for hiding this comment

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

💯

changelog/5446.improvement.rst Outdated Show resolved Hide resolved
@wochinge wochinge merged commit a39bc32 into master Mar 20, 2020
@wochinge wochinge deleted the append-events-without-session-start branch March 20, 2020 15:11
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.

2 participants