feat: Fast enum cache updates #1094
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| logger.debug("Stale actions: %s", actions_to_delete) | ||
|
|
||
| for action_name in actions_to_delete: | ||
| (enums.base.ACTIONS_CACHE / action_name).unlink() |
There was a problem hiding this comment.
Consider adding error handling for the case where the file unlink operation fails. This could happen due to permission issues or if the file is locked by another process.
python/composio/client/utils.py
Outdated
| (enums.base.ACTIONS_CACHE / action_name).unlink() | ||
|
|
||
| if actions_to_update: | ||
| # TODO: handle page size limit |
There was a problem hiding this comment.
The TODO comment about page size limit is important. Consider adding a maximum batch size for actions_to_update and implementing pagination to avoid potential memory issues with large sets of actions.
| @@ -1392,6 +1392,14 @@ def search_for_a_task( | |||
| for task in response.json().get("items", []) | |||
| ] | |||
|
|
|||
| def list_enums(self) -> list[str]: | |||
There was a problem hiding this comment.
Consider adding a docstring to the list_enums method explaining its purpose, return type, and any potential exceptions it might raise.
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 6c30c56 in 1 minute and 41 seconds
More details
- Looked at
251lines of code in5files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. python/composio/client/collections.py:1084
- Draft comment:
The change fromt.Sequencetot.Collectionfor theactions,apps, andtagsparameters is appropriate as it allows for more flexible input types.t.Collectionis a more general type thant.Sequence. - Reason this comment was not posted:
Confidence changes required:0%
The change fromt.Sequencetot.Collectionis appropriate for thegetmethod parameters sincet.Collectionis more general and includest.Sequence. This change allows for more flexibility in the types of collections that can be passed to the method.
2. python/composio/client/collections.py:1395
- Draft comment:
Thelist_enumsmethod is a good addition for fetching a list of enums from the server, ensuring up-to-date data is retrieved. - Reason this comment was not posted:
Confidence changes required:0%
Thelist_enumsmethod inActionsclass is a new addition that fetches a list of enums from the server. This method is used in theEnum.itermethod to replace the local cache reading logic, which is a good change for fetching up-to-date data.
3. python/composio/client/enums/base.py:127
- Draft comment:
Thecreate_actionfunction centralizes the creation ofActionDataobjects, reducing code duplication and improving maintainability. - Reason this comment was not posted:
Confidence changes required:0%
Thecreate_actionfunction centralizes the creation ofActionDataobjects, which is a good refactoring step. It reduces code duplication and makes the codebase easier to maintain.
4. python/composio/client/enums/enum.py:93
- Draft comment:
Theitermethod now useslist_enumsfrom theActionsclass to fetch enum names from the server, ensuring up-to-date information. - Reason this comment was not posted:
Confidence changes required:0%
Theitermethod in theEnumclass has been updated to use thelist_enumsmethod from theActionsclass. This change ensures that the enum names are fetched from the server, providing more up-to-date information compared to relying solely on local cache.
5. python/composio/client/utils.py:215
- Draft comment:
Thecheck_cache_refreshfunction now ensures local cache consistency with server data by updating or deleting actions as necessary, which is a good practice for data integrity. - Reason this comment was not posted:
Confidence changes required:0%
Thecheck_cache_refreshfunction now includes logic to compare local and API actions, updating or deleting as necessary. This ensures the local cache is consistent with the server data, which is a good practice for maintaining data integrity.
Workflow ID: wflow_XZhCV0jv5aZDVc00
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Code Review SummaryOverall, this PR introduces good improvements to the enum caching system with selective updates and better performance monitoring. Here's a breakdown: Strengths:✅ Improved caching mechanism with selective updates Areas for Improvement:
The changes look solid and improve the system's efficiency. Once the suggested improvements are addressed, this PR will be ready for merge. |
![entelligence-ai-pr-reviews[bot]](https://avatars.githubusercontent.com/in/932812?s=60&v=4){kind=small}
| if "appName" not in response: | ||
| return None | ||
|
|
||
| replaced_by = replacement_action_name( | ||
| response["description"], response["appName"] | ||
| ) | ||
| return ActionData( # type: ignore | ||
| name=response["name"], | ||
| app=response["appName"], | ||
| tags=response["tags"], | ||
| no_auth=( | ||
| client.http.get(url=str(client.apps.endpoint / response["appName"])) | ||
| .json() | ||
| .get("no_auth", False) | ||
| ), | ||
| is_local=False, | ||
| is_runtime=False, | ||
| shell=False, | ||
| path=self.storage_path, | ||
| replaced_by=replaced_by, | ||
| ) | ||
| return create_action(client, response, self.storage_path) | ||
|
|
||
| @property | ||
| def name(self) -> str: |
There was a problem hiding this comment.
Ensure create_action Functionality Matches Original Logic
The refactoring introduces a call to create_action, replacing a block of code that constructs an ActionData object. This change is high-risk as it may introduce logical errors if create_action does not replicate the original behavior, especially concerning the no_auth and replaced_by fields.
Actionable Steps:
- Review
create_actionImplementation: Ensure it correctly handles theno_authandreplaced_byfields as the original code did. - Test Edge Cases: Verify that all edge cases covered by the original logic are addressed in the new function.
- Documentation: Update any relevant documentation to reflect changes in logic or function usage.
This will help maintain the integrity of the application and prevent potential data inconsistencies. 🛠️
| def list_enums(self) -> list[str]: | ||
| """Get just the app names on the server.""" | ||
| response = self._raise_if_required( | ||
| response=self.client.http.get( | ||
| str(self.endpoint / "list" / "enums"), | ||
| ) | ||
| ) | ||
| return response.text.split("\n") |
There was a problem hiding this comment.
The list_enums method splits the response text by newline characters, which might not be robust if the server returns enums with spaces or other special characters. It should use a more robust method like json.loads if the server intends to return a JSON array of enums.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def list_enums(self) -> list[str]: | |
| """Get just the app names on the server.""" | |
| response = self._raise_if_required( | |
| response=self.client.http.get( | |
| str(self.endpoint / "list" / "enums"), | |
| ) | |
| ) | |
| return response.text.split("\n") | |
| def list_enums(self) -> list[str]: | |
| """Get just the app names on the server.""" | |
| response = self._raise_if_required( | |
| response=self.client.http.get( | |
| str(self.endpoint / "list" / "enums"), | |
| ) | |
| ) | |
| return json.loads(response.text) |
| def list_enums(self) -> list[str]: | ||
| """Get just the action names on the server""" | ||
| response = self._raise_if_required( | ||
| response=self.client.http.get( | ||
| str(self.endpoint / "list" / "enums"), | ||
| ) | ||
| ) | ||
| return response.text.split("\n") |
There was a problem hiding this comment.
Same issue as above. response.text.split is not robust and should be replaced with json.loads if the server returns a JSON array.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| def list_enums(self) -> list[str]: | |
| """Get just the action names on the server""" | |
| response = self._raise_if_required( | |
| response=self.client.http.get( | |
| str(self.endpoint / "list" / "enums"), | |
| ) | |
| ) | |
| return response.text.split("\n") | |
| def list_enums(self) -> list[str]: | |
| """Get just the action names on the server""" | |
| response = self._raise_if_required( | |
| response=self.client.http.get( | |
| str(self.endpoint / "list" / "enums"), | |
| ) | |
| ) | |
| return json.loads(response.text) |
| api_apps = client.apps.list_enums() | ||
| breakpoint() | ||
| apps_to_update = set(api_apps) - set(local_apps) | ||
| apps_to_delete = set(local_apps) - set(api_apps) | ||
| logger.debug("Apps to fetch: %s", apps_to_update) | ||
| logger.debug("Stale apps: %s", apps_to_delete) | ||
|
|
||
| # for app_name in apps_to_delete: | ||
| # (enums.base.APPS_CACHE / app_name).unlink() | ||
|
|
||
| # if apps_to_update: | ||
| # apps_data = client.http.get( | ||
| # str(client.apps.endpoint(queries={"apps": ",".join(apps_to_update)})) | ||
| # ).json() | ||
| # for app_data in apps_data["items"]: | ||
| # storage_path = enums.base.APPS_CACHE / app_data["name"] | ||
| # AppData(name=app_data["name"], path=storage_path, is_local=False).store() | ||
|
|
||
| # if actions_to_update: | ||
| # actions_data = client.http.get( | ||
| # str( | ||
| # client.actions.endpoint( | ||
| # queries={"actions": ",".join(actions_to_update)} | ||
| # ) | ||
| # ) | ||
| # ).json() | ||
| # for action_data in actions_data["items"]: | ||
| # storage_path = enums.base.ACTIONS_CACHE / action_data["name"] | ||
| # create_action( | ||
| # client, response=action_data, storage_path=storage_path | ||
| # ).store() |
There was a problem hiding this comment.
The check_cache_refresh function contains commented-out code blocks and a breakpoint. These should be removed before merging.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| api_apps = client.apps.list_enums() | |
| breakpoint() | |
| apps_to_update = set(api_apps) - set(local_apps) | |
| apps_to_delete = set(local_apps) - set(api_apps) | |
| logger.debug("Apps to fetch: %s", apps_to_update) | |
| logger.debug("Stale apps: %s", apps_to_delete) | |
| # for app_name in apps_to_delete: | |
| # (enums.base.APPS_CACHE / app_name).unlink() | |
| # if apps_to_update: | |
| # apps_data = client.http.get( | |
| # str(client.apps.endpoint(queries={"apps": ",".join(apps_to_update)})) | |
| # ).json() | |
| # for app_data in apps_data["items"]: | |
| # storage_path = enums.base.APPS_CACHE / app_data["name"] | |
| # AppData(name=app_data["name"], path=storage_path, is_local=False).store() | |
| # if actions_to_update: | |
| # actions_data = client.http.get( | |
| # str( | |
| # client.actions.endpoint( | |
| # queries={"actions": ",".join(actions_to_update)} | |
| # ) | |
| # ) | |
| # ).json() | |
| # for action_data in actions_data["items"]: | |
| # storage_path = enums.base.ACTIONS_CACHE / action_data["name"] | |
| # create_action( | |
| # client, response=action_data, storage_path=storage_path | |
| # ).store() | |
| api_apps = client.apps.list_enums() | |
| apps_to_update = set(api_apps) - set(local_apps) | |
| apps_to_delete = set(local_apps) - set(api_apps) | |
| logger.debug("Apps to fetch: %s", apps_to_update) | |
| logger.debug("Stale apps: %s", apps_to_delete) | |
| for app_name in apps_to_delete: | |
| (enums.base.APPS_CACHE / app_name).unlink() | |
| if apps_to_update: | |
| apps_data = client.http.get |
There was a problem hiding this comment.
❌ Changes requested. Incremental review on 1bb796b in 1 minute and 5 seconds
More details
- Looked at
122lines of code in3files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. python/composio/client/collections.py:357
- Draft comment:
Thelist_enumsmethod is duplicated incollections.pyandenums/base.py. Consider refactoring to avoid code duplication. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment points out potential code duplication, but I can only see the new code in collections.py, not the supposedly duplicated code in enums/base.py. Without seeing both files, I cannot verify if there is actually problematic duplication. The two list_enums() methods in collections.py serve different purposes - one gets app names and one gets action names.
I could be wrong about the two list_enums() methods serving different purposes - they have very similar implementations. Maybe the duplication is actually between these two methods within the same file?
While the implementations are similar, they hit different endpoints (/apps/list/enums vs /actions/list/enums) and return different data (app names vs action names). This appears to be an appropriate separation of concerns between the Apps and Actions classes.
The comment should be deleted because 1) I cannot verify the claimed duplication with enums/base.py since it's not in the diff, and 2) the two list_enums() methods in collections.py appropriately serve different purposes despite similar implementations.
2. python/composio/client/collections.py:1402
- Draft comment:
Thelist_enumsmethod is duplicated incollections.pyandenums/base.py. Consider refactoring to avoid code duplication. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_5Gx7bUxnl1XQsapL
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
| local_apps = list(path.stem for path in enums.base.APPS_CACHE.iterdir()) | ||
|
|
||
| api_apps = client.apps.list_enums() | ||
| breakpoint() |
There was a problem hiding this comment.
Remove the breakpoint() call. It seems to be left over from debugging and is not suitable for production code.
| logger.debug("Apps to fetch: %s", apps_to_update) | ||
| logger.debug("Stale apps: %s", apps_to_delete) | ||
|
|
||
| # for app_name in apps_to_delete: |
There was a problem hiding this comment.
Consider removing or uncommenting the code for handling apps_to_delete and apps_to_update. Leaving it commented can lead to confusion and clutter.
Important
Introduces fast enum cache updates by adding
list_enums()methods, refactoringfetch_and_cache(), and enhancing cache refresh logic.list_enums()method toAppsandActionsclasses incollections.pyto fetch enum names from the server.fetch_and_cache()inaction.pyto usecreate_action()for creatingActionData.check_cache_refresh()inutils.pyto handle cache updates more efficiently, including checking forreplaced_byfield and refreshing cache if necessary.create_action()inbase.pyto encapsulateActionDatacreation logic.get()method inActionsclass to uset.Collectioninstead oft.Sequencefor parameters.enum.pyrelated to local cache fetching.utils.py.This description was created by
for 1bb796b. It will automatically update as commits are pushed.