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

Utilizing retrieval intents in test stories #9657

Merged
merged 14 commits into from
Sep 21, 2021
Merged

Conversation

jupyterjazz
Copy link
Contributor

@jupyterjazz jupyterjazz commented Sep 17, 2021

PR for issue #8459
Proposed changes:

  • Failed test stories will display full retrieval intents.
  • Retrieval intents will be extracted during action prediction in test stories so that we won't have unnecessary mismatches anymore.

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)

@jupyterjazz jupyterjazz added area:rasa-oss 🎡 Anything related to the open source Rasa framework area:rasa-oss/model-testing Issues focused around testing models (e.g. via `rasa test`) labels Sep 17, 2021
@jupyterjazz jupyterjazz requested a review from a team as a code owner September 17, 2021 08:05
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.

Great testing and clean PR 💯 Mostly questions at this point from me

data/test_retrieval_intents_in_test_stories/config.yml Outdated Show resolved Hide resolved
data/test_retrieval_intents_in_test_stories/domain.yml Outdated Show resolved Hide resolved
tests/conftest.py Outdated Show resolved Hide resolved
rasa/core/test.py Show resolved Hide resolved
tests/core/test_test.py Outdated Show resolved Hide resolved
tests/core/test_test.py Outdated Show resolved Hide resolved
tests/core/test_test.py Outdated Show resolved Hide resolved
tests/core/test_test.py Outdated Show resolved Hide resolved
intent: chitchat/ask_name
- action: utter_chitchat/ask_weather

- story: test 2 - test that works well without retrieval intents 1
Copy link
Contributor

Choose a reason for hiding this comment

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

shouldn't this one fail due to the wrong utter_goodbye?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it does fail. And I'm checking failed_test_stories.yml to see if it failed correctly.

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 fix the comment then?

rasa/core/actions/action.py Show resolved Hide resolved
tests/core/test_test.py Outdated Show resolved Hide resolved
failed_stories_path = response_selector_results / "failed_test_stories.yml"
failed_stories = read_yaml(read_file(failed_stories_path, "utf-8"))
# check that the intent is shown correctly in the failed test stories file
target_intents = {
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: This test is a bit hard to read - could we just create another test file with the expected output and then compare these files?

Copy link
Contributor Author

@jupyterjazz jupyterjazz Sep 21, 2021

Choose a reason for hiding this comment

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

You mean having the target dictionaries (target_intents and target_actions) in a file and reading it during the test?

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant having a "expected_failed_test_stories.yml" which you can just compare to the failed_test_stories.yml` from these tests by comparing the strings instead of doing the workarounds you're currently doing

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.

Great testing and rapid implementation 💯

@jupyterjazz jupyterjazz merged commit 8631348 into main Sep 21, 2021
@jupyterjazz jupyterjazz deleted the retrieval_intents-8459 branch September 21, 2021 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:rasa-oss 🎡 Anything related to the open source Rasa framework area:rasa-oss/model-testing Issues focused around testing models (e.g. via `rasa test`)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants