-
Notifications
You must be signed in to change notification settings - Fork 192
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
Model Cache Management Features #239
Conversation
FYI An example of a breaking change is when the add_documents |
Thanks for the comment. |
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.
Thanks for this PR! Left actionable feedback in the comments.
Further to dos:
- Action feedback
- Create a series of unit tests. One that I propose is to load and unload a model ~10 times, to see if the cache is actually being unloaded (else the computer runs out of memory). Please include testing for unusual and edge cases.
- Create corresponding Py-Marqo methods and a few unit tests
- Given this is a new endpoint, create a few API tests. These don't have to be as exhaustive as the unit tests, but should test every endpoint.
- An CUDA API test is proposed: load 2 CUDA models. Unload one. Test to ensure the remaining model is still cached. Test to ensure the unloaded model is indeed unloaded. Create API tests here. CUDA test classes are marked by this decorator. This ensures that the API test only runs on the CUDA test environment.
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.
There are still a few pending questions. Tests are also to be made.
I have updated my review comment
Resolved most conversations.
|
||
else: | ||
raise errors.HardwareCompatabilityError(message=str( | ||
"ERROR: cuda is not supported in your machine!!" |
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.
Can you make the error message stylistically similar to this existing one:
raise HardwareCompatabilityError(message="Requested device is not available to this Marqo instance." |
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
features
What is the current behavior? (You can also link to an open issue here)
users can not view the loaded models and eject a model from the client
What is the new behavior (if this is a feature change)?
users can now view loaded models, eject models, view cuda information from the client
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
no
Have unit tests been run against this PR? (Has there also been any additional testing?)
not yet
Related Python client changes (link commit/PR here)
will link later
Related documentation changes (link commit/PR here)
will link later
Other information:
this PR is related to the issue
Please check if the PR fulfills these requirements