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

docs: initial commit of developer information about tools #7

Merged
merged 21 commits into from
Sep 2, 2024

Conversation

grahamwhiteuk
Copy link
Contributor

No description provided.

@Tomas2D Tomas2D self-requested a review August 28, 2024 12:16
@Tomas2D Tomas2D force-pushed the main branch 5 times, most recently from f4ed0d8 to 99093e4 Compare August 29, 2024 09:51
@Tomas2D Tomas2D changed the base branch from main to main-placeholder August 29, 2024 09:53
@Tomas2D Tomas2D changed the base branch from main-placeholder to main August 29, 2024 09:53
Copy link
Contributor

@Tomas2D Tomas2D left a comment

Choose a reason for hiding this comment

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

Great work! 🚀

However, I see some room for improvement, like:

  • Provide a better explanation of Tool class generics and related generics.
  • Mention that zod schema is under the hood, converted to JSONSchema, and validated via Ajv. When the input is invalid, ToolInputError is thrown.
  • Mention that if one wants to use zod effects (somehow modify/transport/improve the validation process), it can be achieved via override validateInput method (see arxiv tool where it is done).
  • Show an example with DynamicTool (feel free to see tests for inspiration).
  • Clarify content of BaseToolOptions and BaseToolRunOptions interfaces.
  • Use camelCase in naming files (helloworld.ts -> helloWorld.ts).
    Understanding why a tool must return a class that extends BaseToolOutput is crucial. This class provides you with the getTextContent() method that the agent/llm can use, ensuring the proper functioning of your tool.

docs/tools.md Outdated Show resolved Hide resolved
docs/tools.md Show resolved Hide resolved
docs/tools.md Outdated

- Declare an input schema:

This is used to define the format of the input to your tool. The agent will formalise the natural language input(s) it has received and structure them into the fields described in the tool's input. The input schema is specified using [Zod](https://github.com/colinhacks/zod).
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This comment contradicts the comment at the top of the review. Would it be preferable to inform the users via this documentation that zod effects are not supported, or that they can be supported by that requires overriding the validateInput function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've attempted an initial resolution in 3483a55 but this may need to be changed as a result of my above observation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Zod effects are basically some extra "application level" transformations that are not transferable to JSON Schema. But the framework (under the hoods) converts it to a JSON Schema. So to do something like "zod effects" you have to specify inputSchema without them and do post-validation manually later in validateSchema method.

The inputSchema definition must always be an object and pass validation by the validateSchema() function defined in schema.ts.

This statement may sound like you are forcing the user to implement this feature. Either remove the statement or provide a simple example.

docs/tools.md Outdated Show resolved Hide resolved
docs/tools.md Outdated Show resolved Hide resolved
examples/tools/helloworld.ts Outdated Show resolved Hide resolved
examples/tools/openlibrary.ts Outdated Show resolved Hide resolved
examples/tools/openlibrary.ts Outdated Show resolved Hide resolved
examples/tools/openlibrary.ts Outdated Show resolved Hide resolved
examples/tools/openlibrary.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@Tomas2D Tomas2D left a comment

Choose a reason for hiding this comment

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

Consider adding a link from README.md to tools.md

docs/tools.md Show resolved Hide resolved
docs/tools.md Outdated Show resolved Hide resolved
docs/tools.md Outdated Show resolved Hide resolved
examples/tools/openLibrary.ts Outdated Show resolved Hide resolved
@grahamwhiteuk
Copy link
Contributor Author

Consider adding a link from README.md to tools.md

Fixed in ac327b0 (the link will be broken until this PR is merged as the tools file doesn't currently exist on the main branch)

docs/tools.md Show resolved Hide resolved
Copy link
Contributor

@Tomas2D Tomas2D left a comment

Choose a reason for hiding this comment

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

Please add one of the following secrets to your fork so the GitHub action can pass E2E Tests.

GENAI_API_KEY="..."
WATSONX_API_KEY="..."
OPENAI_API_KEY="..."

One of them should be enough (ideally BAM - GENAI_API_KEY)

@grahamwhiteuk
Copy link
Contributor Author

GENAI_API_KEY is now added and the above comments are addressed in f94bea5

@Tomas2D Tomas2D merged commit 3fd62fe into i-am-bee:main Sep 2, 2024
1 check passed
@grahamwhiteuk grahamwhiteuk deleted the feat-tool-example-and-docs branch September 3, 2024 09:24
michael-desmond pushed a commit to michael-desmond/bee-agent-framework that referenced this pull request Sep 3, 2024
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.

2 participants