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

Update Cloud Storage Docs #1731

Merged
merged 11 commits into from
Mar 1, 2019
Merged

Conversation

ShailChoksi
Copy link
Contributor

Proposed changes: Documentation was lacking on how the storing and retrieval of model works.
Might help with:
#1426

  • ...

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

@akelad
Copy link
Contributor

akelad commented Feb 25, 2019

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

@akelad akelad requested a review from wochinge February 25, 2019 12:56
docs/persist.rst Outdated Show resolved Hide resolved
docs/persist.rst Outdated
@@ -55,7 +55,13 @@ Rasa NLU supports using `S3 <https://aws.amazon.com/s3/>`_ and

If there is no container with the name ``AZURE_CONTAINER`` Rasa will create it.

Models are gzipped before saving to cloud.
Models are gzipped before saving to cloud. The gzipped file naming convention
is `{PROJECT}___{MODEL_NAME}.tar.gz` and should be in the root folder of the storage service.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
is `{PROJECT}___{MODEL_NAME}.tar.gz` and should be in the root folder of the storage service.
is `{PROJECT}{MODEL_NAME}.tar.gz` and it is stored in the root folder of the storage service.

Copy link
Contributor Author

@ShailChoksi ShailChoksi Feb 25, 2019

Choose a reason for hiding this comment

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

Was this behavior changed in v0.14? We are using v0.13.7 so I tested on that which has the ___ in between model and project name. @wochinge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Line 96-100 in persistor.py:

    @staticmethod
    def _project_prefix(project: Text) -> Text:

        p = project or RasaNLUModelConfig.DEFAULT_PROJECT_NAME
        return '{}___'.format(p)

docs/persist.rst Outdated Show resolved Hide resolved
docs/persist.rst Outdated Show resolved Hide resolved
docs/persist.rst Outdated Show resolved Hide resolved
@wochinge
Copy link
Contributor

@careless25 Thanks for your contribution. I left a few comments. Implement them and then the change is ready to go 🚀

@codeclimate
Copy link

codeclimate bot commented Feb 25, 2019

Code Climate has analyzed commit b43b816 and detected 0 issues on this pull request.

View more on Code Climate.

@ShailChoksi
Copy link
Contributor Author

@wochinge Addressed your comments

@@ -55,7 +55,13 @@ Rasa NLU supports using `S3 <https://aws.amazon.com/s3/>`_ and

If there is no container with the name ``AZURE_CONTAINER`` Rasa will create it.

Models are gzipped before saving to cloud.
Models are gzipped before they are saved in the cloud. The gzipped file naming convention
is `{PROJECT}___{MODEL_NAME}.tar.gz` and it is stored in the root folder of the storage service.
Copy link
Contributor

Choose a reason for hiding this comment

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

@careless25 Sure it is not {PROJECT}{MODEL_NAME}.tar.gz?

Copy link
Contributor Author

@ShailChoksi ShailChoksi Feb 26, 2019

Choose a reason for hiding this comment

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

Yes, see lines 96-100 in persistor.py
#1731 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 You know the code better than I - awesome job!

docs/persist.rst Outdated
Models are gzipped before saving to cloud.
Models are gzipped before they are saved in the cloud. The gzipped file naming convention
is `{PROJECT}___{MODEL_NAME}.tar.gz` and it is stored in the root folder of the storage service.
Currently, you are not able manually to specify the path on cloud storage.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Currently, you are not able manually to specify the path on cloud storage.
Currently, you are not able to manually specify the path on the cloud storage.

docs/persist.rst Outdated
is `{PROJECT}___{MODEL_NAME}.tar.gz` and it is stored in the root folder of the storage service.
Currently, you are not able manually to specify the path on cloud storage.

If storing trained models, Rasa NLU will gzip the new model and upload to the container. If retrieving/loading models
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
If storing trained models, Rasa NLU will gzip the new model and upload to the container. If retrieving/loading models
If storing trained models, Rasa NLU will gzip the new model and upload it to the container. If retrieving/loading models

docs/persist.rst Outdated
Currently, you are not able manually to specify the path on cloud storage.

If storing trained models, Rasa NLU will gzip the new model and upload to the container. If retrieving/loading models
from the cloud storage, Rasa NLU will download the gzipped model locally an extract the contents to the location
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
from the cloud storage, Rasa NLU will download the gzipped model locally an extract the contents to the location
from the cloud storage, Rasa NLU will download the gzipped model locally and extract the contents to the location

@wochinge
Copy link
Contributor

@careless25 Thanks! I added a few more comments!

@ShailChoksi
Copy link
Contributor Author

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.

@careless25 Awesome! Thanks for your work!

@wochinge wochinge merged commit 71a0fe2 into RasaHQ:master Mar 1, 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.

3 participants