Initial implementation of converter for training data files

[degiz]

Closes #6402

Proposed changes:

• CLI command rasa data convert {nlu|core} -f yaml to convert training data from MD to YAML

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]

does it make sense that it's rasa data convert ?

[federicotdn]

It's working nicely!
Two things I think we still need:

• Docstrings on the public methods/functions.
• Maybe one or two tests that run assertions on the contents of the converted files. But since the writers themselves are already tested maybe this isn't necessary.

(I've also added other comments)

does it make sense that it's rasa data convert ?

@wochinge Would there be other rasa data commands?

[wochinge]

@wochinge Would there be other rasa data commands?

there is already rasa data split and I believe rasa data validate

[wochinge]

Does it make sense to use os.walk (or even completely re-using some parts of data.get_core_nlu_files?

[degiz]

I chose os.listdir() over the os.walk() to avoid possible confusion for the users, in case they keep the files in different sub folders and want to experiment with the files one by one.
But I might be wrong!

[degiz]

@wochinge @federicotdn The fun part is that we already have rasa data convert 😄

usage: rasa data convert [-h] [-v] [-vv] [--quiet] {nlu} ...

positional arguments:
  {nlu}
    nlu          Converts NLU data between Markdown and json formats.

Looks like we actually need to reuse it here.

What about rasa data convert -f yaml --nlu {DIR} --core {DIR} -output ?

[degiz]

Ok, actually even like this:

rasa data convert --nlu {DIR} --core {DIR} -f yaml -output {DIR}

And add a note that we currently convert to YAML only from MD.

[degiz]

Ok, another idea:

rasa data convert nlu --data {DIR} -f yaml -out {DIR}
rasa data convert core --data {DIR} -f yaml -out {DIR}

Then we're super consistent.

[tmbo]

assuming we might also want to make changes to the domain / configuration that we might want to migrate, would it make sense to use something more general, e.g. rasa migrate?

[degiz]

rasa migrate

I like the idea! Should we probably use it once we really will migrate something? Currently it's purely about converting training data from one format to the other.

That would be actually a great tool to migrate the whole project and make sure it's 2.0 compatible.

[tmbo]

what do I need to do to make this support nlg (responses) as well?

[degiz]

NLGMarkdownReader::reads returns TrainingDataas well as MarkdownReader, so it's mostly about:

• writing tests for RasaYamlWriter making sure the conversion is correct
• extending the "convert_to_yaml" function from this review to support NLGMarkdownReader

[tmbo]

That would be actually a great tool to migrate the whole project and make sure it's 2.0 compatible.

yes that is a good point, let's separate that and allow data migration as a separate command 👍

In any case, I think it already makes sense to add some instructions to the documentation about how to use the data migration https://rasa.com/docs/rasa/next/migration-guide#rasa-110-to-rasa-20 (should be a new section, what does the user need to do?)

[degiz]

add some instructions to the documentation

That's true! I'm just waiting for this review to have an approval to be sure what the final cli syntax looks like 😄

[federicotdn]

Looks good!! Stuff we might want to add on a future PR: docstrings for the public methods/functions, and deeper testing for the convertion results (file contents).

[federicotdn]

The warning is not showing when doing rasa convert core but iterating over NLU files. Maybe the if structure can be changed like this:

Suggested change

	if MarkdownReader.is_markdown_nlu_file(source_path):
	if not is_nlu:
	continue
	_write_nlu_yaml(source_path, output_path, source_path)
	num_of_files_converted += 1
	elif not is_nlu and MarkdownStoryReader.is_markdown_story_file(source_path):
	_write_core_yaml(source_path, output_path, source_path)
	num_of_files_converted += 1
	else:
	print_warning(f"Skipped file '{source_path}'")
	if is_nlu and MarkdownReader.is_markdown_nlu_file(source_path):
	_write_nlu_yaml(source_path, output_path, source_path)
	num_of_files_converted += 1
	continue
	if not is_nlu and MarkdownStoryReader.is_markdown_story_file(source_path):
	_write_core_yaml(source_path, output_path, source_path)
	num_of_files_converted += 1
	continue
	print_warning(f"Skipped file '{source_path}'")

[degiz]

It's a little bit trickier. MarkdownStoryReader.is_markdown_story_file returns true for the NLU files 🤦
So our rasa train and other commands work only because we first check if it's NLU before Core.
So this if condition is hacky but correct.

[degiz]

docstrings for the public methods/functions

I agree, but non of the new methods are "public", I've renamed them to have "_" prefix

deeper testing for the convertion results (file contents)

Do you think we should read the files and check the actual content?

[federicotdn]

Do you think we should read the files and check the actual content?

I think it's worth it, yes. But because the writers are already tested I wouldn't consider it a top priority.

[m-vdb]

using my admin rights to merge this - everything passes except windows tests that we identified are not passing on any build at the moment