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

for "rasa test core --evaluate-model-directory" set default for "--model" to directory #5122

Merged
merged 15 commits into from
Jan 28, 2020

Conversation

chkoss
Copy link
Contributor

@chkoss chkoss commented Jan 23, 2020

Proposed changes:
If rasa test core is called with --evaluate-model-directory and a file as --model, use the folder that file is in as --model instead.

This fixes #4896 : When no --model argument is given, instead of defaulting to the latest model in ./models it now defaults to ./models

Side effect: When called with "--model some_folder/some_file.tar.gz", it uses some_folder as --model instead. Is this something we want? If yes, should the code throw a warning like "you provided a file as --model, will use the folder that file is in instead"?

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)

…e as --model, use the folder that file is in as --model instead
@claassistantio
Copy link

claassistantio commented Jan 23, 2020

CLA assistant check
All committers have signed the CLA.

@chkoss chkoss requested a review from wochinge January 23, 2020 14:34
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.

Congrats to your first PR 🎉

Regarding the side-effect which you mentioned (thanks for pointing this ambiguous behavior out!):
how about checking the passed model is != model.get_latest_model(directory) (the function is in the module rasa.model) and then print a warning. In case the passed model name is == model.get_latest_model(directory) we can assume it's the default model and wasn't explicitly passed in by the user.

rasa/cli/test.py Outdated Show resolved Hide resolved
@chkoss chkoss requested a review from wochinge January 28, 2020 11:01
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.

Nice work with all the pytest fixtures! 💯

rasa/test.py Outdated Show resolved Hide resolved
rasa/test.py Show resolved Hide resolved
rasa/test.py Show resolved Hide resolved
tests/test_test.py Outdated Show resolved Hide resolved
tests/test_test.py Show resolved Hide resolved
tests/test_test.py Outdated Show resolved Hide resolved
tests/test_test.py Show resolved Hide resolved
tests/test_test.py Outdated Show resolved Hide resolved
tests/test_test.py Outdated Show resolved Hide resolved
tests/test_test.py Outdated Show resolved Hide resolved
@chkoss chkoss requested a review from wochinge January 28, 2020 15:44
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.

Great work 🎉 See the open comments, but looks very great!

rasa/test.py Outdated Show resolved Hide resolved
rasa/test.py Outdated Show resolved Hide resolved
rasa/test.py Outdated Show resolved Hide resolved
rasa/test.py Outdated Show resolved Hide resolved
@chkoss chkoss merged commit 55caf93 into master Jan 28, 2020
@chkoss chkoss deleted the issue4896 branch January 28, 2020 21:31
@wochinge
Copy link
Contributor

🎉

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.

"rasa test core --evaluate-model-directory" should have directory as default
3 participants