Rasa role group

[tomgun132]

No description provided.

[rlinke]

@tomgun132 thanks for working on this. regarding the implementation @Sebastianwho found a small issue, where the quotation marks are missing and subsequently the generated rasa training file is corrupt/invalid.

coming back to this issue from the main repo: SimGus#48 (comment) is there a way we can incorporate your changes into the main chatette?

@SimGus: I created a fork with the fix, but am unsure how to provide tests for that, as I am unfamiliar where to best add these tests. Can you give some guidance?

the fixing commit is here: rlinke@5ff8234

this is the issue:

%[intent]
    @[testentity]("group": "1")

@[testentity]
    test

expected output:

<!-- Generated using Chatette v1.6.2 -->

## intent:intent
- [test]{"entity": "testentity", "group": "1"}

actual output:

<!-- Generated using Chatette v1.6.2 -->

## intent:intent
- [test]{"entity": "testentity, "group": "1"}

(notice the missing qutotation mark after "testentity")

[tomgun132]

Hello @rlinke,

Thank you very much for commenting on my update. I haven't been able to continue to work on this due to my real work. However, I'm planning to finish this as soon as I can. When I pull request this fork to the main repository, these changes should be implemented into the main chatette after @SimGus do a deep review on my code. Also thanks for pointing out the mistake I made on the code. I'm not sure on whether you should do a PR to my forked repository or I should do the fix myself.

Cheers

[SimGus]

Hi @rlinke,

To answer your question about testing this, here is some guidance.
The adapter you fixed is tested in tests/unit-testing/adapter/test_rasa_md.py, but the tests are not very thorough. Therefore you would need to create a new unit test for your fix.

If you don't feel confident enough don't worry, I'll create more thorough unit tests at some point, but otherwise here is what I would do in a nutshell:

• Create a new unit test that would test the method prepare_example of the adapter you changed
• Build a sample of examples to provide this method (so basically build an object containing a couple of examples you want to test the adapter on)
• Check that method prepare_example returns the correct string when provided these examples

You can find an example of such a strategy of testing about method __format_synonyms of the same adapter here.

Regardless of whether or not you decide to implement such a test, you can make a pull request with your changes to @tomgun132's branch. They will then be included in the main chatette once I merge his branch

I hope this helps :)

[tomgun132]

Hi @rlinke hope you don't mind I added the fix myself and thank you once again for pointing out the error I made.

I think I've mostly finished my part on adding Slot Role/Group and Rasa Yaml adapter format. I managed to train the generated training data with Rasa v2.2.2, though I think the recent version of Rasa doesn't change that much in term of training data format.

Also, sorry for the late finishing @SimGus, I've added a simple unit testing for the new SlotRoleGroupReference class and some files for system testing. However, I couldn't think of a simple format for the solution to test on the new slot role/group format, unless it's inside tests/unit-testing/adapter/ folder. All current tests have been passed on a linux machine and 1 error on windows machine from this line. I ignore it since it's just path format difference.

Also, I hope you don't mind that I add encoding='utf-8' to all io.open function since I mostly work on a Windows machine and
without it, I can't open/read file for Japanese language.

Next I should probably merge this to the master branch of the forked repository then create a new PR to @SimGus main repository. However, if @rlinke decides to add the unit testing, I'll hold merging the until he finish the implementation. What do you think?

[SimGus]

Hello @tomgun132,

It's really not a problem it took time, thanks again for working on this!

The tests you added/changed seem fine to me. For the error you get when running the tests on Windows, it's really not a problem as long as it passes on a Linux machine :)

About the encoding, I honestly don't remember why I decided not to specify it, even though it makes much more sense to always consider the input files as unicode encoded, so I agree with this change

If @rlinke comes back to you saying that he wants to add some unit tests, I'm all for it. Otherwise, you can open a PR to the dev branch of my repo and I'll review the changes more thoroughly (don't worry, most of it seemed alright when I took a quick look)

[rlinke]

@SimGus
I had a look at the added tests, they look fine to me. I don't know what to add to it, without testing out of scope or adding redundancy. Feel free to review/merge as per your standard.

@tomgun132 @SimGus : Thanks for working on this project.

[tomgun132]

@SimGus alright. I'll open PR to your dev branch soon. Thanks