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

fix conversation starts with slots initial value #7235

Merged
merged 14 commits into from Nov 25, 2020
Merged

fix conversation starts with slots initial value #7235

merged 14 commits into from Nov 25, 2020

Conversation

Ghostvv
Copy link
Contributor

@Ghostvv Ghostvv commented Nov 10, 2020 •
edited

Proposed changes:

  • fix rules with conversation starts with slots initial value

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)

@Ghostvv
Copy link
Contributor Author

Ghostvv commented Nov 10, 2020

@samsucik as agreed during debug session, could you please add tests?

@Ghostvv Ghostvv added the type:bug 🐛 Inconsistencies or issues which will cause an issue or problem for users or implementors. label Nov 16, 2020
@samsucik
Copy link
Contributor

@Ghostvv I've added 2 tests:

  1. Checking that two contradicting rules don't lead to contradiction if one of them is marked as conversation_start: true.
  2. Checking that the stuff from 1) works when a slot has an initial value set in the domain, whether or not the slot is also explicitly set at the beginning of a story on which you want to make predictions.

Does this sound right, or did you have another test case in mind?

@wochinge
Copy link
Contributor

Please change the target of this PR to master or 2.1.x once I release it this afternoon.

@Ghostvv
Copy link
Contributor Author

Ghostvv commented Nov 17, 2020

@wochinge why? we don't support 2.0 bug fixes anymore?

@wochinge
Copy link
Contributor

Yes, you can do that but then you need to take care of 1) merging these changes to master and cutting the release

Ghostvv
Ghostvv commented Nov 17, 2020
View reviewed changes
Copy link
Contributor Author

@Ghostvv Ghostvv left a comment

Choose a reason for hiding this comment

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

@samsucik that are the tests that I had in my mind. Left a couple of comments.
@wochinge could you please give it a review, since I did the original code change, it wouldn't be good if I review the PR

tests/core/policies/test_rule_policy.py Outdated Show resolved Hide resolved
tests/core/policies/test_rule_policy.py Show resolved Hide resolved
tests/core/policies/test_rule_policy.py Show resolved Hide resolved
tests/core/policies/test_rule_policy.py Outdated Show resolved Hide resolved
@wochinge
Copy link
Contributor

Hey, not sure if I can get to it today. I'm at a conference today. Will try to get it done by latest friday.

This was referenced Nov 18, 2020
Closed
Closed
@Ghostvv Ghostvv mentioned this pull request Nov 19, 2020
Merged
4 tasks
@Ghostvv
Copy link
Contributor Author

Ghostvv commented Nov 20, 2020

@wochinge would you have time to review it today, or should we ask someone else?

@wochinge
Copy link
Contributor

I'll have a look today 👍

wochinge
wochinge reviewed Nov 20, 2020
View reviewed changes
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

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

Nice, edge cases 💯

  • could you please add a changelog entry?

tests/core/policies/test_rule_policy.py Outdated Show resolved Hide resolved
tests/core/policies/test_rule_policy.py Outdated Show resolved Hide resolved
tests/core/policies/test_rule_policy.py Outdated Show resolved Hide resolved
rasa/core/policies/rule_policy.py Outdated Show resolved Hide resolved
rasa/core/policies/rule_policy.py Outdated Show resolved Hide resolved
rasa/core/policies/rule_policy.py Outdated Show resolved Hide resolved
rasa/core/policies/rule_policy.py Show resolved Hide resolved
@Ghostvv
Copy link
Contributor Author

Ghostvv commented Nov 23, 2020

@wochinge I think it is ready for final review

@Ghostvv
Copy link
Contributor Author

Ghostvv commented Nov 23, 2020

@wochinge do you have an idea why docs PR is failing? we didn't change any docs in this PR. Maybe it is failing on 2.0.x branch?

wochinge
wochinge approved these changes Nov 25, 2020
View reviewed changes
rasa/core/policies/rule_policy.py
reversed_rule_states[turn_index], conversation_state
)
)
# the rule must be applicable because we got (without any applicability issues)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice comments!

@Ghostvv
Copy link
Contributor Author

Ghostvv commented Nov 25, 2020

@samsucik after it is merged, could you please release a minor for 2.0, merge 2.0.x into 2.1.x, release a minor for 2.1, merge 2.1.x into master? sorry for the longest chain 🙈

@samsucik
Copy link
Contributor

@Ghostvv sure. This will be my first time; sounds like a good opportunity to learn something new! Wish me luck though 😄

@rasabot rasabot merged commit bb18ea4 into 2.0.x Nov 25, 2020
@rasabot rasabot deleted the fix-rules-2 branch November 25, 2020 17:04
This was referenced Nov 26, 2020
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samsucik samsucik samsucik left review comments

@wochinge wochinge wochinge approved these changes

Assignees

@samsucik samsucik

Labels
type:bug 🐛 Inconsistencies or issues which will cause an issue or problem for users or implementors.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Ghostvv @samsucik @wochinge @rasabot