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

[ATO 490] Rasa train doesn't throw an error if form in slot mappings active loop doesn't exist #12324

Conversation

Urkem
Copy link
Contributor

@Urkem Urkem commented Apr 24, 2023

Proposed changes:

  • Implement ATO-490 by adding flags --validate-before-training, --fail-on-validation-warnings and --validation-max-history to rasa train to bring it inline with rasa data validate

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)

@Urkem Urkem changed the title [ATO 490] Rasa train doesn't throw an error if form in slot mappings active loop doesn't exist [ATO 490] Rasa train doesn't throw an error if form in slot mappings if active loop doesn't exist Apr 24, 2023
@Urkem Urkem changed the title [ATO 490] Rasa train doesn't throw an error if form in slot mappings if active loop doesn't exist [ATO 490] Rasa train doesn't throw an error if form in slot mappings active loop doesn't exist Apr 24, 2023
@Urkem Urkem requested review from a team and radovanZRasa and removed request for a team April 25, 2023 09:36
@Urkem Urkem marked this pull request as ready for review April 25, 2023 09:37
@Urkem Urkem requested a review from a team as a code owner April 25, 2023 09:37
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Looks great 🎉 Left some suggestions for improvements.
I'd also recommend outlining this solution to Product and CSE to get their stamp of approval (for the overall solution of giving flexibility to run validation checks via additional flags).

rasa/cli/interactive.py Outdated Show resolved Hide resolved
rasa/cli/train.py Show resolved Hide resolved
tests/cli/test_utils.py Outdated Show resolved Hide resolved
tests/cli/test_utils.py Outdated Show resolved Hide resolved
tests/cli/test_rasa_train.py Show resolved Hide resolved
changelog/12324.improvement.md Outdated Show resolved Hide resolved
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 4 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Urkem Urkem requested a review from ancalita April 28, 2023 10:36
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏻 All my previous comments have been addressed, I only asked a clarifying question on a unit test.

tests/cli/test_rasa_train.py Show resolved Hide resolved
Urkem added 2 commits May 3, 2023 12:13
…esnt-throw-an-error-if-form-in-slot-mappings-active-loop-doesnt-exist
@@ -135,6 +135,10 @@ these, `rasa train` will fall back to one of these commands by default.
The name of the model by default is `<timestamp>.tar.gz`. If you want to name your model differently,
you can specify the name using the `--fixed-model-name` flag.

By default validation is run before training the model. If you want to skip validation, you can use the `--skip-validation` flag.
If you want to fail on validation warnings, you can use the `--fail-on-validation-warnings` flag.
The --validation-max-history is analogous to the `--max-history` argument of `rasa data validate`.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
The --validation-max-history is analogous to the `--max-history` argument of `rasa data validate`.
The `--validation-max-history` is analogous to the `--max-history` argument of `rasa data validate`.

Copy link
Member

Choose a reason for hiding this comment

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

@ancalita ancalita self-requested a review May 4, 2023 07:47
Copy link
Member

@ancalita ancalita left a comment

Choose a reason for hiding this comment

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

Looks great 💯 Only two tiny comments that should be addressed before merging.

@@ -135,6 +135,10 @@ these, `rasa train` will fall back to one of these commands by default.
The name of the model by default is `<timestamp>.tar.gz`. If you want to name your model differently,
you can specify the name using the `--fixed-model-name` flag.

By default validation is run before training the model. If you want to skip validation, you can use the `--skip-validation` flag.
If you want to fail on validation warnings, you can use the `--fail-on-validation-warnings` flag.
The --validation-max-history is analogous to the `--max-history` argument of `rasa data validate`.
Copy link
Member

Choose a reason for hiding this comment

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

rasa/cli/interactive.py Show resolved Hide resolved
@github-actions
Copy link
Contributor

github-actions bot commented May 4, 2023

🚀 A preview of the docs have been deployed at the following URL: https://12324--rasahq-docs-rasa-v2.netlify.app/docs/rasa

@Urkem Urkem merged commit 8e78695 into main May 4, 2023
@Urkem Urkem deleted the ATO-490-rasa-train-doesnt-throw-an-error-if-form-in-slot-mappings-active-loop-doesnt-exist branch May 4, 2023 19:07
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.

3 participants