-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: add ZoomInfo Toolkit for CAMEL-AI framework #3389
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
base: master
Are you sure you want to change the base?
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
JINO-ROHIT
left a comment
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.
Thanks for the PR, i have added few comments, feel free to implement them if you think they make sense
| global _zoominfo_access_token, _zoominfo_token_expires_at | ||
|
|
||
| # Check if current token is still valid | ||
| if _zoominfo_access_token and time.time() < _zoominfo_token_expires_at: |
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.
is this necessary?
|
|
||
| def _get_zoominfo_token() -> str: | ||
| r"""Get or refresh ZoomInfo JWT token.""" | ||
| global _zoominfo_access_token, _zoominfo_token_expires_at |
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.
is there a reason to make this global? is there a more safer way to implement this? global vars have an issue with race conditions
|
also, it would be helpful to not reopen another PR, please update changes within the same PR |
|
thanks @haoye2 for the PR and @JINO-ROHIT for the review, awesome to see you back in the CAMEL community — warm welcome! |
waleedalzarooni
left a comment
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.
Hey @haoye2,
Thanks for your work on the PR, looking forward to the final version!
| response.raise_for_status() | ||
| return response.json() | ||
|
|
||
| except requests.exceptions.RequestException as e: |
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.
include some more info here for debugging
for example:
HTTP status code
Response body
Request endpoint
Original exception type
| @@ -0,0 +1,312 @@ | |||
| # ========= Copyright 2023-2024 @ CAMEL-AI.org. All Rights Reserved. ========= | |||
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.
could you ensure all necessary dependencies are added to pyproject.toml as per the guidelines in Contributing.MD (zi_api_auth_client, pyjwt, cryptography)
| 'NotionMCPToolkit', | ||
| 'VertexAIVeoToolkit', | ||
| 'MinimaxMCPToolkit', | ||
| "ZoomInfoToolkit", |
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.
| "ZoomInfoToolkit", | |
| "ZoomInfoToolkit", |
spacing needs fixing
|
|
||
| import os | ||
| import time | ||
| import json |
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.
unused imports
import json
from cryptography.hazmat.primitives import hashes
from cryptography.hazmat.primitives.asymmetric import padding
| json_data: Optional[Dict[str, Any]] = None, | ||
| params: Optional[Dict[str, Any]] = None, | ||
| ) -> Dict[str, Any]: | ||
| r"""Make a request to ZoomInfo API with proper error handling.""" |
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.
docstring must match description conventions of other methods
| headers=request_headers, | ||
| json=json_data, | ||
| params=params, | ||
| timeout=30, |
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.
should this use the configurable timeout parameter in __init__, if so possibly move the method inside the toolkit class
| page: int = 1, | ||
| sort_by: Literal["name", "employeeCount", "revenue"] = "name", | ||
| sort_order: Literal["asc", "desc"] = "asc", | ||
| **kwargs |
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.
I think it would be best to remove the kwargs argument as it may confuse the agent and allow it to add incorrect params
| (None, "ZOOMINFO_USERNAME"), | ||
| (None, "ZOOMINFO_PASSWORD"), | ||
| ]) | ||
| # Search for companies by various criteria |
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.
remove all the comments above methods as descriptions within methods explain the purpose of functions anyways
| def __init__(self, timeout: Optional[float] = None): | ||
| super().__init__(timeout=timeout) | ||
| # Validate credentials on initialization | ||
| _get_zoominfo_token() |
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.
remove _get_zoominfo_token() since it's already called in _make_info_request no need for redundant API call
| @@ -0,0 +1,91 @@ | |||
| # ========= Copyright 2023-2024 @ CAMEL-AI.org. All Rights Reserved. ========= | |||
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.
were you able to test this with the API since the free trial doesn't give you access to it
Description
This pull request changed the branch; all subsequent commits will be made on this pull request.
This PR refactors the ZoomInfo toolkit by removing redundant fallback imports and cleaning up duplicate BaseToolkit dependencies. This is an improved version of the previous PR #3368, addressing the feedback and implementing a cleaner approach.
Note: I have created a new branch for this PR to ensure a clean history. All future commits and updates related to this refactoring will be made on this PR instead of the original PR #3368.
Changes Made
Related Issues
Testing
Checklist
Additional Context
This refactoring improves the overall code quality and removes unnecessary complexity from the ZoomInfo toolkit implementation. The changes are minimal but impactful for maintainability.
For reviewers: Please focus your review on this PR rather than PR #3368, as this contains the latest and cleanest implementation. The original PR can be closed once this one is approved.