Make it possible to use `or` with slot values #9317

alwx · 2021-08-10T09:36:13Z

Proposed changes:

fix Make it possible to OR slot values - Implementation #8933

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)

ancalita

Looks great 💯 I've only left a few nit-picking comments, nothing too major.
I was also wondering if the task regarding rasa.shared.core.training_data.visualization modification in the Definition of Done was set aside? or something you have to follow-up on?

rasa/shared/core/training_data/story_reader/story_reader.py

rasa/shared/core/training_data/story_reader/story_step_builder.py

rasa/shared/core/training_data/structures.py

rasa/shared/utils/schemas/stories.yml

joejuzl

Thanks for this!
I think it would be really cool if we could test this through the TrainingDataGenerator.
So we can see that the actual trackers are created as we expect (test_common_story_reader.py is probably the most similar).

Also we need to make sure that story visualisation still works correctly (it's used by Rasa X).

rasa/shared/core/training_data/story_reader/yaml_story_reader.py

rasa/shared/utils/schemas/stories.yml

tests/shared/core/training_data/story_reader/test_yaml_story_reader.py

ancalita

Looking good 👍 I think the visualisation module also needs updating.

ancalita · 2021-08-17T09:22:54Z

tests/shared/core/training_data/story_reader/test_common_story_reader.py

+        },
+    )
+    assert tracker.events[2] == ActionExecuted("utter_default")
+    assert tracker.events[3] == SlotSet(key="name", value="joe")


There's a missing newline at the end here.
Also, would index 4 in tracker.events also be a SlotSet event, the one with value = bob?

That's a good assumption but no — however, I am not sure why. My assumption was that there should be two trackers and trackers[0].events[3] == SlotSet(key="name", value="joe") and trackers[1].events[3] == SlotSet(key="name", value="bob") but that doesn't happen.
I was thinking that's an issue but it doesn't happen with or expressions when the intents (not SlotSets) are used, and it was like that before all these changes — so I now wonder if it's the bug or I just got that all wrong.
Maybe somebody can clarify? @joejuzl or @wochinge or maybe even someone from research

Very good catch @ancalita ! It should be two trackers - one with "joe" and one with "bob". If you do the or with intents you also get two trackers. This might require some changes in the TrackerGenerator

This still needs to be handled right @alwx ?

Should be good to go now — I rebased the PR: there is no common_story_reader.py anymore, and the test to check if story steps get copied or not has already been added to test_yaml_story_reader.py (e.g. test_or_statement_with_slot_was_set).

I think the rebasing just deleted this test. We still need a test which asserts that the generator creates two trackers out of the story with the or slot. So you basically need a test which asserts that training.load_data causes 2 trackers

Finally added a test for this! (test_or_statement_story_with_or_slot_was_set)

alwx · 2021-08-18T15:44:47Z

Just checked it: slot_was_set doesn't affect the visualization at all — so nothing needs to be adapted in rasa.shared.core.training_data.visualization

joejuzl · 2021-08-30T10:02:17Z

tests/shared/core/training_data/story_reader/test_common_story_reader.py

+        },
+    )
+    assert tracker.events[2] == ActionExecuted("utter_default")
+    assert tracker.events[3] == SlotSet(key="name", value="joe")


This still needs to be handled right @alwx ?

ancalita · 2021-09-13T14:14:34Z

tests/core/test_tracker_stores.py

@@ -398,8 +398,8 @@ def test_db_url_with_query_from_endpoint_config(tmp_path: Path):
        driver: my-driver
        another: query
    """
-    f = tmp_path / "tmp_config_file.yml"
-    f.write_text(endpoint_config)
+    stories_path = tmpdir / "stories.yml"


Not sure why these modifications were needed? In addition to the code quality failings, the test also fails.

I guess it was some post-merge stuff, sorry 🙈
Removed it!

ancalita · 2021-09-13T14:16:24Z

tests/shared/core/training_data/test_structures.py

+        steps[0].as_story_string()
+        == """
+## hello world
+* slot{"slot_was_set": [{"name": "joe"}]} OR slot{"slot_was_set": [{"name": "bob"}]}


It seems that actually the CI expects this:
slot{"name": "joe"} OR slot{"name": "bob"} 🤔

ancalita · 2021-09-14T07:54:08Z

tests/shared/core/training_data/test_structures.py

+      - action: some_action
+    """
+
+    reader = YAMLStoryReader(is_used_for_training=False)


Do we need a test here when the is_used_for_training flag is True too?

It's tested in another test (test_or_statement_with_slot_was_set_is_used_for_training) — in this case it makes no sense 'cause it will break the logic.

What do you mean by it will break the logic? You mean the method as_story_string() can't be called for each of the 2 steps?

Because when you specify that is_used_for_training=True then checkpoints are getting added and trackers are getting copied (see add_stories method in StoryStepBuilder, line 110). As a result, reader.read_from_parsed_yaml(yaml_content) will create multiple trackers and calling .as_story_string on them will lead to results like that which are meaningless:

## hello world - slot{"name": "joe"} > GENR_OR_b2ae7

However, this result is important when these trackers are used for training, and that case is already tested in test_or_statement_with_slot_was_set_is_used_for_training

Great, thanks for the deep-dive!

ancalita

LGTM 🙌🏼

joejuzl · 2021-09-14T12:03:03Z

rasa/shared/core/training_data/story_reader/story_step_builder.py

+
+        Args:
+            events: Events that need to be added.
+            is_used_for_training: Identifies if it's a part of OR statement.


Do we need this anymore if we no longer support MD?

rasa/shared/core/training_data/story_reader/story_step_builder.py

joejuzl · 2021-09-14T12:04:58Z

rasa/shared/core/training_data/story_reader/story_step_builder.py

-    def _generate_checkpoint_name_for_or_statement(
-        self, messages: List[UserUttered]
-    ) -> str:
+    def _generate_checkpoint_name_for_or_statement(self, messages: List[Event]) -> str:


This method refers to messages_texts_or_intents - does this work for all events?

The __str__ method is definied for all the events so it does because it requires only that. However, the names should be changed.

rasa/shared/core/training_data/structures.py

joejuzl · 2021-09-15T08:35:46Z

tests/shared/core/training_data/story_reader/test_yaml_story_reader.py

-        ),
-    ],
-)
-async def test_story_with_retrieval_intent_warns(


Do we not expect this warning any more?

Returned it back — in this case we can actually just remove the optional argument for YAMLStoryReader to test exactly the same warning.

joejuzl · 2021-09-15T08:37:27Z

tests/shared/core/training_data/test_structures.py

-    assert len(steps) == 1
-
-    assert (
-        steps[0].as_story_string()


Is this no longer used?

It isn't. is_used_for_training=False can no longer be used so the tracker(s) generated by StoryReader can only be used for training.
It makes sense because previously the ability to generate trackers not for training purposes was linked to conversion between Markdown and Yaml.

So can we delete as_story_string() as part of removing mark down support?

It still seems to be used by the interactive learning so I would skip that for now.

So should we keep the test then?

joejuzl · 2021-09-15T08:37:32Z

tests/shared/core/training_data/test_structures.py

-        steps[0].as_story_string()
-        == """
-## hello world
-* slot{"name": "joe"} OR slot{"name": "bob"}


Same as above.

joejuzl

💯

alwx changed the title ~~Or slot was set~~ Make it possible to use or with slot values Aug 10, 2021

alwx requested a review from joejuzl August 10, 2021 12:19

alwx marked this pull request as ready for review August 10, 2021 12:19

alwx requested a review from a team as a code owner August 10, 2021 12:19

ancalita reviewed Aug 11, 2021

View reviewed changes

alwx removed the request for review from joejuzl August 12, 2021 08:07

joejuzl suggested changes Aug 13, 2021

View reviewed changes

rasa/shared/core/training_data/story_reader/yaml_story_reader.py Outdated Show resolved Hide resolved

rasa/shared/utils/schemas/stories.yml Show resolved Hide resolved

tests/shared/core/training_data/story_reader/test_yaml_story_reader.py Outdated Show resolved Hide resolved

alwx requested a review from ancalita August 17, 2021 08:48

ancalita reviewed Aug 17, 2021

View reviewed changes

alwx mentioned this pull request Aug 18, 2021

Remove Markdown training data format #9390

Merged

16 tasks

alwx requested review from joejuzl and ancalita August 18, 2021 15:43

joejuzl suggested changes Aug 30, 2021

View reviewed changes

alwx added 12 commits September 7, 2021 13:53

Schema change

3cecdfb

Kinda works but needs updates

290c283

Test story added

f2376bd

Better code style

6678ccd

Black

028b0ba

Update for test_structures.py

8d728b9

Docs changes

a930a32

Updates for tests

dd02912

Code style updates

1a668e7

Updated structures

102adfe

More tests

d18d192

Tests, tests, tests

bac6dc9

alwx force-pushed the or-slot-was-set branch from 6f77e0f to bac6dc9 Compare September 7, 2021 11:53

alwx requested a review from joejuzl September 7, 2021 12:18

alwx added 2 commits September 9, 2021 13:40

Missing test

5080214

Test test_or_statement_trackers_length

5e65bbc

alwx added 3 commits September 10, 2021 12:37

Test update

4393ca6

Black fixes

b6c63e2

Code style update

e74e9bd

alwx requested a review from wochinge September 10, 2021 10:40

ancalita reviewed Sep 13, 2021

View reviewed changes

Code style updates and fixes

afbbbb0

alwx requested a review from ancalita September 14, 2021 07:28

ancalita reviewed Sep 14, 2021

View reviewed changes

alwx requested a review from ancalita September 14, 2021 08:32

ancalita approved these changes Sep 14, 2021

View reviewed changes

Merge branch 'main' into or-slot-was-set

b27e197

joejuzl suggested changes Sep 14, 2021

View reviewed changes

alwx requested a review from joejuzl September 14, 2021 16:21

alwx added 3 commits September 14, 2021 18:22

Updated tests

223ccab

Fixes

98fedcc

Merge branch 'main' into or-slot-was-set

fb63a97

joejuzl suggested changes Sep 15, 2021

View reviewed changes

Return back test

0721763

alwx requested a review from joejuzl September 15, 2021 09:16

joejuzl approved these changes Sep 15, 2021

View reviewed changes

alwx added 2 commits September 16, 2021 12:56

Merge branch 'main' into or-slot-was-set

79169df

Merge branch 'main' into or-slot-was-set

98b57f8

alwx merged commit 83cf28e into main Sep 17, 2021

alwx deleted the or-slot-was-set branch September 17, 2021 12:11

indam23 mentioned this pull request Oct 7, 2021

Make behaviour of rules w.r.t. unmentioned featurized slots explicit and configurable #9815

Closed

Make it possible to use or with slot values #9317

Make it possible to use or with slot values #9317

Conversation

alwx commented Aug 10, 2021 • edited Loading

ancalita left a comment

Choose a reason for hiding this comment

joejuzl left a comment

Choose a reason for hiding this comment

ancalita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alwx commented Aug 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alwx Sep 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ancalita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alwx Sep 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joejuzl left a comment

Choose a reason for hiding this comment

Make it possible to use `or` with slot values #9317

Make it possible to use `or` with slot values #9317

alwx commented Aug 10, 2021 •

edited

Loading

alwx Sep 14, 2021 •

edited

Loading

alwx Sep 15, 2021 •

edited

Loading