Initial implementation of Core YAML parser #6001

degiz · 2020-06-11T16:53:45Z

Part of #5996

Proposed changes:

Created new YAML parser
Extracted common parts for YAML and MD to the separate classes
Added unit tests

Forms and Rules are not covered in this PR.

Things to address in the next PRs:

Update the docs
Split loaders for Core and NLU files

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)

wochinge

Love the cleanness of the implementation 🚀

What about a YAMLStoryWriter ?

data/test_yaml_stories/stories_2.yml

data/test_yaml_stories/stories_3.yml

rasa/core/training/dsl.py

rasa/core/training/story_reader/markdown_story_reader.py

rasa/core/training/story_reader/yaml_story_reader.py

tests/core/test_dsl.py

tests/core/training/story_reader/test_markdown_story_reader.py

tests/core/training/story_reader/test_yaml_story_reader.py

degiz · 2020-06-24T13:13:36Z

Thanks for the review! I'm bringing the checkpoints and or for the yaml parsers, so I'll request yet another one.

What about a YAMLStoryWriter?

As mentioned in the #5970 I plan to implement writers for nlu and stories as a separate reviews.

federicotdn

Tobias' review was already super extensive so I didn't add many comments. It's working nicely at the moment and I was able to write stories using the new format. Here's some things that we should consider changing:

If I don't prefix the user intent with /, I get three warnings for this. Though I am not sure if this is specific to this format.
When adding a new story, if I do not write story correctly (e.g. storie), the block is ignored. I think we should show a warning in this case, if the key does not correspond to any of the ones we recognize.
If I don't add steps , or write steps incorrectly, the entire story is ignored. We should show a warning in this case.
(In reference to my comment below) If I don't specify action/user/slot/etc. correctly, the step is ignored. We should should a warning for this. Maybe we can do something similar to what I suggested in the NLU PR, take one of the keys of the dict and include them in the warning.
If I don't write entities correctly in the user step, they are ignored. Maybe a warning for this would work as well.

So I guess the main improvements we could do are making sure the user knows if they have made a mistake writing stories.

rasa/core/training/loading.py

rasa/core/training/story_reader/markdown_story_reader.py

rasa/core/training/story_reader/yaml_story_reader.py

- Created new YAML parser - Extracted common parts for YAML and MD to the separate classes - Added unit tests

- Added support for `OR` and `Checkpoints` for the YAML parser - Addressing PR review comments - Added more tests

wochinge

haven't checked the tests yet, will do that separately.

Great work 🚀 💪 🎸 🥒 🌶️

data/test_mixed_yaml_md_stories/stories_part_1.yml

data/test_multifile_yaml_stories/stories_part_1.yml

data/test_yaml_stories/stories.yml

rasa/core/training/loading.py

rasa/core/training/story_reader/yaml_story_reader.py

wochinge

One more thing: I think the logger.error()s should actually be raise_warnings, no? I'm referring to the ones in yaml_story_reader

tests/core/training/story_reader/test_common_story_reader.py

tests/core/training/story_reader/test_markdown_story_reader.py

tests/core/training/story_reader/test_yaml_story_reader.py

rasa/validator.py

wochinge · 2020-06-29T14:14:10Z

stories:
- story:
  steps:
  - user: /greet
  - action: utter_greet

This breaks with the rasa init bot for me.

  File "/Users/tobias/Workspace/stack/rasa/importers/importer.py", line 242, in get_domain
    domains = await asyncio.gather(*domains)
  File "/Users/tobias/Workspace/stack/rasa/importers/rasa.py", line 64, in get_domain
    domain = Domain.load(self._domain_path)
  File "/Users/tobias/Workspace/stack/rasa/core/domain.py", line 100, in load
    other = cls.from_path(path)
  File "/Users/tobias/Workspace/stack/rasa/core/domain.py", line 110, in from_path
    domain = cls.from_file(path)
  File "/Users/tobias/Workspace/stack/rasa/core/domain.py", line 123, in from_file
    return cls.from_yaml(rasa.utils.io.read_file(path))
  File "/Users/tobias/Workspace/stack/rasa/core/domain.py", line 133, in from_yaml
    return cls.from_dict(data)
  File "/Users/tobias/Workspace/stack/rasa/core/domain.py", line 149, in from_dict
    slots = cls.collect_slots(data.get("slots", {}))
  File "/Users/tobias/Workspace/stack/rasa/core/domain.py", line 268, in collect_slots
    slot = slot_class(slot_name, **slot_dict[slot_name])
TypeError: __init__() got an unexpected keyword argument 'text'

wochinge · 2020-06-29T16:17:55Z

Ignore my comment from above. I think that was due to an experimental state my domain file was in :-D

wochinge · 2020-06-29T16:21:22Z

with

.
├── __init__.py
├── actions.py
├── config.yml
├── credentials.yml
├── data
│   ├── nlu.md
│   └── stories.yml
├── domain.yml

degiz · 2020-06-29T16:29:57Z

@wochinge yes, I see it too. Bad that I didn't add a test for that.

- Addessed the comments - Adjusted the tests

wochinge

Really great work @degiz 💪

Do you wanna add a changelog?

data/test_wrong_yaml_stories/intent_without_leading_slash.yml

data/test_wrong_yaml_stories/wrong_yaml.yml

data/test_yaml_stories/stories_3.yml

wochinge · 2020-06-30T08:24:41Z

rasa/core/training/loading.py

+        reader = MarkdownStoryReader(
+            interpreter, domain, template_variables, use_e2e, filename
+        )
+    elif PurePath(filename).suffix in YAML_FILE_EXTENSIONS:


that doesn't work? Why do we need PurePath?

Suggested change

elif PurePath(filename).suffix in YAML_FILE_EXTENSIONS:

elif Path(filename).suffix in YAML_FILE_EXTENSIONS:

Path is a child of PurePath, and the pathlib docs mention PurePath(filename).suffix as an example, I simply followed it.
But since you've found that confusing I can change that.

rasa/core/training/story_reader/yaml_story_reader.py

wochinge · 2020-06-30T08:33:01Z

rasa/core/training/story_reader/yaml_story_reader.py

+                f"Issue found in {self.source_name}: \n"
+                f'User intent "{user_utterance}" should start with '
+                f'"{INTENT_MESSAGE_PREFIX}"',
+                docs=DOCS_URL_STORIES,


we still should tell them that we skipped

rasa/core/training/story_reader/yaml_story_reader.py

rasa/data.py

degiz · 2020-06-30T09:05:57Z

Do you wanna add a changelog?

I guess yes, thanks. Even though this is only an "Initial implementation". But probably later we can simply modify the same changelog entry?

federicotdn

🎉

degiz force-pushed the 5996_new_stories_parser branch from 6058c1f to 5d45f3c Compare June 12, 2020 12:37

degiz force-pushed the 5996_new_stories_parser branch from 5d45f3c to 599dae9 Compare June 19, 2020 13:53

degiz changed the title ~~YAML parser for the Core training data~~ Initial implenemtation of Core YAML parser Jun 19, 2020

degiz force-pushed the 5996_new_stories_parser branch from 599dae9 to d277ace Compare June 22, 2020 13:41

degiz marked this pull request as ready for review June 22, 2020 13:41

degiz requested review from wochinge and federicotdn June 22, 2020 13:41

wochinge changed the title ~~Initial implenemtation of Core YAML parser~~ Initial implementation of Core YAML parser Jun 24, 2020

wochinge reviewed Jun 24, 2020

View reviewed changes

federicotdn suggested changes Jun 26, 2020

View reviewed changes

degiz force-pushed the 5996_new_stories_parser branch from d277ace to 5dddb64 Compare June 26, 2020 15:35

sravankumar2001 approved these changes Jun 26, 2020

View reviewed changes

degiz force-pushed the 5996_new_stories_parser branch 3 times, most recently from 0d16c86 to 358f67b Compare June 29, 2020 11:52

degiz requested a review from wochinge June 29, 2020 12:57

degiz force-pushed the 5996_new_stories_parser branch from 358f67b to a5dc96a Compare June 29, 2020 12:58

degiz requested a review from federicotdn June 29, 2020 12:59

degiz force-pushed the 5996_new_stories_parser branch from 4c8d562 to d89f792 Compare June 29, 2020 13:53

Alexander Khizov added 3 commits June 29, 2020 15:54

Initial implenemtation of Core YAML parser

0a1228d

- Created new YAML parser - Extracted common parts for YAML and MD to the separate classes - Added unit tests

Iteration on the PR

ede032a

- Added support for `OR` and `Checkpoints` for the YAML parser - Addressing PR review comments - Added more tests

Iteration on the PR: fix lint and types

a027513

wochinge reviewed Jun 29, 2020

View reviewed changes

degiz force-pushed the 5996_new_stories_parser branch from d89f792 to 973be61 Compare June 29, 2020 16:03

degiz force-pushed the 5996_new_stories_parser branch 2 times, most recently from c39e551 to 1c66a07 Compare June 29, 2020 19:20

Iteration on the PR

f9aab71

- Addessed the comments - Adjusted the tests

degiz force-pushed the 5996_new_stories_parser branch from 1c66a07 to f9aab71 Compare June 29, 2020 20:18

degiz requested a review from wochinge June 30, 2020 07:34

wochinge approved these changes Jun 30, 2020

View reviewed changes

degiz added the status:ready-to-merge label Jun 30, 2020

federicotdn approved these changes Jun 30, 2020

View reviewed changes

Iteration on the PR: addressed the comments

53f4f92

degiz force-pushed the 5996_new_stories_parser branch from cdeb739 to 53f4f92 Compare June 30, 2020 09:38

rasabot merged commit a706b30 into master Jun 30, 2020

rasabot deleted the 5996_new_stories_parser branch June 30, 2020 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of Core YAML parser #6001

Initial implementation of Core YAML parser #6001

degiz commented Jun 11, 2020 •

edited

Loading

wochinge left a comment

degiz commented Jun 24, 2020

federicotdn left a comment

wochinge left a comment

wochinge left a comment

wochinge commented Jun 29, 2020 •

edited

Loading

wochinge commented Jun 29, 2020

wochinge commented Jun 29, 2020 •

edited

Loading

degiz commented Jun 29, 2020

wochinge left a comment •

edited

Loading

wochinge Jun 30, 2020

degiz Jun 30, 2020

wochinge Jun 30, 2020

degiz commented Jun 30, 2020

federicotdn left a comment

	elif PurePath(filename).suffix in YAML_FILE_EXTENSIONS:
	elif Path(filename).suffix in YAML_FILE_EXTENSIONS:

Initial implementation of Core YAML parser #6001

Initial implementation of Core YAML parser #6001

Conversation

degiz commented Jun 11, 2020 • edited Loading

wochinge left a comment

Choose a reason for hiding this comment

degiz commented Jun 24, 2020

federicotdn left a comment

Choose a reason for hiding this comment

wochinge left a comment

Choose a reason for hiding this comment

wochinge left a comment

Choose a reason for hiding this comment

wochinge commented Jun 29, 2020 • edited Loading

wochinge commented Jun 29, 2020

wochinge commented Jun 29, 2020 • edited Loading

degiz commented Jun 29, 2020

wochinge left a comment • edited Loading

Choose a reason for hiding this comment

wochinge Jun 30, 2020

Choose a reason for hiding this comment

degiz Jun 30, 2020

Choose a reason for hiding this comment

wochinge Jun 30, 2020

Choose a reason for hiding this comment

degiz commented Jun 30, 2020

federicotdn left a comment

Choose a reason for hiding this comment

degiz commented Jun 11, 2020 •

edited

Loading

wochinge commented Jun 29, 2020 •

edited

Loading

wochinge commented Jun 29, 2020 •

edited

Loading

wochinge left a comment •

edited

Loading