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

rasa test core: check if model was provided #6929

Merged
merged 13 commits into from
Oct 7, 2020
Merged

rasa test core: check if model was provided #6929

merged 13 commits into from
Oct 7, 2020

Conversation

tabergma
Copy link
Contributor

@tabergma tabergma commented Oct 6, 2020

Proposed changes:
Check if a model was provided when executing rasa test core. If not, print a useful error message and stop.

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)

@tabergma tabergma requested a review from wochinge October 6, 2020 10:58
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.

Thanks!
Let's also add a test for this 👍

"No model provided. Please make sure to specify the model to test with '--model'."
)
return

if isinstance(args.model, str):
model_path = rasa.cli.utils.get_validated_path(
args.model, "model", DEFAULT_MODELS_PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

The default value doesn't make sense then, does it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does. The errors happens if args.model = [None].

Copy link
Contributor

Choose a reason for hiding this comment

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

but [None] already means that there is no model in the DEFAULT_MODELS_PATH, no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. The default value of --model for rasa test core is set to the following: https://github.com/RasaHQ/rasa/blob/master/rasa/cli/arguments/test.py#L155
In case there is no model folder, it is set to [None]. As it is a list of length 1 we extract the first value with is None. As None is not of type str it goes into the else part, which failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it never got to this line here. This line is used, for example, in rasa test nlu where you can just have one model path (no list) and this check is working as expected.

@wochinge wochinge added this to the 2.0 Rasa Open Source milestone Oct 6, 2020
@rasabot rasabot merged commit a3e09f4 into 2.0.x Oct 7, 2020
@rasabot rasabot deleted the args-model-none branch October 7, 2020 01:27
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.

None yet

3 participants