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

Fixed crash when --domain not provided for rasa train command #3914

Merged
merged 12 commits into from
Jul 8, 2019
Merged

Fixed crash when --domain not provided for rasa train command #3914

merged 12 commits into from
Jul 8, 2019

Conversation

RanaMostafaAbdElMohsen
Copy link
Contributor

@RanaMostafaAbdElMohsen RanaMostafaAbdElMohsen commented Jul 2, 2019

Fixes #3794

Proposed changes:

  • An Error message will be displayed in case of invalid domain file provided
    or no domain file provided at all for rasa train core only
  • One more additional check added to --domain argument

Tests:

  • [CORE] Provide Invalid domain file path [Error Message Displayed]
    python rasa/rasa train core --domain rasa/examples/formbot/domain1.yml --config rasa/examples/formbot/config.yml --stories rasa/examples/formbot/data/ --out rasa/examples/formbot/models --augmentation 0 --quiet

  • [CORE] No domain file provided [Error Message Displayed]
    python rasa/rasa train core --config rasa/examples/formbot/config.yml --stories rasa/examples/formbot/data/ --out rasa/examples/formbot/models --augmentation 0 --quiet

  • [CORE] Provided valid domain file [PASS]
    python rasa/rasa train core --domain rasa/examples/formbot/domain.yml --config rasa/examples/formbot/config.yml --stories rasa/examples/formbot/data/ --out rasa/examples/formbot/models --augmentation 0 --quiet

  • [NLU] No domain file provided [PASS]
    python rasa/rasa train nlu --config rasa/examples/formbot/config.yml --nlu rasa/examples/formbot/data/ --out rasa/examples/formbot/models

Status (please check what you already did):

  • made PR ready for code review
  • added some tests for the functionality
  • updated the documentation
  • updated the changelog
  • reformat files using black (please check Readme for instructions)

An Error message will be displayed in case of invalid domain file provided
or no domain file provided at all for 'rasa train core' only
@CLAassistant
Copy link

CLAassistant commented Jul 2, 2019

CLA assistant check
All committers have signed the CLA.

@akelad
Copy link
Contributor

akelad commented Jul 5, 2019

Thanks for submitting this PR, we'll give it a review as soon as possible!

@wochinge wochinge requested a review from erohmensing July 6, 2019 11:35
- Additional changes for nlu/metadata.json to make sure nlu has been trained
Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

Hi @RanaMostafaAbdElMohsen! A few things before I make a thorough review:

  1. Now that there have been some updates to the command (I believe due to a PR for making sure temp directories get deleted), would you please solve the merge conflicts?
  2. Your first test makes sense -- if the provided domain is invalid, it should raise an error, not break. However, if you do rasa train core without providing a domain (2nd test case), in my opinion, the ideal behavior is that it would look for domain.yml, like rasa train and rasa train nlu does. Can you make that happen?

BTW, thanks for making your first PR well-tested and super descriptive! Looking forward to adding you as a code contributor. 😄

@RanaMostafaAbdElMohsen
Copy link
Contributor Author

RanaMostafaAbdElMohsen commented Jul 7, 2019

Hi @erohmensing ,
Thanks for your feedback. I would like to bring your attention to two of test cases in
testing directory :

tests/cli/test_rasa_train.py

test case 1 :

test_train_core_temp_files

test case 2 :

test_train_nlu_temp_files

These two test cases failed locally on my machine; however, the test cases pass on travis CI.
The test case fails due to function count_rasa_temp_files()

It fails due to permission error
for f in os.listdir(entry.path): PermissionError: [Errno 13] Permission denied: '/tmp/systemd-private'
I believe a bug should be fired to handle this missing scenario in case of permission denied files

@RanaMostafaAbdElMohsen
Copy link
Contributor Author

RanaMostafaAbdElMohsen commented Jul 7, 2019

@erohmensing ,
Referring to point number 2 that you have mentioned it in your previous comment for 2nd testcase.
In fact, it already searches for domain.yml but in its command working directory but not in passed directory as --config argument and --stories argument. It already mimics same behaviour as rasa train core and rasa train.

Here are some test scenarios in case we made it look for domain.yml in same directory passed for --config and --stories argument which user may find rather misleading :

  • Problem here that user may pass different directories in command for --config and--stories as test scenario. So in that case, the domain file will be searched in which directory?!

  • Another test scenario, what if directory passed for --config and --stories are not found, the domain will be searched in this unfound directory

  • Another test scenario which I find it very convincing, what if user wants to run domain.yml file that is found to be existing in command working directory but not the other domain file referenced at directory passed for --config and --stories argument.

I believe that the best course of action is that if not referenced directory for domain.yml, it will search for it in current working directory same as rasa train core and rasa train commands which is already covered.

@erohmensing
Copy link
Contributor

erohmensing commented Jul 8, 2019

I believe that the best course of action is that if not referenced directory for domain.yml, it will search for it in current working directory same as rasa train core and rasa train commands which is already covered.

I totally agree, and I understand what you mean now. I was thinking in the case of the working dir being the one with domain.yml in it, but obviously that's not the case. Thanks for the clarification!

Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

Functionality looks good! Just a few comments on the error/warning messages.

rasa/train.py Outdated Show resolved Hide resolved
rasa/train.py Outdated Show resolved Hide resolved
rasa/train.py Outdated Show resolved Hide resolved
rasa/train.py Outdated Show resolved Hide resolved
@erohmensing
Copy link
Contributor

@federicotdn can you look into what he mentioned above about the temp dir tests? #3914 (comment)

@federicotdn
Copy link
Contributor

federicotdn commented Jul 8, 2019

@erohmensing fixed! Take a look: #3958
@RanaMostafaAbdElMohsen you would need to update your branch after that PR is merged. Sorry for the inconvenience.

@RanaMostafaAbdElMohsen
Copy link
Contributor Author

hello @erohmensing ,
I have resolved all comments that you have advised.
Please review it.

Copy link
Contributor

@erohmensing erohmensing left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the fix! Congrats on being a code contributor!

Edit: Oops, I'm sorry I forgot one last thing. Can you please update the changelog? It should go under fixed -- something like
``rasa train core`` no longer crashes without a ``--domain`` arg

(the double ticks are .rst notation for this style) I'd fix it myself but I can't write to your repo. Almost got too merge-happy 😅

@RanaMostafaAbdElMohsen
Copy link
Contributor Author

Awesome, thanks for the fix! Congrats on being a code contributor!

Edit: Oops, I'm sorry I forgot one last thing. Can you please update the changelog? It should go under fixed -- something like
rasa train core no longer crashes without a --domain arg

(the double ticks are .rst notation for this style) I'd fix it myself but I can't write to your repo. Almost got too merge-happy 😅

Thanks @erohmensing for your help.
Done.

@erohmensing
Copy link
Contributor

Sorry, it should go under unreleased master version, not 1.1.4. Sorry for the confusion, thanks for responding so fast! (in the future, allowing people with write access to edit your contribution makes small things like this a lot easier!)

@RanaMostafaAbdElMohsen
Copy link
Contributor Author

RanaMostafaAbdElMohsen commented Jul 8, 2019

Sorry, it should go under unreleased master version, not 1.1.4. Sorry for the confusion, thanks for responding so fast! (in the future, allowing people with write access to edit your contribution makes small things like this a lot easier!)

Sorry for wrong placement. Done :D
When is it expected to be merged @erohmensing ?

@erohmensing erohmensing merged commit 7097a41 into RasaHQ:master Jul 8, 2019
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 train command error message
5 participants