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

Refactor system handling and update type definitions #701

Merged
merged 5 commits into from
Sep 9, 2024

Conversation

pelikhan
Copy link
Member

@pelikhan pelikhan commented Sep 9, 2024

This pull request includes several commits that refactor the system handling in the codebase. It moves the flexTokens property from ScriptRuntimeOptions to ModelOptions in the types file. It also removes an extra line in the prompt_template.d.ts file. These changes improve clarity and extendability in the system resolution and update functions.

  • 🛠️ In the "chat.ts" file, trailing whitespaces were added to a line.
  • 🔄 In "promptcontext.ts", there is a shift from retrieving systems from the runOptions to obtaining them through a call to resolveSystems(). The variable system is no longer destructured from runOptions. Instead, systemId is resolved within a for loop from the execution of resolveSystems(prj, runOptions).
  • 📦 New function resolveSystems() is introduced in "systems.ts", whereby systems are resolved based on a given script instead of a template. Also, there is a modification in the usage of arrayify() function to convert the system parameters to an array.
  • 👾 In "promptdom.ts", there is a refactor in creating the function node to include description within the dedent function.
  • 📚 Change in "prompt_template.d.ts" Prior ScriptRuntimeOptions, is split into two interfaces - ScriptRuntimeOptions and PromptSystemOptions for better categorization of options related to system scripts and runtime respectively. This leads to an extension of PromptSystemOptions in PromptScript interface and PromptGeneratorOptions.
  • 📝 New file nested-agents.genai.mts is added. This defines two new tools namely "agent_file_system" and "agent_code_interpreter". These tools are defined to execute the LLM (low-latency model) requests for searching/reading from the file system and to execute python code.

generated by pr-describe

packages/core/src/chat.ts Outdated Show resolved Hide resolved
@@ -265,7 +266,7 @@ export async function createPromptContext(
role: "system",
content: "",
}
for (const systemId of system) {
for (const systemId of resolveSystems(prj, runOptions)) {
Copy link

Choose a reason for hiding this comment

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

The function resolveSystems is missing the second argument runOptions. This could lead to unexpected behavior or runtime errors. Please ensure that the necessary arguments are provided. 🧐

generated by pr-review-commit missing_argument

return {
type: "function",
name,
description: dedent(description),
Copy link

Choose a reason for hiding this comment

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

The function dedent is not defined in this scope. Please ensure that it is imported or defined before use. 🕵️‍♀️

generated by pr-review-commit undefined_function

Copy link

github-actions bot commented Sep 9, 2024

The changes in the GIT_DIFF look good overall. Here is a short summary:

  1. In src/chat.ts, a trailing space has been added to the line where the embeddingsModel is defined. This change is minor and should not affect the functionality of the code.

  2. In src/promptcontext.ts, the system array from runOptions is no longer directly used in a for loop. Instead, the resolveSystems() function is used to obtain the array of systems. This change enhances the modularity of the code by delegating the responsibility of resolving systems to a separate function.

  3. In src/promptdom.ts, the createFunctionNode() function has been updated to dedent the description. This change improves the formatting of the description, making it more readable.

  4. In src/systems.ts, the resolveSystems() function has been updated to take a script parameter instead of a template. The system and tools are retrieved from the script instead of the template. This change optimizes the function by directly passing the script object that contains the needed information.

  5. In src/types/prompt_template.d.ts, the ScriptRuntimeOptions interface has been split into two - PromptSystemOptions and ScriptRuntimeOptions. The system and tools properties have been moved to PromptSystemOptions. This change improves the separation of concerns, making the type definitions clearer and more specific.

  6. Lastly, the PromptGeneratorOptions interface now extends PromptSystemOptions. This change is in line with the updates in src/types/prompt_template.d.ts.

No concerns were found in these changes. The code looks good and the changes improve the code quality.

LGTM 🚀

generated by pr-review

@@ -215,7 +216,7 @@ export async function createPromptContext(
},
runPrompt: async (generator, runOptions): Promise<RunPromptResult> => {
try {
const { label, system = [] } = runOptions || {}
const { label } = runOptions || {}
Copy link

Choose a reason for hiding this comment

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

The system array from runOptions is no longer being destructured, which could lead to system being undefined in the following code.

generated by pr-review-commit missing_system

return {
type: "function",
name,
description: dedent(description),
Copy link

Choose a reason for hiding this comment

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

The dedent function is being used to format the description but it's not imported in this file. This could lead to a runtime error.

generated by pr-review-commit missing_dedent_import

/**
* Budget of tokens to apply the prompt flex renderer.
*/
flexTokens?: number
Copy link

Choose a reason for hiding this comment

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

The flexTokens property is added to the ModelOptions interface but it's not clear what it does or what type it should be. It would be helpful to add a type and a more detailed comment explaining its purpose.

generated by pr-review-commit missing_flexTokens_definition

@pelikhan pelikhan merged commit 471e9f2 into main Sep 9, 2024
10 checks passed
@pelikhan pelikhan deleted the run_prompt_tools branch September 9, 2024 16:15
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