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.

Sign up for GitHub

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

Jump to bottom

Make UnexpecTEDIntentPolicy compatible with E2E data #9203

Merged
merged 9 commits into from
Jul 29, 2021
Merged

Make UnexpecTEDIntentPolicy compatible with E2E data #9203

merged 9 commits into from
Jul 29, 2021

Conversation

dakshvar22
Copy link
Contributor

@dakshvar22 dakshvar22 commented Jul 26, 2021 •
edited
Loading

Proposed changes:

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)

@dakshvar22 dakshvar22 requested review from a team and tttthomasssss and removed request for a team and tttthomasssss July 26, 2021 16:52
@dakshvar22 dakshvar22 requested a review from kedz July 27, 2021 09:04
@kedz
Copy link
Contributor

kedz commented Jul 28, 2021 •
edited
Loading

@dakshvar22 When does this happen: ActionExecuted events with no action_name.
Is that because of an E2E prediction?

@dakshvar22
Copy link
Contributor Author

dakshvar22 commented Jul 28, 2021 •
edited
Loading

It's the debatable case that was discussed on the disentangling doc as well and something which is lying around from the E2E implementation. When you create an E2E story like -

story:
  - user: "Hello, i'm looking for a play station"
  - bot: "Which version"

See the usage of bot instead of action. Whenever a training story like this is added, ActionExecuted events don't have an action_name but they have action_text.

kedz
kedz approved these changes Jul 28, 2021
View reviewed changes
Copy link
Contributor

@kedz kedz left a comment

Choose a reason for hiding this comment

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

This PR looks good!

@dakshvar22 dakshvar22 enabled auto-merge (squash) July 28, 2021 13:14
@kedz
Copy link
Contributor

kedz commented Jul 28, 2021

@dakshvar22 This is beyond the scope of this PR but is it worth considering whether to hide E2E turns from UnexpecTEDIntentPolicy rather than removing them from training data? I'm worried that this current setup will cause UnexpecTEDIntentPolicy to do unpredictable things at run time after an E2E turn because it's input state features will include featurizations of user or bot utterances and it will not have seen those at training time or if these turns are hidden from the policy at run time, the states will still be unusual given the training data censoring. WDYT?

@dakshvar22 dakshvar22 disabled auto-merge July 28, 2021 13:21
@dakshvar22
Copy link
Contributor Author

Let me first disable auto-merge 😅

@dakshvar22
Copy link
Contributor Author

dakshvar22 commented Jul 29, 2021 •
edited
Loading

@kedz I think you have raised a good point. Hiding E2E turns might be a good idea however I think we should think more about this problem and potential solutions when either of these features are promoted to first class features. Otherwise, we'll add a lot of code for interaction between two experimental features without being sure about either which does not seem like the best idea. Right now, we won't have a lot of users trying out both the features together and it's unlikely that a lot of people run into this issue and not sure to what extent. I've created a separate issue for this so that we don't lose track of it. Most likely it will be deprioritized for now since it's not a bug and will probably picked up when we are revisiting the feature. Would you be okay with that?

@dakshvar22 dakshvar22 mentioned this pull request Jul 29, 2021
Closed
@kedz
Copy link
Contributor

kedz commented Jul 29, 2021

@dakshvar22 💯 Thanks for creating the issue!

@dakshvar22 dakshvar22 merged commit 55dee81 into 2.8.x Jul 29, 2021
@dakshvar22 dakshvar22 deleted the unexpected_e2e branch July 29, 2021 17:32
aeshky added a commit that referenced this pull request Aug 6, 2021
@aeshky @joejuzl
@wochinge @dakshvar22 @m-vdb
Merge 2.8.2 to main (#9285)
970cef7 
* Add ability to point to a certificate in endpoints.yml (#9118)

* Add cert file functionality to Endpoint

* Apply suggestions from code review

types

Co-authored-by: Tobias Wochinger <[email protected]>

* update changelog

Co-authored-by: Tobias Wochinger <[email protected]>

* Fix epoch override for TEDPolicy (#9182)

* bump rasa-sdk dep and min compat version

* fix bug add tests

* add changelog

* change test cases

* remove test cases

* prepared release of version 2.8.1 (#9183)

* update rasa-sdk

* prepared release of version 2.8.1

* update lock

* Make UnexpecTEDIntentPolicy compatible with E2E data (#9203)

* bump rasa-sdk dep and min compat version

* add test and code

* add changelog and refactor method

* add test

* more test cases

* use applied_events

* 2.8.x: Remove experimental feature warning for story validation and entity roles and groups (#9237)

* I removed the experimental feature designation from the docs.

* I removed the entity roles and groups experimental feature warning message from the code, and tested it using 'rasa train' on a minimal example.

* I removed the unused import: rasa.shared.utils.common

* Adding change log files for issues 8791 and 8024.

* Rewording change logs to make it clear that the behaviour of the features remains unchanged.

* prepared release of version 2.8.2 (#9262)

* prepared release of version 2.8.2

* Updated the date of release.

* install yarn dependency in cloned repository, in docs publication workflow

* silence yarn warning

* Fix typo in push_docs_to_branch.sh

* Generated a new poetry lock file

Co-authored-by: Joe Juzl <[email protected]>
Co-authored-by: Tobias Wochinger <[email protected]>
Co-authored-by: Daksh Varshneya <[email protected]>
Co-authored-by: m-vdb <[email protected]>
ErickGiffoni pushed a commit to FGA-GCES/rasa that referenced this pull request Aug 31, 2021
@aeshky @joejuzl
@wochinge @dakshvar22 @m-vdb @ErickGiffoni
Merge 2.8.2 to main (RasaHQ#9285)
de28d04 
* Add ability to point to a certificate in endpoints.yml (RasaHQ#9118)

* Add cert file functionality to Endpoint

* Apply suggestions from code review

types

Co-authored-by: Tobias Wochinger <[email protected]>

* update changelog

Co-authored-by: Tobias Wochinger <[email protected]>

* Fix epoch override for TEDPolicy (RasaHQ#9182)

* bump rasa-sdk dep and min compat version

* fix bug add tests

* add changelog

* change test cases

* remove test cases

* prepared release of version 2.8.1 (RasaHQ#9183)

* update rasa-sdk

* prepared release of version 2.8.1

* update lock

* Make UnexpecTEDIntentPolicy compatible with E2E data (RasaHQ#9203)

* bump rasa-sdk dep and min compat version

* add test and code

* add changelog and refactor method

* add test

* more test cases

* use applied_events

* 2.8.x: Remove experimental feature warning for story validation and entity roles and groups (RasaHQ#9237)

* I removed the experimental feature designation from the docs.

* I removed the entity roles and groups experimental feature warning message from the code, and tested it using 'rasa train' on a minimal example.

* I removed the unused import: rasa.shared.utils.common

* Adding change log files for issues 8791 and 8024.

* Rewording change logs to make it clear that the behaviour of the features remains unchanged.

* prepared release of version 2.8.2 (RasaHQ#9262)

* prepared release of version 2.8.2

* Updated the date of release.

* install yarn dependency in cloned repository, in docs publication workflow

* silence yarn warning

* Fix typo in push_docs_to_branch.sh

* Generated a new poetry lock file

Co-authored-by: Joe Juzl <[email protected]>
Co-authored-by: Tobias Wochinger <[email protected]>
Co-authored-by: Daksh Varshneya <[email protected]>
Co-authored-by: m-vdb <[email protected]>
ErickGiffoni pushed a commit to FGA-GCES/rasa that referenced this pull request Sep 9, 2021
@aeshky @joejuzl
@wochinge @dakshvar22 @m-vdb @ErickGiffoni
Merge 2.8.2 to main (RasaHQ#9285)
255d53b 
* Add ability to point to a certificate in endpoints.yml (RasaHQ#9118)

* Add cert file functionality to Endpoint

* Apply suggestions from code review

types

Co-authored-by: Tobias Wochinger <[email protected]>

* update changelog

Co-authored-by: Tobias Wochinger <[email protected]>

* Fix epoch override for TEDPolicy (RasaHQ#9182)

* bump rasa-sdk dep and min compat version

* fix bug add tests

* add changelog

* change test cases

* remove test cases

* prepared release of version 2.8.1 (RasaHQ#9183)

* update rasa-sdk

* prepared release of version 2.8.1

* update lock

* Make UnexpecTEDIntentPolicy compatible with E2E data (RasaHQ#9203)

* bump rasa-sdk dep and min compat version

* add test and code

* add changelog and refactor method

* add test

* more test cases

* use applied_events

* 2.8.x: Remove experimental feature warning for story validation and entity roles and groups (RasaHQ#9237)

* I removed the experimental feature designation from the docs.

* I removed the entity roles and groups experimental feature warning message from the code, and tested it using 'rasa train' on a minimal example.

* I removed the unused import: rasa.shared.utils.common

* Adding change log files for issues 8791 and 8024.

* Rewording change logs to make it clear that the behaviour of the features remains unchanged.

* prepared release of version 2.8.2 (RasaHQ#9262)

* prepared release of version 2.8.2

* Updated the date of release.

* install yarn dependency in cloned repository, in docs publication workflow

* silence yarn warning

* Fix typo in push_docs_to_branch.sh

* Generated a new poetry lock file

Co-authored-by: Joe Juzl <[email protected]>
Co-authored-by: Tobias Wochinger <[email protected]>
Co-authored-by: Daksh Varshneya <[email protected]>
Co-authored-by: m-vdb <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kedz kedz kedz approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@dakshvar22 @kedz