- Notifications
You must be signed in to change notification settings - Fork 60
[ODSC-52993] Set up caching for listing models and compartments #592
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
Changes from all commits
c1a0e30 86421f7 d117663 36d4100 127ef8d e66505b e92903e 367f9ff File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| | @@ -3,6 +3,8 @@ | |
| # Copyright (c) 2024 Oracle and/or its affiliates. | ||
| # Licensed under the Universal Permissive License v 1.0 as shown at https://oss.oracle.com/licenses/upl/ | ||
| | ||
| from urllib.parse import urlparse | ||
| from tornado.web import HTTPError | ||
| from ads.aqua.decorator import handle_exceptions | ||
| from ads.aqua.extension.base_handler import AquaAPIhandler | ||
| from ads.aqua.model import AquaModelApp | ||
| | @@ -14,7 +16,6 @@ class AquaModelHandler(AquaAPIhandler): | |
| @handle_exceptions | ||
| def get(self, model_id=""): | ||
| """Handle GET request.""" | ||
| | ||
| if not model_id: | ||
| return self.list() | ||
| return self.read(model_id) | ||
| | @@ -24,6 +25,16 @@ def read(self, model_id): | |
| """Read the information of an Aqua model.""" | ||
| return self.finish(AquaModelApp().get(model_id)) | ||
| | ||
| @handle_exceptions | ||
| def delete(self, id=""): | ||
| """Handles DELETE request for clearing cache""" | ||
| url_parse = urlparse(self.request.path) | ||
| paths = url_parse.path.strip("/") | ||
| if paths.startswith("aqua/model/cache"): | ||
| Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't it be better to move the paths into the const variable? At least all of them will be in one place. Member Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. agree. Added this to pending items and will update all paths in a separate PR. | ||
| return self.finish(AquaModelApp().clear_model_list_cache()) | ||
| else: | ||
| raise HTTPError(400, f"The request {self.request.path} is invalid.") | ||
| | ||
| @handle_exceptions | ||
| def list(self): | ||
| """List Aqua models.""" | ||
| | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| | @@ -5,6 +5,9 @@ | |
| | ||
| from oci.exceptions import ServiceError | ||
| from oci.identity.models import Compartment | ||
| from datetime import datetime, timedelta | ||
| from threading import Lock | ||
| from cachetools import TTLCache | ||
| | ||
| from ads.aqua import logger | ||
| from ads.aqua.base import AquaApp | ||
| | @@ -30,6 +33,11 @@ class AquaUIApp(AquaApp): | |
| """ | ||
| | ||
| _compartments_cache = TTLCache( | ||
| maxsize=10, ttl=timedelta(hours=2), timer=datetime.now | ||
| ) | ||
| _cache_lock = Lock() | ||
| | ||
| def list_log_groups(self, **kwargs) -> str: | ||
| """Lists all log groups for the specified compartment or tenancy. This is a pass through the OCI list_log_groups | ||
| API. | ||
| | @@ -72,16 +80,10 @@ def list_logs(self, **kwargs) -> str: | |
| log_group_id=log_group_id, **kwargs | ||
| ).data.__repr__() | ||
| | ||
| def list_compartments(self, **kwargs) -> str: | ||
| """Lists the compartments in a compartment specified by TENANCY_OCID env variable. This is a pass through the OCI list_compartments | ||
| def list_compartments(self) -> str: | ||
VipulMascarenhas marked this conversation as resolved. Show resolved Hide resolved | ||
| """Lists the compartments in a tenancy specified by TENANCY_OCID env variable. This is a pass through the OCI list_compartments | ||
| API. | ||
| Parameters | ||
| ---------- | ||
| kwargs | ||
| Keyword arguments, such as compartment_id, | ||
| for `list_compartments <https://docs.oracle.com/en-us/iaas/tools/python/2.119.1/api/logging/client/oci.identity.IdentityClient.html#oci.identity.IdentityClient.list_compartments>`_ | ||
| Returns | ||
| ------- | ||
| str: | ||
| | @@ -93,6 +95,13 @@ def list_compartments(self, **kwargs) -> str: | |
| f"TENANCY_OCID must be available in environment" | ||
| " variables to list the sub compartments." | ||
| ) | ||
| | ||
| if TENANCY_OCID in self._compartments_cache.keys(): | ||
| logger.info( | ||
| f"Returning compartments list in {TENANCY_OCID} from cache." | ||
| ) | ||
| return self._compartments_cache.get(TENANCY_OCID) | ||
| | ||
| compartments = [] | ||
| # User may not have permissions to list compartment. | ||
| try: | ||
| | @@ -132,7 +141,13 @@ def list_compartments(self, **kwargs) -> str: | |
| 0, | ||
| Compartment(id=TENANCY_OCID, name=" ** Root - Name N/A **"), | ||
| Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That what we want to show to user if they don't have permissions? Member Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wasn't sure if it should be either empty or return the tenancy ocid. My thinking was that they should be able to see the root compartment at least. | ||
| ) | ||
| return compartments.__repr__() | ||
| # convert the string of the results flattened as a dict | ||
| res = compartments.__repr__() | ||
VipulMascarenhas marked this conversation as resolved. Show resolved Hide resolved | ||
| | ||
| # cache compartment results | ||
| self._compartments_cache.__setitem__(key=TENANCY_OCID, value=res) | ||
| | ||
| return res | ||
| | ||
| # todo : update this once exception handling is set up | ||
| except ServiceError as se: | ||
| | @@ -150,6 +165,25 @@ def get_default_compartment(self) -> dict: | |
| logger.error("No compartment id found from environment variables.") | ||
| return dict(compartment_id=COMPARTMENT_OCID) | ||
| | ||
| def clear_compartments_list_cache(self) -> dict: | ||
| """Allows caller to clear compartments list cache | ||
| Returns | ||
| ------- | ||
| dict with the key used, and True if cache has the key that needs to be deleted. | ||
| """ | ||
| res = {} | ||
| logger.info(f"Clearing list_compartments cache") | ||
| with self._cache_lock: | ||
| if TENANCY_OCID in self._compartments_cache.keys(): | ||
| self._compartments_cache.pop(key=TENANCY_OCID) | ||
| res = { | ||
| "key": { | ||
| "tenancy_ocid": TENANCY_OCID, | ||
| }, | ||
| "cache_deleted": True, | ||
| } | ||
| return res | ||
| | ||
| def list_model_version_sets(self, **kwargs) -> str: | ||
| """Lists all model version sets for the specified compartment or tenancy. | ||
| | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| | @@ -163,7 +163,7 @@ pii = [ | |
| "spacy==3.6.1", | ||
| ] | ||
| llm = ["langchain>=0.0.295", "evaluate>=0.4.0"] | ||
| aqua = ["fire"] | ||
| aqua = ["fire", "cachetools"] | ||
| Member There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to check license for this. Member Author There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did a search on slack and seems a lot of projects are using it, still need to confirm. | ||
| | ||
| [project.urls] | ||
| "Github" = "https://github.com/oracle/accelerated-data-science" | ||
| | ||
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.
Not sure if we need this :) User will not be able to reach out this end point. If only we add supporting refresh button in UI which might be a good idea.
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.
agree- depends on what we want to start with first. If we just use it for service models, then it makes sense not to have it. For custom models, we'll need it.