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

ATO-218 Always set slot values #11333

Merged
merged 3 commits into from
Jul 15, 2022
Merged

ATO-218 Always set slot values #11333

merged 3 commits into from
Jul 15, 2022

Conversation

losterloh
Copy link
Contributor

@losterloh losterloh commented Jul 13, 2022

This fixes a bug where slot values vanish when using the augmented
memoization policy. If in the past, the slot is set to a specific value,
and then later the slot being set to the same value, no new SlotSet
event is emitted. However, this leads to the augmented memoiziation
policy wrongly assuming this slot to be unset, as the older, original
SlotSet event was pruned already.

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)

@losterloh losterloh force-pushed the ATO-218-always-set-slots branch from ab526b3 to f6e87a7 Compare July 13, 2022 14:24
@losterloh losterloh changed the base branch from 3.0.x to 3.1.x July 13, 2022 14:24
@losterloh losterloh marked this pull request as ready for review July 13, 2022 14:32
@losterloh losterloh requested a review from a team as a code owner July 13, 2022 14:32
@losterloh losterloh requested review from sanchariGr and removed request for a team July 13, 2022 14:32
@losterloh losterloh force-pushed the ATO-218-always-set-slots branch from f6e87a7 to f377154 Compare July 13, 2022 15:46
Copy link
Collaborator

@sanchariGr sanchariGr left a comment

Choose a reason for hiding this comment

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

This test looks good, but some other slot tests are failing can you modify those as well?

@losterloh losterloh force-pushed the ATO-218-always-set-slots branch from f377154 to 8491f55 Compare July 14, 2022 08:53
@losterloh losterloh requested a review from a team July 14, 2022 08:53
@losterloh losterloh requested a review from a team as a code owner July 14, 2022 08:53
@losterloh losterloh requested review from dakshvar22 and aerowithanl and removed request for a team July 14, 2022 08:53
@losterloh losterloh force-pushed the ATO-218-always-set-slots branch from 8491f55 to d2948f0 Compare July 14, 2022 08:55
@losterloh losterloh changed the base branch from 3.1.x to 3.2.x July 14, 2022 08:55
@losterloh
Copy link
Contributor Author

done @sanchariGr , see current changes - CI should hopefully run green now.

@losterloh losterloh requested review from ancalita and indam23 July 14, 2022 08:59
changelog/11333.bugfix.md Outdated Show resolved Hide resolved
tests/core/test_actions.py Outdated Show resolved Hide resolved
changelog/11333.bugfix.md Outdated Show resolved Hide resolved
@losterloh losterloh requested a review from indam23 July 14, 2022 09:13
Copy link
Member

@ancalita ancalita 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 😎
Left a few suggestions, let's wait for the added AugMemo test cases examples.

tests/core/test_actions.py Show resolved Hide resolved
rasa/core/actions/action.py Outdated Show resolved Hide resolved
tests/core/test_actions.py Outdated Show resolved Hide resolved
@losterloh losterloh force-pushed the ATO-218-always-set-slots branch from eb6805d to 262a312 Compare July 14, 2022 12:27
@losterloh losterloh requested a review from ancalita July 14, 2022 13:18
This fixes a bug where slot values vanish when using the augmented
memoization policy. If in the past, the slot is set to a specific value,
and then later the slot being set to the same value, no new SlotSet
event is emitted. However, this leads to the augmented memoiziation
policy wrongly assuming this slot to be unset, as the older, original
SlotSet event was pruned already.
@losterloh losterloh force-pushed the ATO-218-always-set-slots branch from a584cc4 to 88fe676 Compare July 14, 2022 15:44
@losterloh losterloh changed the base branch from 3.2.x to 3.1.x July 14, 2022 15:44
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Amazing ⚡
Final thing, you might want to place this file into .gitignore? It's showing up in the committed files:
data/test_action_extract_slots_11333/models/.gitkeep

@losterloh
Copy link
Contributor Author

@ancalita The purpose of the .gitkeep file is so that the models directory always exists, since git only knows files and not directories. So it's there on purpose. Good idea with reducing the epochs, will do that still.

@losterloh
Copy link
Contributor Author

@ancalita reducing the epochs to 10 is actually breaking the test 😬 So I won't do that after all.

@losterloh losterloh enabled auto-merge July 14, 2022 16:01
@ancalita
Copy link
Member

@ancalita reducing the epochs to 10 is actually breaking the test 😬 So I won't do that after all.

No worries, yeah there's a sweet spot with these epochs, if you go below a level it just breaks.

@sanchariGr sanchariGr force-pushed the ATO-218-always-set-slots branch from 88fe676 to 0e74c32 Compare July 15, 2022 09:17
@sanchariGr sanchariGr force-pushed the ATO-218-always-set-slots branch from 0e74c32 to 66343c9 Compare July 15, 2022 12:47
@github-actions
Copy link
Contributor

🚀 A preview of the docs have been deployed at the following URL: https://11333--rasahq-docs-rasa-v2.netlify.app/docs/rasa

@losterloh losterloh merged commit b78194a into 3.1.x Jul 15, 2022
@losterloh losterloh deleted the ATO-218-always-set-slots branch July 15, 2022 13:48
indam23 pushed a commit that referenced this pull request Jul 18, 2022
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