ENG-1551 ai xplain sdk caching onboarded models pipelines and agents #389

xainaz · 2025-02-05T21:50:13Z

Also added file lock, global expiry time in seconds is configurable as an environmental variable.

ahmetgunduz

@xainaz Thanks for the PR!

I have a main issue with the current iplementation. We have separate cache methods for models, pipelines and agents. This will make very hard for us to make any change in future. Having all into one class named AixplainCache object that has more modular desigh such that the utils methods and all repetitive methods for different asset types should be part of this class.

Can you please redesign your codebase if this make sense to you. Otherwise we can discuss to find a way to make it more modular .

ahmetgunduz

@xainaz Can you please add functional test for it? i approved the rest.

thiago-aixplain · 2025-03-18T17:12:05Z

aixplain/enums/aixplain_cache.py

+
+        Args:
+            asset_type (str): Type of the asset (e.g., "models", "pipelines", "agents").
+            cache_filename (str): Filename for storing cached data.


Make this field optional. If not provided, use the asset id as the filename

Asset id or asset type?

asset ID. One file for each asset in the proper folder.

aixplain/enums/aixplain_cache.py

aixplain/modules/agent/__init__.py

kadirpekel · 2025-03-20T16:19:14Z

aixplain/enums/aixplain_cache.py

+        if cached_data is not None:
+            return self.parse_assets(cached_data)
+
+        api_key = config.TEAM_API_KEY


Better to move this fetching part to a dedicated function to make the structure more modular and testable.

kadirpekel · 2025-03-20T16:23:24Z

aixplain/enums/aixplain_cache.py

+            type=str,
+        )
+
+        assets_details = {


Better to use a dataclass, typed dict or pydantic model if present in the context instead of using arbitrary dictionaries for cache.

kadirpekel · 2025-03-20T16:24:58Z

aixplain/enums/aixplain_cache.py

+            return Enum(self.asset_type.capitalize(), {}), {}
+
+        if "items" not in assets_data:
+            return Enum(self.asset_type.capitalize(), {}), {}


Why are we using dynamic enums such like this? What is the benefit behind it?

kadirpekel · 2025-03-20T16:28:35Z

aixplain/utils/cache_utils.py

+                if time.time() - cache_data["timestamp"] < int(get_cache_expiry()):
+                    return cache_data["data"]
+                else:
+                    return None


If we know the cache is expired, shouldn't we cleanup the cache files and also lock files in case of need?

kadirpekel · 2025-03-20T16:33:59Z

aixplain/enums/aixplain_cache.py

+    This class reduces code repetition and allows easier maintenance.
+    """
+
+    def __init__(self, asset_type: str, cache_filename: Optional[str] = None):


instead of expecting arbitrary strings here for asset_type we should either expect the real type of the asset conforming interface or at least an enumeration.

Furthermore, instead of having individual AssetCache instances for each asset type, a more generic and centralized design could be considered if applicable.

kadirpekel · 2025-03-20T16:36:37Z

aixplain/enums/aixplain_cache.py

+        Returns:
+            Tuple[Enum, Dict]: (Enum of asset IDs, Dictionary with asset details)
+        """
+        cached_data = load_from_cache(self.cache_file, self.lock_file)


I believe that load_from_cache and save_to_cache function should be an integral part of this class if we're considering this class to be main caching logic for assets

kadirpekel · 2025-03-20T16:39:10Z

aixplain/utils/cache_utils.py

-            json.dump({"timestamp": time.time(), "data": data}, f)
+        with FileLock(lock_file):
+            with open(cache_file, "w") as f:
+                json.dump({"timestamp": time.time(), "data": data}, f)
    except Exception as e:
        logging.error(f"Failed to save cache to {cache_file}: {e}")


why we're hiding the exception and just log it? If we have no valid plan or purpose for preventing an exception to raise, we should let it raise to halt the program to proceed with an unwanted data flow.

kadirpekel · 2025-03-20T16:41:59Z

aixplain/utils/cache_utils.py

+                if time.time() - cache_data["timestamp"] < int(get_cache_expiry()):
+                    return cache_data["data"]
+                else:
+                    return None


Where are we doing the house keeping? I mean if we here know that the cache expired, shouldn't we delete the waste?

kadirpekel · 2025-03-20T16:49:51Z

aixplain/modules/model/__init__.py

@@ -91,9 +91,28 @@ def __init__(
            model_params (Dict, optional): parameters for the function.
            **additional_info: Any additional Model info to be saved
        """
+        ModelCache = AixplainCache("models", "models")
+        ModelEnum, ModelDetails = ModelCache.load_assets()


Each time we're instantiating a model class (or any other asset using the same technique) will load the entire cache again and again which is quite inefficient . The entire cache should be instantiated once and loaded elsewhere independently from asset instantiation. Cache could be lazily populated at first demand or should be prepopulated when the library loaded.

kadirpekel · 2025-03-20T16:50:25Z

aixplain/modules/pipeline/asset.py

@@ -74,6 +75,17 @@ def __init__(
            status (AssetStatus, optional): Pipeline status. Defaults to AssetStatus.DRAFT.
            **additional_info: Any additional Pipeline info to be saved
        """
+        PipelineCache = AixplainCache("pipelines", "pipelines")


Having the same concerns above.

kadirpekel · 2025-03-20T16:56:04Z

aixplain/modules/model/__init__.py

+        ModelCache = AixplainCache("models", "models")
+        ModelEnum, ModelDetails = ModelCache.load_assets()
+
+        if id in ModelDetails:


If we need the entire data to populate Model class instance, why we wouldn't directly get the related Model class instance directly from the cache?

For example below the sample code should be used in the ModelFactory

ModelCache = AixplainCache(Model) ModelCache.load_assets() # this could run once internally (populate on demand) model = ModelCache.get(id) if not model: model = Model(id=..., foo=bar)

kadirpekel · 2025-03-20T16:57:57Z

aixplain/modules/model/__init__.py

@@ -91,9 +91,28 @@ def __init__(
            model_params (Dict, optional): parameters for the function.
            **additional_info: Any additional Model info to be saved
        """
+        ModelCache = AixplainCache("models", "models")


We should separate the caching logic and the core logic. Here he have the core logic of model asset and cache should be managed externally. In our case, as we have factory classes for each asset, those factory classes should be responsible to populate instances whether from cache for not.

kadirpekel · 2025-03-20T17:01:02Z

aixplain/enums/aixplain_cache.py

+from aixplain.utils.cache_utils import save_to_cache, load_from_cache, CACHE_FOLDER
+
+
+class AixplainCache:


I won't rather such name containing the company name which is so generic. Such name like AssetCache would be more focused and pointing the purpose.

xainaz added 2 commits February 6, 2025 00:38

caching for agents, pipelines and models

292f441

formatting

6c8e3cf

xainaz requested a review from thiago-aixplain February 5, 2025 21:50

thiago-aixplain requested a review from ahmetgunduz February 6, 2025 13:53

ahmetgunduz requested changes Feb 6, 2025

View reviewed changes

xainaz and others added 2 commits February 7, 2025 15:31

Agent Cache Class added

7872aa4

removed unused imports

92d8e28

ahmetgunduz requested changes Feb 10, 2025

View reviewed changes

Solve conflict

6dec714

thiago-aixplain requested changes Mar 18, 2025

View reviewed changes

thiago-aixplain changed the title ~~Eng 1551 ai xplain sdk caching onboarded models pipelines and agents~~ ENG-1551 ai xplain sdk caching onboarded models pipelines and agents Mar 18, 2025

made requested changes and added functional tests

31d7b79

thiago-aixplain requested review from kadirpekel and thiago-aixplain March 20, 2025 15:37

kadirpekel reviewed Mar 20, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ENG-1551 ai xplain sdk caching onboarded models pipelines and agents #389

ENG-1551 ai xplain sdk caching onboarded models pipelines and agents #389

xainaz commented Feb 5, 2025

ahmetgunduz left a comment

ahmetgunduz left a comment

thiago-aixplain Mar 18, 2025

xainaz Mar 18, 2025

thiago-aixplain Mar 20, 2025

kadirpekel Mar 20, 2025

kadirpekel Mar 20, 2025

kadirpekel Mar 20, 2025

kadirpekel Mar 20, 2025

kadirpekel Mar 20, 2025 •

edited

Loading

kadirpekel Mar 20, 2025

kadirpekel Mar 20, 2025

kadirpekel Mar 20, 2025 •

edited

Loading

kadirpekel Mar 20, 2025

kadirpekel Mar 20, 2025

kadirpekel Mar 20, 2025

kadirpekel Mar 20, 2025

kadirpekel Mar 20, 2025

kadirpekel Mar 20, 2025

		from aixplain.utils.cache_utils import save_to_cache, load_from_cache, CACHE_FOLDER


		class AixplainCache:

ENG-1551 ai xplain sdk caching onboarded models pipelines and agents #389

Are you sure you want to change the base?

ENG-1551 ai xplain sdk caching onboarded models pipelines and agents #389

Conversation

xainaz commented Feb 5, 2025

ahmetgunduz left a comment

Choose a reason for hiding this comment

ahmetgunduz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadirpekel Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadirpekel Mar 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadirpekel Mar 20, 2025 •

edited

Loading

kadirpekel Mar 20, 2025 •

edited

Loading