-
Notifications
You must be signed in to change notification settings - Fork 183
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
Toolkit Task Improvements #1268
Conversation
Codecov ReportAttention: Patch coverage is
📢 Thoughts on this report? Let us know! |
89c688c
to
c057ceb
Compare
c057ceb
to
bc6c893
Compare
elif not subtask.actions: | ||
# handle case when the LLM failed to follow the ReAct prompt and didn't return a proper action | ||
subtask.output = subtask.input |
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.
We need to handle this slightly differently depending on whether we're using native tool calling or not. Moved this logic to ActionsSubtask
.
def __parse_actions(self, actions_matches: list[str]) -> None: | ||
def __parse_actions(self, actions_matches: list[str]) -> list[ToolAction]: | ||
if len(actions_matches) == 0: | ||
return | ||
return [] | ||
try: | ||
data = actions_matches[-1] | ||
actions_list: list[dict] = json.loads(data, strict=False) | ||
|
||
self.actions = [self.__process_action_object(action_object) for action_object in actions_list] | ||
return [self.__process_action_object(action_object) for action_object in actions_list] | ||
except json.JSONDecodeError as e: | ||
logger.exception("Subtask %s\nInvalid actions JSON: %s", self.origin_task.id, e) | ||
|
||
self.output = ErrorArtifact(f"Actions JSON decoding error: {e}", exception=e) | ||
|
||
return [] | ||
|
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.
Fewer side effects, more consistent with native tool calling action parsing.
!!! warning | ||
`JsonSchemaRule` may break [ToolkitTask](../structures/tasks.md#toolkit-task) which relies on a specific [output token](https://github.com/griptape-ai/griptape/blob/e6a04c7b88cf9fa5d6bcf4c833ffebfab89a3258/griptape/tasks/toolkit_task.py#L28). | ||
|
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.
Technically this is still true for non native tool calling, but that feels edge case enough where it's not worth mentioning.
|
||
Actions might store their output in memory as artifacts (with `memory_name` and `artifact_namespace`). If action output is stored in memory, ALWAYS try to pass it to another action. NEVER make up memory names or artifact namespaces. |
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 was causing the LLM to hallucinate these keys into the Tool schema. The Tool schema is descriptive enough, we don't need to mention them here.
bc6c893
to
d99b7cc
Compare
d99b7cc
to
c78e6b1
Compare
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.
Code looks good to me. Personal structures/tool use cases are happy with this branch as well.
c78e6b1
to
b5025d8
Compare
Describe your changes
Changed
ToolkitTask
system prompt to no longer mentionmemory_name
andartifact_namespace
.ToolkitTask
with native tool calling no longer need to provide their final answer asAnswer:
.Fixed
memory_name
andartifact_namespace
into Tool schemas when usingToolkitTask
.ToolkitTask
.Issue ticket number and link
https://discord.com/channels/1096466116672487547/1105169591619027055/1296270653238022226
📚 Documentation preview 📚: https://griptape--1268.org.readthedocs.build//1268/