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

use temp directory to save models trained through API #4480

Merged
merged 12 commits into from
Sep 20, 2019

Conversation

wochinge
Copy link
Contributor

Proposed changes:

  • models trained with POST /model/train now get saved in a temporary directory by
    default (instead of the models directory)

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)

@wochinge
Copy link
Contributor Author

@ricwo
Being able to specify the out with the request seems pretty dangerous to me. You could use that to overwrite any files on the server you want to, couldn't you?

output_path=rjs.get("out", tempfile.gettempdir())

I think we should remove the option to do so.

@wochinge wochinge marked this pull request as ready for review September 18, 2019 09:31
@wochinge wochinge requested a review from ricwo September 18, 2019 09:32
rasa/server.py Outdated Show resolved Hide resolved
This could be used to overwrite random files on the `rasa` server.
Hence, it's a significant security risk and has to be removed.
Copy link
Contributor

@ricwo ricwo left a comment

Choose a reason for hiding this comment

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

Looks great

@wochinge
Copy link
Contributor Author

@ricwo https://docs.python.org/3/library/tempfile.html#tempfile.gettempdir stays the same over multiple runs. It could only lead to problems, when running multiple rasa instances on one machine, but I think we can neglect that for now (the worst case is that a training is done although it could have been skipped). What do you mean?

@ricwo
Copy link
Contributor

ricwo commented Sep 18, 2019

yes that sounds good, let's use that - alternatively we could introduce a boolean save_to_rasa_default_model_dir which defaults to True. that way the default behaviour would be unchanged. when firing training requests from rasa X we would set it to False. what do you think?

@wochinge
Copy link
Contributor Author

Yep, good idea! Will do the change tomorrow to rasa and X

@wochinge wochinge requested review from erohmensing and ricwo and removed request for erohmensing September 19, 2019 12:08
Copy link
Contributor

@ricwo ricwo left a comment

Choose a reason for hiding this comment

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

Looks great 🚀 Thanks for making this backwards-compatible

rasa/server.py Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
CHANGELOG.rst Outdated Show resolved Hide resolved
@wochinge wochinge merged commit e7285e6 into 1.3.x Sep 20, 2019
@wochinge wochinge deleted the server-training-output branch September 20, 2019 07:48
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.

2 participants