Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add verify stories and rules actions in domain to rasa data validate #8645

Merged
merged 16 commits into from
May 18, 2021

Conversation

ancalita
Copy link
Member

Proposed changes:

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)

@ancalita ancalita requested a review from wochinge May 10, 2021 10:27
@ancalita ancalita linked an issue May 10, 2021 that may be closed by this pull request
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where can I find a clear Definition of Done for the issue? It's unclear to me what items need to be covered.

changelog/7799.bugfix.md Outdated Show resolved Hide resolved
rasa/validator.py Outdated Show resolved Hide resolved
rasa/validator.py Show resolved Hide resolved
rasa/validator.py Outdated Show resolved Hide resolved
rasa/validator.py Outdated Show resolved Hide resolved
tests/test_validator.py Outdated Show resolved Hide resolved
tests/test_validator.py Outdated Show resolved Hide resolved
@ancalita
Copy link
Member Author

ancalita commented May 12, 2021

@wochinge Indeed, there is no clear Definition of Done in the issue, I worked my way through the problems listed on the issue thread, which were:

  • verify that actions in both rules and stories are in the domain
  • verify that form slots use the slots defined in the domain

There was mentioned something about adding validation for MultiProjectImporter but since that implements the TrainingDataImporter interface, I reasoned that the validator should work with it too so I don't need to action anything?
I can clarify Definition of Done in the issue thread, let me know what you think.

@ancalita ancalita force-pushed the rasa-validate-7799 branch from dc883de to 71ca0a0 Compare May 12, 2021 11:54
@ancalita ancalita changed the base branch from 2.6.x to main May 12, 2021 11:54
Copy link
Contributor

@wochinge wochinge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was mentioned something about adding validation for MultiProjectImporter but since that implements the TrainingDataImporter interface,

Unfortunately the usage of RasaFileImporter is hardcoded here. It would require replacing with this

tests/test_validator.py Outdated Show resolved Hide resolved
rasa/validator.py Show resolved Hide resolved
rasa/validator.py Outdated Show resolved Hide resolved
tests/test_validator.py Outdated Show resolved Hide resolved
@ancalita ancalita requested a review from wochinge May 17, 2021 14:45
@ancalita
Copy link
Member Author

Unfortunately the usage of RasaFileImporter is hardcoded here. It would require replacing with this

In the end, I've decided that adding support for MultiProjectImporter should be tackled separately and prioritised accordingly once it's decided if to keep feature as experimental - I've quickly tested making the change you suggested but it leads to many failed tests in test_rasa_data.py because config is empty in these tests. WDYT?

@wochinge
Copy link
Contributor

sounds good! Do you create a new issue or do we keep track of the remaining work in the old issue?


if event.action_name not in self.domain.action_names_or_texts:
rasa.shared.utils.io.raise_warning(
f"The action '{event.action_name}' is used in your stories, but it "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what if it's a rule?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works for rules too, I can rephrase to 'stories or rules'?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe there is also an attribute on the story steps which states which one it is. How about also including the story / rule name in the warning so the training story / rule is easier to find for them?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've had a look, but source_name attribute shows the entire path of the file, so instead I used block_name attribute to refer to the title of the story or rule. I'll push the changes once I amend the test assertions too :)

rasa/validator.py Show resolved Hide resolved
tests/test_validator.py Outdated Show resolved Hide resolved
tests/test_validator.py Outdated Show resolved Hide resolved
@ancalita
Copy link
Member Author

sounds good! Do you create a new issue or do we keep track of the remaining work in the old issue?

I'll create a new issue for this.

@ancalita ancalita requested a review from wochinge May 17, 2021 15:46
@ancalita ancalita merged commit 1c5e35d into main May 18, 2021
@ancalita ancalita deleted the rasa-validate-7799 branch May 18, 2021 13:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Does Rasa Data Validate not check Rules.yml/Stories.yml?
2 participants