Initial implementation of NLU YAML parser #5989

degiz · 2020-06-10T14:27:30Z

Part of #5983

Proposed changes:

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

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)

federicotdn · 2020-06-23T13:40:01Z

I started testing this by using an old folder I had initialized with rasa init --no-prompt (on master). Then, I changed nlu.md to nlu.yml, and adapted the contents to YAML. I checked the validity of the YAML just in case. I also deleted nlu.md.

However, after running rasa train there seems to be a problem when stories.md is loaded. An exception is raised:

Traceback (most recent call last):
  File "/home/fede/Workspace/rasa/env/bin/rasa", line 11, in <module>
    load_entry_point('rasa', 'console_scripts', 'rasa')()
  File "/home/fede/Workspace/rasa/rasa/__main__.py", line 108, in main
    cmdline_arguments.func(cmdline_arguments)
  File "/home/fede/Workspace/rasa/rasa/cli/train.py", line 77, in train
    nlu_additional_arguments=extract_nlu_additional_arguments(args),
  File "/home/fede/Workspace/rasa/rasa/train.py", line 52, in train
    nlu_additional_arguments=nlu_additional_arguments,
  File "uvloop/loop.pyx", line 1456, in uvloop.loop.Loop.run_until_complete
  File "/home/fede/Workspace/rasa/rasa/train.py", line 88, in train_async
    config, domain, training_files
  File "/home/fede/Workspace/rasa/rasa/importers/importer.py", line 81, in load_from_config
    config, config_path, domain_path, training_data_paths
  File "/home/fede/Workspace/rasa/rasa/importers/importer.py", line 139, in load_from_dict
    RasaFileImporter(config_path, domain_path, training_data_paths)
  File "/home/fede/Workspace/rasa/rasa/importers/rasa.py", line 36, in __init__
    training_data_paths
  File "/home/fede/Workspace/rasa/rasa/data.py", line 96, in get_core_nlu_files
    path
  File "/home/fede/Workspace/rasa/rasa/data.py", line 117, in _find_core_nlu_files_in_directory
    if is_nlu_file(full_path):
  File "/home/fede/Workspace/rasa/rasa/data.py", line 145, in is_nlu_file
    return loading.guess_format(file_path) != loading.UNK
  File "/home/fede/Workspace/rasa/rasa/nlu/training_data/loading.py", line 184, in guess_format
    content = io_utils.read_yaml_file(filename)
  File "/home/fede/Workspace/rasa/rasa/utils/io.py", line 209, in read_yaml_file
    return read_yaml(read_file(filename, DEFAULT_ENCODING))
  File "/home/fede/Workspace/rasa/rasa/utils/io.py", line 124, in read_yaml
    return yaml_parser.load(content) or {}
  File "/home/fede/Workspace/rasa/env/lib/python3.7/site-packages/ruamel/yaml/main.py", line 343, in load
    return constructor.get_single_data()
  File "/home/fede/Workspace/rasa/env/lib/python3.7/site-packages/ruamel/yaml/constructor.py", line 111, in get_single_data
    node = self.composer.get_single_node()
  File "_ruamel_yaml.pyx", line 703, in _ruamel_yaml.CParser.get_single_node
  File "_ruamel_yaml.pyx", line 904, in _ruamel_yaml.CParser._parse_next_event
ruamel.yaml.scanner.ScannerError: while scanning an alias
  in "<unicode string>", line 2, column 1
did not find expected alphabetic or numeric character
  in "<unicode string>", line 2, column 2

It appears to be a problem with _is_nlg_story_format, which is not detecting stories.md as a Markdown stories file for some reason. But that's strange since this PR does not modify that function (or the regexp it uses).

federicotdn · 2020-06-23T13:46:33Z

A simple way to reproduce: on master, run rasa init --no-prompt, then switch to this branch and do rasa train (no YAML needed).

degiz · 2020-06-23T20:16:17Z

@federicotdn thanks for spotting that!
I think locally I only tried the rasa train --nlu. Problem fixed, but I also need to make sure that situation is covered with a test 😄
Also, there's a bunch of test failing in the test_train.py, I'll check that.

federicotdn

Tested a couple of different NLU YAML files and it's working great! I have a couple of thoughts that I've written down, in no particular order:

Some warnings are appearing three times when I train. For example, if I omit the examples section of a lookup table. Not sure if this is specific to YAML.
If a user has a typo on their section name (e.g. regexp instead of regex) I think we could help them with a warning in that case.
For the warnings that start with Unexpected block found in..., maybe we could change the >> for a got: {value}, but expected a multiline string.
If I specify a list of strings under examples for an intent, an exception is raised. I think we should cover this case because users might try to write examples with lists (without metadata).
In interactive learning, I am forced to pick a file ending with .md to write the new data. I am not sure if we want to update interactive learning with this PR, maybe it's out of scope.
The rasa init command should create a nlu.yml file instead of a nlu.md file. Again, maybe not in the scope of this PR.
Some methods are missing docstrings.

rasa/data.py

rasa/nlu/featurizers/sparse_featurizer/regex_featurizer.py

rasa/nlu/training_data/entities_parser.py

rasa/nlu/training_data/formats/rasa_yaml.py

rasa/nlu/training_data/formats/readerwriter.py

rasa/nlu/training_data/loading.py

rasa/nlu/training_data/lookup_tables_parser.py

rasa/nlu/training_data/synonyms_parser.py

ricwo

I tested some basic training and it works well! when editing multiline examples, I think it's quite intuitive to provide lists instead of a multiline string. I think @federicotdn already suggested this in the review, but I think it would be awesome if we could support both.

When making YAML syntax errors, it would be very helpful if we could output some hints about the error. This could even be just the YAML exception. Right now, all the CLI shows is Path 'data' doesn't contain valid NLU data in it. Please verify the data format. The NLU model training will be skipped now.. What do you think?

rasa/nlu/training_data/entities_parser.py

tests/nlu/training_data/formats/test_rasa_yaml.py

tests/nlu/training_data/test_entities_parser.py

tests/nlu/training_data/test_training_data.py

degiz · 2020-06-27T09:56:59Z

@ricwo @federicotdn Thanks a lot for your patience and the detailed code review! I know there were obvious issues with missing docstrings and other stuff 😞 I was in a bit of hurry to get this thing working asap, sorry!

degiz · 2020-06-30T16:38:57Z

When making YAML syntax errors

@ricwo I have a plan to verify both NLU and Core YAML files against a schema. It will print a proper message like "Unexpected key..". I want to do that in a separate PR not to make this one even bigger.

provide lists instead of a multiline string

The latest decision we made was to be strict and support only multiple strings. Thanks for spotting that, I'll adjust the code and the tests.

ricwo

Great job 👍

I'm seeing some confusing warning messages when training with the example nlu.yml:

UserWarning: Unexpected key 'metadata' found in 'data/nlu.yml'.Supported keys are: 'intent', 'synonym', 'regex', 'lookup'. This key will be skipped.

and

UserWarning: Unexpected key 'examples' found in 'data/nlu.yml'.Supported keys are: 'intent', 'synonym', 'regex', 'lookup'. This key will be skipped.

It might just be a matter of adding them as elifs in rasa_yaml._parse_items()

rasa/nlu/training_data/entities_parser.py

ricwo · 2020-07-05T08:03:09Z

rasa/nlu/training_data/formats/markdown.py

@@ -151,32 +136,27 @@ def _load_files(self, line: Text) -> None:

    def _parse_item(self, line: Text) -> None:
        """Parses an md list item line based on the current section type."""
+        import rasa.nlu.training_data.lookup_tables_parser as lookup_tables_parser
+        import rasa.nlu.training_data.synonyms_parser as synonyms_parser
+


can these be module-level imports since this is called very frequently?

unfortunately there is something wrong with the cyclic import in the whole NLU package and I haven't managed yet to resolve it.

ricwo · 2020-07-05T08:03:45Z

rasa/nlu/training_data/formats/markdown.py

+        import rasa.nlu.training_data.entities_parser as entities_parser
+        import rasa.nlu.training_data.synonyms_parser as synonyms_parser


same here, can they be module-level imports or does it lead to circular imports?

same answer.

rasa/nlu/training_data/formats/rasa_yaml.py

rasa/nlu/training_data/formats/markdown.py

rasa/nlu/training_data/lookup_tables_parser.py

ricwo · 2020-07-05T08:30:33Z

tests/nlu/training_data/test_training_data.py

 def test_guess_format_from_non_existing_file_path():
    assert guess_format("not existing path") == UNK


this test could be added to the parametrized one: test_get_supported_file_format()

not really, if the path doesn't exist, the get_file_format() raises.

rasa/nlu/training_data/formats/rasa_yaml.py

federicotdn

Did some testing again! Some comments:

Getting the same as Rick for the examples and metadata keys in particular.
I would also add that warnings are appearing twice. If I add a foo key, I get twice:

/home/fede/Workspace/rasa/rasa/utils/common.py:364: UserWarning: Unexpected key 'foo' found in 'data/nlu.yml'.Supported keys are: 'intent', 'synonym', 'regex', 'lookup'. This key will be skipped.
  More info at https://rasa.com/docs/rasa/nlu/training-data-format/

Using a list of strings instead of a multiline string crashes the parser with an error that users won't understand. We could either show a warning and ignore the contents, or raise an exception with a more informative message.
If I specify a file in examples for a lookup, we are treating that as a one-line lookup. Would there be an easy way of warning the user that the string will not be interpreted as a file path?

I think there will be opportunities in the future to see where users are getting stuck, and how we can help them by adding more warnings here and there. Other than that, the parser looks ready to be merged!

rasa/nlu/training_data/formats/rasa_yaml.py

federicotdn · 2020-07-05T19:01:54Z

rasa/nlu/training_data/formats/rasa_yaml.py

+                example.get(KEY_INTENT_TEXT, "")
+                for example in examples
+                if example
+            ]


If I don't specify text for one of the entries, this defaults to "". Maybe we could show a warning saying that text needs to be specified?

That's a good point!

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

degiz changed the title ~~Initial commit~~ NL Jun 10, 2020

degiz changed the title NL YAML parser for the NLU training data Jun 10, 2020

degiz force-pushed the 5983_new_nlu_parser branch 2 times, most recently from 1611414 to ac8aae0 Compare June 17, 2020 12:27

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

degiz mentioned this pull request Jun 17, 2020

New Training Data Format - NLU #5983

Closed

degiz force-pushed the 5983_new_nlu_parser branch from ac8aae0 to bc5ac00 Compare June 17, 2020 12:40

degiz requested review from federicotdn and ricwo June 17, 2020 12:40

degiz force-pushed the 5983_new_nlu_parser branch 2 times, most recently from e808fb9 to 6ea2e8c Compare June 17, 2020 13:59

degiz marked this pull request as ready for review June 22, 2020 07:18

degiz force-pushed the 5983_new_nlu_parser branch from 6ea2e8c to 4aae5b5 Compare June 23, 2020 20:13

degiz force-pushed the 5983_new_nlu_parser branch from 4aae5b5 to 6c7af68 Compare June 24, 2020 09:21

federicotdn suggested changes Jun 24, 2020

View reviewed changes

ricwo suggested changes Jun 24, 2020

View reviewed changes

degiz force-pushed the 5983_new_nlu_parser branch 4 times, most recently from 400763f to 4b9ea9a Compare June 28, 2020 07:21

degiz force-pushed the 5983_new_nlu_parser branch 5 times, most recently from b51759c to 4f8272a Compare July 1, 2020 15:19

wochinge changed the title ~~Initial implenemtation of NLU YAML parser~~ Initial implementation of NLU YAML parser Jul 2, 2020

degiz force-pushed the 5983_new_nlu_parser branch 5 times, most recently from a2f8a17 to 7e5e5b1 Compare July 2, 2020 12:53

degiz requested review from federicotdn and ricwo July 2, 2020 13:33

ricwo approved these changes Jul 5, 2020

View reviewed changes

federicotdn reviewed Jul 6, 2020

View reviewed changes

Alexander Khizov added 2 commits July 6, 2020 11:45

Initial implenemtation of NLU YAML parser

10f749d

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

Fixed bug with an YAML exception not being processed

7a36bed

degiz force-pushed the 5983_new_nlu_parser branch from 7e5e5b1 to 48bd067 Compare July 6, 2020 09:45

degiz requested a review from federicotdn July 6, 2020 09:46

federicotdn approved these changes Jul 6, 2020

View reviewed changes

degiz force-pushed the 5983_new_nlu_parser branch from 48bd067 to 8043094 Compare July 6, 2020 09:57

degiz added the status:ready-to-merge label Jul 6, 2020

Addressing PR comments

2d33c46

degiz force-pushed the 5983_new_nlu_parser branch from 8043094 to 2d33c46 Compare July 6, 2020 11:21

Merge branch 'master' into 5983_new_nlu_parser

e10a4e6

rasabot merged commit dcb9878 into master Jul 6, 2020

rasabot deleted the 5983_new_nlu_parser branch July 6, 2020 12:19

This was referenced Jan 15, 2021

Support for writing back NLU intent/example metadata to YAML #7731

Closed

Support for writing NLU intent/example metadata to YAML #7761

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial implementation of NLU YAML parser #5989

Initial implementation of NLU YAML parser #5989

degiz commented Jun 10, 2020 •

edited

Loading

federicotdn commented Jun 23, 2020

federicotdn commented Jun 23, 2020

degiz commented Jun 23, 2020

federicotdn left a comment

ricwo left a comment

degiz commented Jun 27, 2020

degiz commented Jun 30, 2020

ricwo left a comment

ricwo Jul 5, 2020

degiz Jul 6, 2020

ricwo Jul 5, 2020

degiz Jul 6, 2020

ricwo Jul 5, 2020

degiz Jul 6, 2020

federicotdn left a comment

federicotdn Jul 5, 2020

degiz Jul 6, 2020

		import rasa.nlu.training_data.entities_parser as entities_parser
		import rasa.nlu.training_data.synonyms_parser as synonyms_parser

		def test_guess_format_from_non_existing_file_path():
		assert guess_format("not existing path") == UNK

Initial implementation of NLU YAML parser #5989

Initial implementation of NLU YAML parser #5989

Conversation

degiz commented Jun 10, 2020 • edited Loading

federicotdn commented Jun 23, 2020

federicotdn commented Jun 23, 2020

degiz commented Jun 23, 2020

federicotdn left a comment

Choose a reason for hiding this comment

ricwo left a comment

Choose a reason for hiding this comment

degiz commented Jun 27, 2020

degiz commented Jun 30, 2020

ricwo 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

federicotdn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

degiz commented Jun 10, 2020 •

edited

Loading