-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Conversation
There was a problem hiding this 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
@Imod7 I don't understand this part in your proposed changes, what do you mean?
|
There was a problem hiding this 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 🚀
5e9d55e
to
76677e2
Compare
I think I did all the last changes that you mentioned and I also removed
because as you showed yesterday I checked the functions related to
Changed the docstring again of |
There was a problem hiding this 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?
- 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)
Just re-request review once you're done and I have a final look + approve 🎉 |
There was a problem hiding this 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 greenmerge
button yourself once all checks passed 🎉 (which I recommend for your first PR as pressing a button is way more fun and satisfying 😄 )
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:
test data
usingTrainingDataImporter
inrun_evaluation
function.Optional
parameters inload_from_dict
of TrainingDataImporter to make it work with the data we have inrun_evaluation
.run_nlu_test
now we have 2 functions described below :run_nlu_test
that runsrun_nlu_test_async
in the event loop.run_nlu_test_async
run_nlu_test_async
instead ofargparse.Namespace
thus making it more reusable.run_evaluation
compare_nlu
test_nlu
compare_nlu_models
train_nlu_async
test_nlu_comparison
train_nlu_async
function public so we can call it from other modules.