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

Implementing the use of TrainingDataImporter in rasa test #7674

Merged
merged 1 commit into from
Jan 8, 2021

Conversation

Imod7
Copy link
Contributor

@Imod7 Imod7 commented Jan 3, 2021

Implementing the 2nd Part of the changes described in Issue 5986 related to run_evaluation.
The current PR is a continuation of this one #7539 - in the 7539 I had issues with the CLA certificate and then squash and rebase became very difficult because of merged commits so I decided to create a new one, the current one.

Proposed changes:

  • Loading test data using TrainingDataImporter in run_evaluation function.
  • Changed the Optional parameters in load_from_dict of TrainingDataImporter to make it work with the data we have in run_evaluation.
  • Instead of having 1 function, the run_nlu_test now we have 2 functions described below :
    1. run_nlu_test that runs run_nlu_test_async in the event loop.
    2. run_nlu_test_async
  • Passing directly the specific arguments to run_nlu_test_async instead of argparse.Namespace thus making it more reusable.
  • Made async the following functions :
    1. run_evaluation
    2. compare_nlu
    3. test_nlu
    4. compare_nlu_models
    5. train_nlu_async
    6. test_nlu_comparison
  • Making train_nlu_async function public so we can call it from other modules.

@Imod7 Imod7 requested a review from wochinge January 3, 2021 14:43
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 iteration! 💯 Left a few comments

rasa/train.py Outdated Show resolved Hide resolved
rasa/cli/test.py Show resolved Hide resolved
@wochinge
Copy link
Contributor

wochinge commented Jan 4, 2021

@Imod7 I don't understand this part in your proposed changes, what do you mean?

Making async a list of functions and then adding the corresponding await statements when they are called.

rasa/cli/test.py Outdated Show resolved Hide resolved
rasa/cli/test.py Outdated Show resolved Hide resolved
rasa/cli/test.py Show resolved Hide resolved
rasa/train.py Outdated Show resolved Hide resolved
rasa/__main__.py Outdated Show resolved Hide resolved
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.

Wuhuu, we're nearly there 💯 Awesome work with disentangling all the async things 🚀

rasa/cli/test.py Outdated Show resolved Hide resolved
rasa/cli/test.py Outdated Show resolved Hide resolved
rasa/cli/test.py Outdated Show resolved Hide resolved
rasa/cli/test.py Outdated Show resolved Hide resolved
rasa/cli/test.py Outdated Show resolved Hide resolved
rasa/cli/test.py Outdated Show resolved Hide resolved
rasa/cli/test.py Outdated Show resolved Hide resolved
rasa/cli/test.py Outdated Show resolved Hide resolved
rasa/cli/test.py Outdated Show resolved Hide resolved
rasa/test.py Outdated Show resolved Hide resolved
@Imod7 Imod7 force-pushed the domi-importer branch 2 times, most recently from 5e9d55e to 76677e2 Compare January 5, 2021 23:39
@Imod7
Copy link
Contributor Author

Imod7 commented Jan 5, 2021

I think I did all the last changes that you mentioned and I also removed Optional from these parameters in run_nlu_test_async :

data_path: Text,
models_path: Text,
output_dir: Text,
no_errors: bool,

because as you showed yesterday I checked the functions related to set_test_nlu_arguments and :

  • in add_nlu_data_param it adds default nlu/data path data
  • in add_model_param it adds the default models path/directory models
  • in add_test_nlu_argument_group it sets the default output directory results
  • in add_errors_success_params it sets default no_errors False

Changed the docstring again of run_nlu_test_async and run_nlu_test.
Squashed and rebased the last commit.

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.

Two more comments. Can you please also update the pull request description and convert your PR from a draft to a actual pull request?

rasa/__main__.py Outdated Show resolved Hide resolved
rasa/cli/test.py Show resolved Hide resolved
rasa/cli/test.py Show resolved Hide resolved
- Reverting all tests that were async from tests/test_train.py back to how it was & all calls of train_nlu without await statement.
- Move vars(args) as the last argument and changed the type so it is the same as the type of the function parameter where it is used.
- Replaced default values of run_nlu_test_async to None.
- Changing train_nlu_async to public & removed coroutine check (main)
- Passing directly the specific arguments to run_nlu_test_async
- Implementing the use of TrainingDataImporter in rasa test (run_evaluation)
@wochinge
Copy link
Contributor

wochinge commented Jan 7, 2021

Just re-request review once you're done and I have a final look + approve 🎉

@Imod7 Imod7 requested a review from wochinge January 7, 2021 15:13
@Imod7 Imod7 marked this pull request as ready for review January 7, 2021 15:51
@Imod7 Imod7 requested a review from a team as a code owner January 7, 2021 15:51
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 Rasa Open Source change 🚀

Two notes:

  • It's (in my opinion) ok to stay a little bit more high level in your pull request description. I can see the details in the code. In my opinion it's more important to explain why certain things where changed (e.g. why did did you have to make things async). No need to change anything in this PR, rather a suggestion for the future).
  • Either add the label ready-to-merge to the PR so that the PR is automatically merged or press the green merge button yourself once all checks passed 🎉 (which I recommend for your first PR as pressing a button is way more fun and satisfying 😄 )

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 should use TrainingDataImporter
2 participants