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

enable conversation sessions by default #6934

Merged
merged 4 commits into from
Oct 6, 2020
Merged

enable conversation sessions by default #6934

merged 4 commits into from
Oct 6, 2020

Conversation

wochinge
Copy link
Contributor

@wochinge wochinge commented Oct 6, 2020

Proposed changes:

  • enable conversation sessions even if no session_config is given in the domain. This is a breaking change in contrast to Rasa Open Source 1 where this would result in disabled conversation sessions. The default value for session_expiration_time is the same one which we have been using in our rasa init project.
  • update migration guide accordingly
  • remove duplicate constants (caused when introducing rasa.shared).

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 wochinge changed the base branch from master to 2.0.x October 6, 2020 12:28
@@ -29,6 +29,17 @@ how you can migrate from one version to another.
component in your `policies` configuration (`config.yml`) you can replace it
with `TEDPolicy`. It accepts the same configuration parameters.

* [Conversation sessions](domain.mdx#session-configuration) are now enabled by default
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The title of the targeted section is currently Session Configuration. I'd vote for renaming this to Conversation Sessions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Mm, I think it makes the most sense to have the title of the section to reflect the high level keys in the domain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, then let's leave it as it is.

This enables sessions by default.
@wochinge wochinge added this to the 2.0 Rasa Open Source milestone Oct 6, 2020
@wochinge wochinge marked this pull request as ready for review October 6, 2020 12:32
Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

Looks good, this behavior makes more sense with regards to the "default configuration" we already reference in the docs 👌

@@ -185,7 +184,6 @@ def from_dict(cls, data: Dict) -> "Domain":
def _get_session_config(session_config: Dict) -> SessionConfig:
session_expiration_time_min = session_config.get(SESSION_EXPIRATION_TIME_KEY)

# TODO: 2.0 reconsider how to apply sessions to old projects and legacy trackers
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we've just decided to leave them as is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I changed the default from 0 to 60 which essentially enables them by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

i guess i assumed that this note was about "should we read all of the conversations from 2018 as one conversation even if we introduce sessions now? with regards to applying them to legacy trackers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, okay. Yes, these will have sessions enabled, too, now

@tmbo tmbo merged commit 0fc5d25 into 2.0.x Oct 6, 2020
@tmbo tmbo deleted the session-config-default branch October 6, 2020 14:06
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