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

agentic functions support #652

Merged
merged 5 commits into from
Aug 26, 2024
Merged

agentic functions support #652

merged 5 commits into from
Aug 26, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Aug 26, 2024

  • 🛠️ Several tools and functions were renamed: definition has been changed to spec, and fn has been adjusted to impl. This modifies how tools and functions are defined, resolved, and called throughout the codebase (files run.ts, chat.ts, promptdom.ts and runpromptcontext.ts are affected).
  • 💡 A dynamic tool definition feature has been added, allowing tools to be declared directly with a ToolCallback object instead of separately specifying its name, description, parameters, and implementation (runpromptcontext.ts and prompt_type.d.ts changes).
  • 📚 The ToolCallOutput type has been expanded to include number and boolean values giving more flexibility to tool function return types (prompt_template.d.ts.
  • 💫 The API was extended to provide an additional defTool method that accepts a ToolCallback object (prompt_template.d.ts and prompt_type.ts).
  • 🔄 The default AI model was changed from "openai:gpt-4o" to "openai:gpt-4".
  • 🚀 A new calculator tool has been included in the package samples, showcasing the new way to define tools using defTool method with a ToolCallback object (agentic-calculator.genai.mts). This also required additional dependencies (@agentic/calculator and @agentic/core) to be added to the project (package.json).

generated by pr-describe

@@ -188,10 +188,10 @@ async function runToolCalls(
const context: ToolCallContext = {
trace,
}
const output = await tool.fn({ context, ...args })
const output = await tool.impl({ context, ...args })

Choose a reason for hiding this comment

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

The method name has been changed from 'fn' to 'impl'. This could potentially break any code that relies on the 'fn' method.

generated by pr-review-commit method_change

@@ -40,7 +40,7 @@ export const CLIENT_RECONNECT_DELAY = 3000
export const CLIENT_RECONNECT_MAX_ATTEMPTS = 20
export const RETRIEVAL_PERSIST_DIR = "retrieval"
export const HIGHLIGHT_LENGTH = 4000
export const DEFAULT_MODEL = "openai:gpt-4o"
export const DEFAULT_MODEL = "openai:gpt-4"

Choose a reason for hiding this comment

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

The default model has been changed from 'openai:gpt-4o' to 'openai:gpt-4'. This could potentially affect the performance or behavior of the model.

generated by pr-review-commit model_change

Copy link

The pull request introduces several changes in the code, mainly in the naming of certain variables and functions. Here's a breakdown of the primary changes:

  1. The word definition has been replaced by spec in several instances, which suggests a refactoring for increased readability or consistency in the naming convention.

  2. The DEFAULT_MODEL value in constants.ts has been changed from "openai:gpt-4o" to "openai:gpt-4".

  3. The function name fn has been changed to impl in several places. This could be to make the function name more descriptive and improve code readability.

  4. There are changes to error messages, such as changing "tool" to "script" in error messages. This could be for better precision in error reporting.

  5. The ToolCallOutput type in prompt_template.d.ts now includes number and boolean as possible output types. This broadens the range of possible return types for functions that have ToolCallOutput as their output type.

  6. The defTool function has been overloaded to now also accept a ToolCallback object as a single argument. This provides an alternative way to define a tool.

In general, the changes seem to be improvements aimed at refining the code, making it more readable, and enhancing the flexibility of certain functions and types. However, without the context of the entire codebase, it's hard to be definitive.

Here are a few potential concerns:

  1. The change in the DEFAULT_MODEL value might have impacts elsewhere in the codebase if other parts rely on the old value.

  2. The overload of defTool function requires careful handling to avoid confusion or misuse.

Overall, these changes look good. However, it's critical to test these changes thoroughly and ensure that the renaming and new function overload don't cause undesired side effects in the rest of the application.

So, considering the above points, I would say "LGTM 🚀", but with a note of caution to thoroughly test the changes before merging.

generated by pr-review

@pelikhan pelikhan changed the title Refactor code to use @agentic/calculator for calculations agentic functions support Aug 26, 2024
@@ -218,7 +218,7 @@ export async function runScript(
GENAI_ANY_REGEX.test(scriptId) &&
resolve(t.filename) === resolve(scriptId))
)
if (!script) throw new Error(`tool ${scriptId} not found`)
if (!script) throw new Error(`script ${scriptId} not found`)

Choose a reason for hiding this comment

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

The error message should be "tool" instead of "script" as the context is about finding a tool.

generated by pr-review-commit incorrect_error_message

if (output === undefined || output === null)
throw new Error(
`tool ${tool.definition.name} output is undefined`
`tool ${tool.spec.name} output is undefined`

Choose a reason for hiding this comment

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

The error message should be "tool" instead of "script" as the context is about a tool's output.

generated by pr-review-commit incorrect_error_message

@@ -40,7 +40,7 @@ export const CLIENT_RECONNECT_DELAY = 3000
export const CLIENT_RECONNECT_MAX_ATTEMPTS = 20
export const RETRIEVAL_PERSIST_DIR = "retrieval"
export const HIGHLIGHT_LENGTH = 4000
export const DEFAULT_MODEL = "openai:gpt-4o"
export const DEFAULT_MODEL = "openai:gpt-4"

Choose a reason for hiding this comment

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

The constant DEFAULT_MODEL should be "openai:gpt-4o" instead of "openai:gpt-4" as per the previous value.

generated by pr-review-commit incorrect_constant_value

@@ -1,4 +1,3 @@
import { createProgressSpinner } from "./spinner"
import replaceExt from "replace-ext"

Choose a reason for hiding this comment

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

Missing import statement for createProgressSpinner from "./spinner".

generated by pr-review-commit missing_import

}
spinner.stop()
}

Choose a reason for hiding this comment

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

Missing progress report for jsonl2json function.

generated by pr-review-commit missing_progress_report

text += `${file}, ${tokens}\n`
}
}
progress.stop()
console.log(text)
}

Choose a reason for hiding this comment

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

Missing progress report for parseTokens function.

generated by pr-review-commit missing_progress_report

@@ -40,7 +40,7 @@ export const CLIENT_RECONNECT_DELAY = 3000
export const CLIENT_RECONNECT_MAX_ATTEMPTS = 20
export const RETRIEVAL_PERSIST_DIR = "retrieval"
export const HIGHLIGHT_LENGTH = 4000
export const DEFAULT_MODEL = "openai:gpt-4o"
export const DEFAULT_MODEL = "openai:gpt-4"

Choose a reason for hiding this comment

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

The model name should be "openai:gpt-4o" instead of "openai:gpt-4".

generated by pr-review-commit incorrect_model_name

@pelikhan pelikhan merged commit 5f573a5 into main Aug 26, 2024
9 checks passed
@pelikhan pelikhan deleted the agentic-tools branch August 26, 2024 14:06
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.

1 participant