Skip to content

enhance: simplify the dynamic tool logic #302

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

Jump to bottom
Merged
merged 1 commit into from
Aug 28, 2024
Merged

enhance: simplify the dynamic tool logic #302

merged 1 commit into from
Aug 28, 2024

Conversation

@thedadams
Copy link
Contributor

A change has been merged into gptscript core that allows us to simplify the dynamic tool logic.

g-linville
g-linville approved these changes Aug 27, 2024
View reviewed changes
Copy link
Member

@g-linville g-linville left a comment

Choose a reason for hiding this comment

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

As long as this doesn't give the LLM access to call credential tools, I'm good with it.

@thedadams
Copy link
Contributor Author

This change doesn't add all the tools that a tool references, like it did previously.

@thedadams
enhance: simplify the dynamic tool logic
bb6beec 
A change has been merged into gptscript core that allows us to simplify
the dynamic tool logic.

Signed-off-by: Donnie Adams <donnie@acorn.io>
@thedadams thedadams requested a review from g-linville August 28, 2024 02:57
@thedadams
Copy link
Contributor Author

The changes here have spiraled.

In many places, we were using rootTool. This is the first tool in the file. The issue with using this is that, if the first tool references another tool in the file (like dynamic-instructions), then using this rootTool will fail because gptscript can't find the referenced tool.

This was causing issues with saving after these changes. Therefore, I removed the rootTool and am using scriptContent wherever possible.

StrongMonkey
StrongMonkey approved these changes Aug 28, 2024
View reviewed changes
Copy link
Contributor

@StrongMonkey StrongMonkey left a comment

Choose a reason for hiding this comment

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

Pretty big changes but simplified a lot of the code. I can help testing once this is in. 👍

components/chat.tsx
workspace,
thread
thread,
gatewayTool()
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we always add gatewayTools now?

server/app.mjs
runningScript = null;
}

state.tools = state.tools.filter((t) => t === knowledgeTool);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nvm, looks like you are filtering out the knowledge tool. I wonder if you can just get the value from code instead of passing around through the socket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't letting me import the package from app.mjs, so this was the best I could do.

@thedadams thedadams merged commit d4d5806 into gptscript-ai:main Aug 28, 2024
@thedadams thedadams deleted the simple-dynamic-tools branch August 28, 2024 13:57
@thedadams thedadams mentioned this pull request Aug 28, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@StrongMonkey StrongMonkey StrongMonkey approved these changes

@njhale njhale Awaiting requested review from njhale

@g-linville g-linville Awaiting requested review from g-linville

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@thedadams @StrongMonkey @g-linville