E2e story printing #7388

wochinge · 2020-11-26T18:29:49Z

Proposed changes:

fix Make sure the e2e stories are printed correctly #7130
fix printing of end-to-end bot actions in rasa interactive
fix failing tests due to the DEFAULT_STORIES_FILE changed to be yaml
fix reading and writing of yaml stories with end-to-end tests and end-to-end training data
raise exception if end-to-end UserUttered / ActionExecuted events are tried to be printed to markdown
change fingerprinting to use yaml to make sure it works with end-to-end training data (fingerprinting is still not stable for checkpoints - however, should be fixed on master and not here
change story structure validator to use str to represent actions instead of markdown representation which uses as_story_string

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)

tests/shared/core/training_data/story_writer/test_yaml_story_writer.py

wochinge · 2020-12-02T08:37:27Z

rasa/shared/core/events.py

+        Returns:
+            Event as string.
+        """
+        if self.use_text_for_featurization:


@Ghostvv I think printing an end-to-end UserUttered event in the conversation test format will just make things worse and more complicated. I'd vote to fail early. What do you think?
I checked the usage and we should be fine except some stuff for the story validation tool which for some reason uses this.

is it used only in markdown, then ok

I'd prefer yaml writer to also use method inside the event then everything is in one place

Joe and I just had quite a session about it and the fact that as_story_string is used for varying use cases (fingerprinting, printing stories, story validation) actually makes changing code quite complicated. I prefer having the representation of an event in a specific format separated out. How an event is featurized / unique represent should be part of the Event , but it shouldn't be related to any markdown stuff.

tests/shared/core/training_data/story_writer/test_yaml_story_writer.py

rasa/shared/core/events.py

wochinge · 2020-12-02T10:48:36Z

Rasa X story printing will be broken

rasa/shared/core/events.py

rasa/test.py

rasa/shared/core/events.py

joejuzl · 2020-12-04T09:06:25Z

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

@@ -231,7 +232,7 @@ def parse_e2e_message(line: Text, is_used_for_training: bool = True) -> Message:
        intent = match.group(2)
        message = match.group(4)
        example = entities_parser.parse_training_example(message, intent)
-        if not is_used_for_training:
+        if not is_used_for_training and not self.use_e2e:


when is parse_e2e_message used but elf.use_e2e False?

when converting stories from Markdown to YAML core.training.converters.test_story_markdown_to_yaml_converter.test_test_stories was failing

joejuzl · 2020-12-04T09:14:51Z

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

@@ -389,7 +372,7 @@ def _user_intent_from_step(
    ) -> Tuple[Text, Optional[Text]]:
        user_intent = step.get(KEY_USER_INTENT, "").strip()

-        if not user_intent:
+        if not user_intent and KEY_USER_MESSAGE not in step:


Does the rest of this method gracefully handle there being no intent?

We have tests for it and they pass 🤷‍♂️

rasa/shared/core/training_data/story_writer/yaml_story_writer.py

rasa/shared/core/training_data/structures.py

tests/shared/core/test_events.py

joejuzl · 2020-12-04T17:22:09Z

LGTM

wochinge · 2020-12-07T09:25:56Z

@Ghostvv Would you mind reviewing?

rasa/core/training/story_conflict.py

Ghostvv

looks good, but it is better if someone who knows yaml writing will take a look

Ghostvv · 2020-12-07T11:39:35Z

rasa/shared/core/events.py


    def __str__(self) -> Text:
+        """Returns text representation of event."""
        return (
            f"UserUttered(text: {self.text}, intent: {self.intent}, "


I think it should be

Suggested change

f"UserUttered(text: {self.text}, intent: {self.intent}, "

f"UserUttered(text: {self.text}, intent: {self.intent_name}, "

otherwise you get all the confidences etc

I agree. We should revisit this on master once we decided on a Python code convention. Let's focus on getting e2e merged and not to fix all problems in the code.

what do you mean? I don't think its convention problem. If you use str as hash method, you'd get different events all the time, because confidences will be different

We don't use str as hash here. The hash actually uses the intent_name. __hash__ is currently not used for hashing on predictions / only on training data.

then hash is wrong, because it contains self.entities which contain confidences and extractors etc, which I don't think should be a part of the hash

fixed representation and hashing

rasa/shared/core/events.py

Ghostvv · 2020-12-07T12:05:23Z

rasa/shared/core/events.py


    def __str__(self) -> Text:
+        """Returns text representation of event."""
        return (
            f"UserUttered(text: {self.text}, intent: {self.intent}, "
            f"entities: {self.entities})"


that will contain a dict with all the confidences, extractors, etc, should we filter only entity names here?

see my answer to #7388 (comment)

wochinge · 2020-12-07T12:23:38Z

looks good, but it is better if someone who knows yaml writing will take a look

@Ghostvv I think the changes re YAML are very small in this PR. I think it'd be better to merge this now and then have @degiz review the entire changes on the e2e branch. Ok?

Can I merge then?

rasa/shared/core/events.py

Ghostvv · 2020-12-07T14:40:03Z

I'd add a TODO/create separate issue about self.entities in hash and we can merge it

Ghostvv · 2020-12-07T14:41:00Z

rasa/shared/core/events.py

@@ -377,30 +409,44 @@ def _from_parse_data(
        )

    def __hash__(self) -> int:
-        return hash((self.text, self.intent_name, jsonpickle.encode(self.entities)))
+        """Returns unique hash of object."""
+        return hash(json.dumps(self.as_sub_state()))


wochinge commented Nov 26, 2020

View reviewed changes

tests/shared/core/training_data/story_writer/test_yaml_story_writer.py Show resolved Hide resolved

wochinge added 2 commits December 1, 2020 15:30

remove unused imports

321e937

test and fix writing YAML stories

a8d8e04

wochinge force-pushed the e2e-story-printing branch from 19d82f9 to a8d8e04 Compare December 1, 2020 14:30

wochinge added 8 commits December 1, 2020 15:47

move MarkdownStoryWriter tests to separate file

4d1ba87

use tmp_path

dae731d

consider end-to-end stories correctly

810bc3e

fix story reading for retrieval intents

4eb2a43

fix missing renames for prepare_from_domain

32537d7

fixup for last merge in from master

ecb3355

dump story not as test story

1493ad7

fix docstring errors

12912fb

wochinge force-pushed the e2e-story-printing branch from d05f869 to 12912fb Compare December 2, 2020 07:59

wochinge added 2 commits December 2, 2020 09:31

remove unused method (not used in Rasa X either)

cbce93d

raise if printing end-to-end things in Markdown

1109185

wochinge commented Dec 2, 2020

View reviewed changes

tests/shared/core/training_data/story_writer/test_yaml_story_writer.py Show resolved Hide resolved

wochinge commented Dec 2, 2020

View reviewed changes

rasa/shared/core/events.py Outdated Show resolved Hide resolved

wochinge added 9 commits December 2, 2020 11:50

add todos

60b2dca

Merge branch 'e2e' into e2e-story-printing

330286f

fix error with entity formatting

d7f2a89

move to rasa.shared

1bfaafd

remove CoreDataImporter

76424f5

change fingerprinting to use yaml writer

4f967b3

fix tests failing due to new default story file

040dad5

adapt remaining parts to as_story_string failing if end-to-end event

8fe7306

remove as_story_string from story validator

cdc5ff6

wochinge commented Dec 2, 2020

View reviewed changes

rasa/shared/core/events.py Show resolved Hide resolved

wochinge commented Dec 2, 2020

View reviewed changes

rasa/test.py Outdated Show resolved Hide resolved

wochinge added 2 commits December 3, 2020 18:00

skip warning for end-to-end user messages in training data

c8971c9

add docs link

311684b

wochinge requested review from Ghostvv and joejuzl December 3, 2020 17:08

remove trailing whitespace

d01f4ab

joejuzl reviewed Dec 4, 2020

View reviewed changes

rasa/shared/core/events.py Outdated Show resolved Hide resolved

joejuzl reviewed Dec 4, 2020

View reviewed changes

wochinge added 2 commits December 4, 2020 17:59

return NotImplemented if other class

6d239d8

remove md_ as it's not related to md

7a34057

wochinge requested a review from joejuzl December 4, 2020 17:19

add docstrings to entire module

dcbdac1

wochinge marked this pull request as ready for review December 7, 2020 10:17

wochinge requested a review from a team December 7, 2020 10:17

add more docstrings

59c7cc0

wochinge force-pushed the e2e-story-printing branch from 6ba5d26 to 59c7cc0 Compare December 7, 2020 10:21

Ghostvv reviewed Dec 7, 2020

View reviewed changes

rasa/core/training/story_conflict.py Show resolved Hide resolved

Ghostvv reviewed Dec 7, 2020

View reviewed changes

increase timeout due to failing windows tests

97e1860

improve string representation of UserUttered

8ba989c

Ghostvv reviewed Dec 7, 2020

View reviewed changes

rasa/shared/core/events.py Outdated Show resolved Hide resolved

fix hashing of UserUttered

a1e2bee

Ghostvv approved these changes Dec 7, 2020

View reviewed changes

Ghostvv reviewed Dec 7, 2020

View reviewed changes

wochinge merged commit de9e17a into e2e Dec 7, 2020

wochinge deleted the e2e-story-printing branch December 7, 2020 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

E2e story printing #7388

E2e story printing #7388

wochinge commented Nov 26, 2020 •

edited

Loading

wochinge Dec 2, 2020

Ghostvv Dec 2, 2020

Ghostvv Dec 2, 2020

wochinge Dec 2, 2020

wochinge commented Dec 2, 2020

joejuzl Dec 4, 2020

wochinge Dec 4, 2020

joejuzl Dec 4, 2020

wochinge Dec 4, 2020

joejuzl commented Dec 4, 2020

wochinge commented Dec 7, 2020

Ghostvv left a comment

Ghostvv Dec 7, 2020

wochinge Dec 7, 2020

Ghostvv Dec 7, 2020

wochinge Dec 7, 2020

Ghostvv Dec 7, 2020

wochinge Dec 7, 2020

Ghostvv Dec 7, 2020

wochinge Dec 7, 2020

wochinge commented Dec 7, 2020

Ghostvv commented Dec 7, 2020

Ghostvv Dec 7, 2020

	f"UserUttered(text: {self.text}, intent: {self.intent}, "
	f"UserUttered(text: {self.text}, intent: {self.intent_name}, "

E2e story printing #7388

E2e story printing #7388

Conversation

wochinge commented Nov 26, 2020 • 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

wochinge commented Dec 2, 2020

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 commented Dec 4, 2020

wochinge commented Dec 7, 2020

Ghostvv 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

Choose a reason for hiding this comment

wochinge commented Dec 7, 2020

Ghostvv commented Dec 7, 2020

Choose a reason for hiding this comment

wochinge commented Nov 26, 2020 •

edited

Loading