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.

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: add together ai plugin (Py SDK) #1363

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abhishekpatil4
Copy link
Contributor

@abhishekpatil4 abhishekpatil4 commented Feb 24, 2025

Important

Add Together AI plugin for Composio to automate GitHub interactions and demonstrate email sending.

  • New Plugin:
    • Adds Together AI plugin for Composio in composio_togetherai.
    • Automates GitHub repository starring via conversational instructions.
  • Setup and Usage:
    • README.md provides installation and usage instructions.
    • setup.py configures the plugin package.
  • Core Functionality:
    • ComposioToolSet class in toolset.py for managing tool calls.
    • togetherai_demo.py demonstrates sending an email using the plugin.

This description was created by Ellipsis for 350c61c. It will automatically update as commits are pushed.

Copy link

vercel bot commented Feb 24, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
composio ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 24, 2025 6:01pm

Comment on lines +128 to +132
if isinstance(response, t.Iterator):
chunk: ChatCompletionChunk = next(response)
completion: ChatCompletionResponse = ChatCompletionResponse(
**chunk.model_dump()
)

Choose a reason for hiding this comment

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

The handle_tool_calls() method only processes the first chunk when given an iterator, discarding remaining chunks which could contain additional tool calls. Should iterate through all chunks.

📝 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
if isinstance(response, t.Iterator):
chunk: ChatCompletionChunk = next(response)
completion: ChatCompletionResponse = ChatCompletionResponse(
**chunk.model_dump()
)
if isinstance(response, t.Iterator):
completion = None
for chunk in response:
if completion is None:
completion = ChatCompletionResponse(**chunk.model_dump())
else:
completion.merge_chunk(chunk)

Comment on lines +5 to +7
client = Together(
api_key="59114908afa42e6b013964084e5b5fe52d375b8b2418e848d3f16bcee96f905a"
)

Choose a reason for hiding this comment

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

API key is hardcoded in the source code, creating a security vulnerability. Store sensitive credentials in environment variables or a secure configuration file.

📝 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
client = Together(
api_key="59114908afa42e6b013964084e5b5fe52d375b8b2418e848d3f16bcee96f905a"
)
client = Together(
api_key=os.getenv('TOGETHER_API_KEY')
)

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to 350c61c in 2 minutes and 42 seconds

More details
  • Looked at 319 lines of code in 5 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. python/plugins/togetherai/README.md:7
  • Draft comment:
    Objective in README (line 7) mentions automating GitHub repo starring while later examples (e.g. in demo) send an email. Update for consistency.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
2. python/plugins/togetherai/README.md:72
  • Draft comment:
    The code snippet uses 'pprint(result)' without importing pprint. Consider adding 'from pprint import pprint' to avoid a NameError.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    This is a documentation file showing example code. While technically correct that pprint needs to be imported, this is a minor detail in what's meant to be a high-level example. The focus is on demonstrating the Together AI integration flow, not Python import specifics. Documentation often omits obvious imports for brevity.
    The comment is technically accurate - the code would fail without the import. Missing imports could confuse beginners trying to follow the example.
    While true, README examples commonly omit basic imports for readability. The pprint import is basic Python knowledge and its omission doesn't obscure the main concepts being demonstrated.
    Delete the comment. The missing pprint import is too minor an issue for a README example where the focus is on demonstrating the Together AI integration flow.
3. python/plugins/togetherai/composio_togetherai/toolset.py:42
  • Draft comment:
    Typo in the error message: 'handelling' should be 'handling'.
  • Reason this comment was not posted:
    Marked as duplicate.
4. python/plugins/togetherai/togetherai_demo.py:6
  • Draft comment:
    Avoid hardcoding API keys in source code. Instead, use environment variables or a secure configuration mechanism to prevent accidental exposure.
  • Reason this comment was not posted:
    Marked as duplicate.
5. python/plugins/togetherai/README.md:56
  • Draft comment:
    Typographical issue: The model name is written as 'gpt-4o'. If this was meant to be 'gpt-4', please correct it to avoid confusion.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
    This is a Together AI integration example, not OpenAI. Together AI likely has its own model naming conventions. Without knowing Together AI's model names, we can't be sure "gpt-4o" is incorrect. The comment makes assumptions based on OpenAI naming that may not apply here.
    I could be wrong if "gpt-4o" is definitely a typo and Together AI only uses "gpt-4" naming. Maybe there's a standard naming convention I'm unaware of.
    Without clear documentation of Together AI's model names, we shouldn't assume this is a typo. The burden of proof is on the comment to show it's wrong.
    Delete the comment since we don't have strong evidence that "gpt-4o" is incorrect in the Together AI context.
6. python/plugins/togetherai/composio_togetherai/toolset.py:42
  • Draft comment:
    Typographical error: the word 'handelling' in the error message should be corrected to 'handling'.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_2GmpJtFNSTSGoqkw


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.

):
raise InvalidEntityIdError(
"separate `entity_id` can not be provided during "
"initialization and handelling tool calls"
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo in error message: 'handelling' should be 'handling'.

Suggested change
"initialization and handelling tool calls"
"initialization and handling tool calls"



client = Together(
api_key="59114908afa42e6b013964084e5b5fe52d375b8b2418e848d3f16bcee96f905a"
Copy link
Contributor

Choose a reason for hiding this comment

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

Avoid hardcoding API keys in code; use environment variables or configuration files for security.


def handle_tool_calls(
self,
response: ChatCompletionResponse | t.Iterator[ChatCompletionChunk],
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the union operator '|' in type annotations limits compatibility to Python 3.10+. Since python_requires is '>=3.9', consider using Union[...] for broader compatibility.

Suggested change
response: ChatCompletionResponse | t.Iterator[ChatCompletionChunk],
response: t.Union[ChatCompletionResponse, t.Iterator[ChatCompletionChunk]],

"""
entity_id = self.validate_entity_id(entity_id or self.entity_id)
outputs = []
if isinstance(response, t.Iterator):
Copy link
Contributor

Choose a reason for hiding this comment

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

When handling streaming responses, only the first chunk is processed using next(response). Additionally, using isinstance(response, t.Iterator) may not work as expected since t.Iterator is not runtime-checkable. Consider using collections.abc.Iterator and iterating over all chunks if multiple are expected.

Suggested change
if isinstance(response, t.Iterator):
if isinstance(response, collections.abc.Iterator):



client = Together(
api_key="59114908afa42e6b013964084e5b5fe52d375b8b2418e848d3f16bcee96f905a"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Critical security issue: API key is exposed in the code. This should be moved to an environment variable or secure configuration.

api_key="59114908afa42e6b013964084e5b5fe52d375b8b2418e848d3f16bcee96f905a"

Please update to use environment variables:

import os
api_key = os.getenv("TOGETHER_API_KEY")

my_task = "Star a repo composiohq/composio on GitHub"

# Create a chat completion request to decide on the action
response = client.chat.completions.create(model="gpt-4o",
Copy link
Collaborator

Choose a reason for hiding this comment

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

The README.md references incorrect model name gpt-4o. This should be updated to the correct Together AI model name.

_check_requested_actions=check_requested_actions,
)

def handle_tool_calls(
Copy link
Collaborator

Choose a reason for hiding this comment

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

The handle_tool_calls method lacks proper error handling for API failures and retries. Consider adding:

  1. Try-catch block for API calls
  2. Retry mechanism for failed requests
  3. Logging for debugging purposes

Example:

def handle_tool_calls(self, response: ChatCompletionResponse | t.Iterator[ChatCompletionChunk],
                     entity_id: t.Optional[str] = None,
                     check_requested_actions: bool = True,
                     max_retries: int = 3) -> t.List[t.Dict]:
    try:
        entity_id = self.validate_entity_id(entity_id or self.entity_id)
        outputs = []
        # ... rest of the code ...
        
    except Exception as e:
        logger.error(f"Error handling tool calls: {str(e)}")
        raise

)
]

def execute_tool_call(
Copy link
Collaborator

Choose a reason for hiding this comment

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

The docstring for execute_tool_call method is incomplete. Consider adding:

  1. Return type information
  2. Parameter descriptions for check_requested_actions
  3. Examples of usage
  4. Possible exceptions that could be raised

Example:

def execute_tool_call(
    self,
    tool_call: t.Union[ChatCompletionMessageToolCall, ToolCalls],
    entity_id: t.Optional[str] = None,
    check_requested_actions: bool = True,
) -> t.Dict:
    """Execute a tool call from Together AI.

    Args:
        tool_call: Tool call metadata from Together AI response
        entity_id: Entity ID to use for executing the function call
        check_requested_actions: Whether to validate against requested actions

    Returns:
        Dict containing output data from the tool call

    Raises:
        ValueError: If tool call function is None
        InvalidEntityIdError: If entity ID validation fails
    """

@shreysingla11
Copy link
Collaborator

Code Review Summary

Critical Issues

  1. 🚨 Security: Exposed API key in togetherai_demo.py needs to be moved to environment variables
  2. ⚠️ Error Handling: Missing proper error handling and retry mechanisms in tool calls
  3. 📝 Documentation: Incomplete docstrings and incorrect model reference in README

Recommendations

  1. Security:

    • Move API key to environment variables
    • Add input validation for critical parameters
  2. Error Handling:

    • Add try-catch blocks for API calls
    • Implement retry mechanism
    • Add proper logging
  3. Documentation:

    • Complete method docstrings with params, returns, raises sections
    • Fix incorrect model name in README
    • Add usage examples
  4. Best Practices:

    • Add input validation
    • Implement rate limiting
    • Add unit tests
    • Follow consistent docstring format

The PR adds valuable functionality but needs these improvements before it can be safely merged. Please address the security issue as a priority.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants