-
Notifications
You must be signed in to change notification settings - Fork 148
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
Add structure run tool #764
Conversation
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.
Nice work and thanks for the refactors!
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 wonder if we should just bake in the whole submit/polling logic into a single method. Can't think of any benefits to having the LLM be the one to poll.
@collindutter @vachillo Thanks for the comments! Attempted to address everything. Lmk how it looks |
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.
Also please create a docs page for this Tool.
@collindutter good call, added a docs page and refactored based on comments! |
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.
Couple more minor things.
@property | ||
def description(self) -> str: | ||
if self._description is None: | ||
from requests import get | ||
|
||
url = urljoin(self.base_url.strip("/"), f"/api/structures/{self.structure_id}/") | ||
|
||
response = get(url, headers=self.headers).json() | ||
if "description" in response: | ||
self._description = response["description"] | ||
else: | ||
raise ValueError(f'Error getting Structure description: {response["message"]}') | ||
|
||
return self._description | ||
|
||
@description.setter | ||
def description(self, value: str) -> None: | ||
self._description = value |
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.
Properties should come after class fields
status = result.get("status") | ||
|
||
wait_attempts = 0 | ||
while status in ["QUEUED", "RUNNING"] and wait_attempts < self.structure_run_max_wait_time_attempts: |
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.
Nit: use tuple over list.
url = urljoin(self.base_url.strip("/"), f"/api/structure-runs/{structure_run_id}") | ||
|
||
result = self._get_structure_run_result_attempt(url) | ||
status = result.get("status") |
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.
This has the potential to be None
. We should use a regular dictionary lookup or handle the None
case if we ever expect it to be so.
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.
Nice 😎
GriptapeCloudStructureRunClient
tool for invoking Griptape Cloud Structure Run APIsBaseGriptapeCloudClient