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

Unload unused models from server memory #909

Merged
merged 6 commits into from
Mar 23, 2018
Merged

Unload unused models from server memory #909

merged 6 commits into from
Mar 23, 2018

Conversation

ricwo
Copy link
Contributor

@ricwo ricwo commented Mar 15, 2018

Proposed changes:

  • only ever keep one model per project in the server's memory under Project._models
  • unload previous NLU model from Project when a new model is requested by the server

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

@wrathagom
Copy link
Contributor

I haven't reviewed the code, but as I read the description this would be a big change for anyone using multiple fixed model names under the same project, which seems to be a common practice.

@ricwo
Copy link
Contributor Author

ricwo commented Mar 15, 2018

This only affects the server's in-memory models. The change means the server's project_store only contains the latest-used model, otherwise if you keep switching models you might run out of memory.

You can of course store as many models as you like, but when you switch them (i.e. parse requests a different model than the previous parse) they are newly loaded into memory 🙂

@ricwo ricwo changed the title Unload models Unload unused models from memory Mar 15, 2018
@ricwo ricwo changed the title Unload unused models from memory Unload unused models from server memory Mar 15, 2018
@wrathagom
Copy link
Contributor

In our use case, and several others I am aware of, we use projects as individual bots and serve multiple models simultaneously from within the project. For example, we may have a pizza bot project and then a model inside for each language English, Spanish, German, etc. We manage our own versions with a version number suffixed to the model name.

Just trying to make you aware that what I am describing is a pretty common use case that would have to be re-designed by the implementation of your proposed method.

@tmbo @amn41 any input.

@amn41
Copy link
Contributor

amn41 commented Mar 16, 2018

thanks for the feedback @wrathagom ! and great that you keep an eye on the issues before we break anything :)

the case we're trying to guard against is that if you are continuously running a server and training models, the old ones never get unloaded and Rasa's memory footprint will monotonically increase.

I'm sure there's some logic we can implement that supports both use cases, possibly adding an auto_onload flag to the server config, or coming up with another scheme for loading / unloading

@ricwo
Copy link
Contributor Author

ricwo commented Mar 20, 2018

I have made some changes that should also accommodate your use case, @wrathagom:

  • fixed model names (those not beginning with model_) are unloaded iff their name differs from a newer one by a version number (the model name has to end on _X or _vX )
  • default model names (model_X) are always unloaded iff the requested one differs from the one previously used

@amn41 I guess we could also consider adding a versioning parameter to the model config that increments the model name according to some versioning scheme, or even directly add a version field

@wrathagom
Copy link
Contributor

@ricwo @amn41 I understand the need for it, indeed it's been a community request for awhile. Just pointing out that there was an issue open related to this #438 , specifically having to do with exposing endpoints for managing the loading/deleting of models. If I had my way that would still be the solution, but then again we're building platforms on top of Rasa not just individual agents. Maybe my team can take the code from this PR and expand it a bit to add the endpoints and contribute that back.

I spoke a bit to soon on our specific use case as ours is actually a hash, which includes among other things the date. Small Talk_294b4d80-5295-0b20-3b79-d30f686fb700

@tmbo
Copy link
Member

tmbo commented Mar 22, 2018

I think it makes sense to do the following:

  • addd a DELETE endpoint to manually unload models (no deletion on disk, just unloading from mem) - this should be simple as we already have the code for the unloading (@ricwo let me know if there are issues)
  • list the loaded models in the status endpoint (right now we list all, so it is not clear which ones are loaded - whatever is easiest here (e.g. adding an attribute, creating a separate property next to available_projects))
  • don't do any automatic unloading

tmbo
tmbo previously requested changes Mar 23, 2018
# type: (Text, Text) -> Dict[Text]
"""Unload a model from server memory."""

project = project or RasaNLUConfig.DEFAULT_PROJECT_NAME
Copy link
Member

Choose a reason for hiding this comment

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

should we use a default here?

Copy link
Member

Choose a reason for hiding this comment

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

lets move this default to the endpoint level


try:
unloaded_model = self.project_store[project].unload(model)
return {"unloaded_model": unloaded_model}
Copy link
Member

Choose a reason for hiding this comment

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

I think it is fine to just return the model without the "unloaded_model" wrapper, so just return unloaded_model


def _list_loaded_models(self):
return [
model for model, interpreter in self._models.items() if interpreter
Copy link
Member

Choose a reason for hiding this comment

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

if interpreter is not None

request.setHeader('Content-Type', 'application/json')
try:
request.setResponseCode(200)
response = self.data_router.unload_model(params.get('project'),
Copy link
Member

Choose a reason for hiding this comment

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

params.get("project", RasaNLUConfig.DEFAULT_PROJECT_NAME)

params.get('model'))
return simplejson.dumps(response)
except Exception as e:
request.setResponseCode(500)
Copy link
Member

Choose a reason for hiding this comment

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

please also log to logger

@amn41 amn41 dismissed tmbo’s stale review March 23, 2018 16:35

changes have been made :)

@amn41 amn41 merged commit 8513cf7 into master Mar 23, 2018
@amn41 amn41 deleted the unload_models branch March 23, 2018 16:35
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.

4 participants