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.

Sign up for GitHub

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

feat: Implement lockfiles for composio toolset #1196

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

feat: Implement lockfiles for composio toolset #1196

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a2b7d3f
feat: Implement lockfiles for composio toolset
tushar-composio Jan 15, 2025
9d3823c
undo unnecessary change
tushar-composio Jan 15, 2025
8ebb3c2
formatting
tushar-composio Jan 15, 2025
64afacb
lints
tushar-composio Jan 15, 2025
01412ef
make lockfile path a constant
tushar-composio Jan 15, 2025
be4984c
format
tushar-composio Jan 16, 2025
33d6814
ensure existing tool versions are respected
tushar-composio Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions python/composio/client/collections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,6 +1027,7 @@ class ActionModel(BaseModel):

name: str
display_name: t.Optional[str] = None
version: str
tushar-composio marked this conversation as resolved.
Show resolved Hide resolved
parameters: ActionParametersModel
response: ActionResponseModel
appName: str
Expand Down
2 changes: 2 additions & 0 deletions python/composio/tools/base/abs.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,8 @@ def _generate_schema(cls) -> None:
cls._schema = {
"name": cls.name,
"enum": cls.enum,
# TODO: get version from api
"version": "0_1",
tushar-composio marked this conversation as resolved.
Show resolved Hide resolved
tushar-composio marked this conversation as resolved.
Show resolved Hide resolved
"appName": cls.tool,
"appId": generate_app_id(cls.tool),
"tags": cls.tags(),
Expand Down
2 changes: 2 additions & 0 deletions python/composio/tools/local/handler.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ def get_action_schemas(
for schema in action_schemas:
schema["name"] = schema["enum"]

# TODO: Put version in local tool schemas
schema["version"] = "0_1"
Copy link

Choose a reason for hiding this comment

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

The hardcoded version string '0_1' should be dynamically fetched to ensure accuracy and consistency.

📝 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.

Suggested change
for schema in action_schemas:
schema["name"] = schema["enum"]
# TODO: Put version in local tool schemas
schema["version"] = "0_1"
for schema in action_schemas:
schema["name"] = schema["enum"]
# TODO: Put version in local tool schemas
schema["version"] = get_version()

Copy link
Collaborator

Choose a reason for hiding this comment

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

There's a potential bug in the get_action_schemas method in handler.py where schema["version"] is set after the loop. This should be inside the loop as it's modifying a single schema instead of all schemas in the list.

return action_schemas


Expand Down
45 changes: 45 additions & 0 deletions python/composio/tools/toolset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import typing_extensions as te
from pydantic import BaseModel
import yaml

from composio import Action, ActionType, App, AppType, TagType
from composio.client import Composio, Entity
Expand Down Expand Up @@ -198,6 +199,7 @@ def __init__(
connected_account_ids: t.Optional[t.Dict[AppType, str]] = None,
*,
max_retries: int = 3,
lock: bool = True,
**kwargs: t.Any,
) -> None:
"""
Expand Down Expand Up @@ -330,6 +332,13 @@ def _limit_file_search_response(response: t.Dict) -> t.Dict:
# composio_openai's Toolset.
self._requested_actions: t.List[str] = []

if lock:
self.locking_enabled = True
self._tool_versions = self.load_tool_versions_from_lockfile()
else:
self.locking_enabled = False
self._tool_versions = {}

def _validating_connection_ids(
self,
connected_account_ids: t.Dict[AppType, str],
Expand Down Expand Up @@ -1019,6 +1028,7 @@ def get_action_schemas(
*,
check_connected_accounts: bool = True,
_populate_requested: bool = False,
# TODO: take manual override version as parameter
) -> t.List[ActionModel]:
runtime_actions = t.cast(
t.List[t.Type[LocalAction]],
Expand Down Expand Up @@ -1054,6 +1064,7 @@ def get_action_schemas(
apps=remote_apps,
actions=remote_actions,
tags=tags,
# TODO: use tool version when fetching actions
)
if check_connected_accounts:
for item in remote_items:
Expand Down Expand Up @@ -1088,6 +1099,10 @@ def get_action_schemas(
action_names = [item.name for item in items]
self._requested_actions += action_names

if self.locking_enabled:
self._tool_versions |= {tool.name: tool.version for tool in items}
self.store_tool_versions_to_lockfile()

return items

def _process_schema(self, action_item: ActionModel) -> ActionModel:
Expand Down Expand Up @@ -1660,6 +1675,36 @@ def _validate_auth_config(

return auth_config, False

@staticmethod
def load_tool_versions_from_lockfile() -> t.Dict[str, str]:
"""Load tool versions from lockfile."""
lockfile_path = Path("./.composio.lock")
Copy link
Collaborator

Choose a reason for hiding this comment

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

The lockfile path is hardcoded as ./.composio.lock. Consider using a constant or configuration value for this path, similar to how other paths are handled in the codebase (e.g., LOCAL_CACHE_DIRECTORY). This would make it easier to change the location if needed and maintain consistency with other file paths in the codebase.

if not lockfile_path.exists():
return {}

with open(lockfile_path, "r") as file:
Copy link
Collaborator

Choose a reason for hiding this comment

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

The file operations here lack proper error handling. Consider wrapping the file operations in try-except blocks to handle potential IOError, PermissionError, or yaml.YAMLError exceptions. This would provide better error messages and prevent crashes in case of file access or parsing issues.

tool_versions = yaml.safe_load(file)
Copy link

Choose a reason for hiding this comment

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

YAML.safe_load() without exception handling can crash on malformed files; add try-except for graceful error handling.

📝 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.

Suggested change
with open(lockfile_path, "r") as file:
tool_versions = yaml.safe_load(file)
try:
with open(lockfile_path, "r") as file:
tool_versions = yaml.safe_load(file)
except yaml.YAMLError as e:
print(f"Error loading YAML file: {e}")
tool_versions = None


# Validate lockfile
if not isinstance(tool_versions, dict):
raise ComposioSDKError(
f"Invalid lockfile format, expected dict, got {type(tool_versions)}"
)

for tool in tool_versions.values():
if not isinstance(tool, str):
raise ComposioSDKError(
f"Invalid lockfile format, expected version to be string, got {tool!r}"
)

return tool_versions

def store_tool_versions_to_lockfile(self) -> None:
"""Store tool versions to lockfile."""
lockfile_path = Path("./.composio.lock")
with open(lockfile_path, "w") as file:
yaml.safe_dump(self._tool_versions, file)
Copy link

Choose a reason for hiding this comment

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

Hardcoded lockfile path can cause write failures or security issues; use a configurable path and validate directory existence. Additionally, validate the lockfile path to prevent potential file path injection vulnerabilities.

📝 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.

Suggested change
lockfile_path = Path("./.composio.lock")
if not lockfile_path.exists():
return {}
with open(lockfile_path, "r") as file:
tool_versions = yaml.safe_load(file)
# Validate lockfile
if not isinstance(tool_versions, dict):
raise ComposioSDKError(
f"Invalid lockfile format, expected dict, got {type(tool_versions)}"
)
for tool in tool_versions.values():
if not isinstance(tool, str):
raise ComposioSDKError(
f"Invalid lockfile format, expected version to be string, got {tool!r}"
)
return tool_versions
def store_tool_versions_to_lockfile(self) -> None:
"""Store tool versions to lockfile."""
lockfile_path = Path("./.composio.lock")
with open(lockfile_path, "w") as file:
yaml.safe_dump(self._tool_versions, file)
def get_tool_versions_from_lockfile(self, lockfile_path_str: str) -> dict:
"""Get tool versions from lockfile."""
try:
lockfile_path = Path(lockfile_path_str)
except TypeError:
raise ComposioSDKError("Invalid lockfile path provided.")
if not lockfile_path.is_absolute():
lockfile_path = Path.cwd() / lockfile_path
if not lockfile_path.exists():
return {}
if not lockfile_path.is_file():
raise ComposioSDKError("Lockfile path is not a file.")
with open(lockfile_path, "r") as file:
tool_versions = yaml.safe_load(file)
# Validate lockfile
if not isinstance(tool_versions, dict):
raise ComposioSDKError(
f"Invalid lockfile format, expected dict, got {type(tool_versions)}"
)
for tool in tool_versions.values():
if not isinstance(tool, str):
raise ComposioSDKError(
f"Invalid lockfile format, expected version to be string, got {tool!r}"
)
return tool_versions
def store_tool_versions_to_lockfile(self, lockfile_path_str: str) -> None:
"""Store tool versions to lockfile."""
try:
lockfile_path = Path(lockfile_path_str)
except TypeError:
raise ComposioSDKError("Invalid lockfile path provided.")
if not lockfile_path.is_absolute():
lockfile_path = Path.cwd() / lockfile_path
# Ensure the directory exists
lockfile_path.parent.mkdir(parents=True, exist_ok=True)
with open(lockfile_path, "w") as file:
yaml.safe_dump(self._tool_versions, file)

Copy link

Choose a reason for hiding this comment

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

File operations lack proper closing in error cases; use context managers consistently for cleanup.

📝 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.

Suggested change
lockfile_path = Path("./.composio.lock")
with open(lockfile_path, "w") as file:
yaml.safe_dump(self._tool_versions, file)
try:
with open(lockfile_path, "w") as file:
yaml.safe_dump(self._tool_versions, file)
except Exception as e:
print("Error writing to lockfile:", e)



def _write_file(file_path: t.Union[str, os.PathLike], content: t.Union[str, bytes]):
"""Write content to a file."""
Expand Down
22 changes: 13 additions & 9 deletions python/plugins/autogen/composio_autogen/toolset.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -242,20 +242,24 @@ def get_tools(
if processors is not None:
self._merge_processors(processors)

tools = [
tools = self.get_action_schemas(
actions=actions,
apps=apps,
tags=tags,
check_connected_accounts=check_connected_accounts,
_populate_requested=True,
)
wrapped_tools = [
self._wrap_tool(
schema=tool.model_dump(
exclude_none=True,
),
entity_id=entity_id or self.entity_id,
)
for tool in self.get_action_schemas(
actions=actions,
apps=apps,
tags=tags,
check_connected_accounts=check_connected_accounts,
_populate_requested=True,
)
for tool in tools
]

return tools
if self.locking_enabled:
self._tool_versions |= {tool.name: tool.version for tool in tools}

return wrapped_tools
Loading