-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
feat: add LlamaIndex TS support to JS SDK #1178
base: master
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
parameters: { | ||
type: "object", | ||
properties: schema.parameters.properties || {}, | ||
required: schema.parameters.required || [], |
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.
The getTools
function in llamaindex.ts
does not correctly handle the case where schema.parameters
is undefined. This can lead to a runtime error when trying to access properties
or required
.
owner: "composioHQ", | ||
repo: "composio", | ||
}, | ||
entityId: "default", | ||
connectedAccountId: "9442cab3-d54f-4903-976c-ee67ef506c9b", |
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.
The test Should create custom action to star a repository
in llamaindex.spec.ts
uses hardcoded values for connectedAccountId
and the repository details. This makes the test less robust and prone to failures if these values change.
await llamaindexToolSet.createAction({ | ||
actionName: "starRepositoryCustomAction", | ||
toolName: "github", | ||
description: "This action stars a repository", | ||
inputParams: z.object({ | ||
owner: z.string(), | ||
repo: z.string(), | ||
}), | ||
callback: async ( | ||
inputParams, | ||
_authCredentials, | ||
executeRequest | ||
): Promise<ActionExecuteResponse> => { | ||
const res = await executeRequest({ | ||
endpoint: `/user/starred/${inputParams.owner}/${inputParams.repo}`, | ||
method: "PUT", | ||
parameters: [], | ||
}); | ||
return res; | ||
}, | ||
}); |
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.
The createAction
call in the test Should create custom action to star a repository
lacks proper error handling. If the action creation fails, the test will not fail and the subsequent getTools
and executeAction
calls might behave unexpectedly.
This comment was generated by github-actions[bot]! JS SDK Coverage Report📊 Coverage report for JS SDK can be found at the following URL: 📁 Test report folder can be found at the following URL: |
🚀 Code review completed! |
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.
👍 Looks good to me! Reviewed everything up to b7a8481 in 58 seconds
More details
- Looked at
192
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. js/src/frameworks/llamaindex.spec.ts:16
- Draft comment:
The test name 'getools' is not descriptive. Consider renaming it to something more meaningful, like 'should return tools array'. - Reason this comment was not posted:
Confidence changes required:50%
The test name 'getools' is not descriptive and seems to be a typo. It should be more descriptive to reflect the test's purpose.
2. js/src/frameworks/llamaindex.spec.ts:22
- Draft comment:
The test case 'check if tools are coming' is redundant with 'check if getTools, actions are coming'. Consider removing one to avoid duplication. - Reason this comment was not posted:
Confidence changes required:50%
The test case 'check if tools are coming' and 'check if getTools, actions are coming' are redundant as they perform the same check. One of them should be removed to avoid duplication.
3. js/src/frameworks/llamaindex.spec.ts:63
- Draft comment:
Typo in variable name 'actionOuput'. It should be 'actionOutput'. - Reason this comment was not posted:
Confidence changes required:50%
The variable name 'actionOuput' is misspelled. It should be 'actionOutput'.
4. js/src/frameworks/llamaindex.ts:36
- Draft comment:
Avoid using 'any' type for 'schema'. Use a more specific type to ensure type safety. - Reason this comment was not posted:
Confidence changes required:50%
The use of 'any' type in TypeScript should be avoided as it defeats the purpose of type safety. A more specific type should be used for 'schema'.
Workflow ID: wflow_1ImRrec93dw0lj3s
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
import { ActionExecuteResponse } from "../sdk/models/actions"; | ||
import { LlamaIndexToolSet } from "./llamaindex"; | ||
|
||
describe("Apps class tests", () => { |
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.
The test suite description Apps class tests
is misleading as it's testing the LlamaIndexToolSet class. Consider renaming it to LlamaIndexToolSet tests
for better clarity.
}); | ||
} | ||
|
||
private _wrapTool( |
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.
The _wrapTool
method accepts schema: any
as a parameter type. Consider creating a proper interface for better type safety:
interface ToolSchema {
name: string;
description: string;
parameters: {
properties?: Record<string, unknown>;
required?: string[];
};
}
entityId: Optional<string> = null | ||
): FunctionTool<Record<string, unknown>, JSONValue | Promise<JSONValue>> { | ||
return FunctionTool.from( | ||
async (params: Record<string, unknown>) => { |
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.
Consider adding error handling for the JSON parsing operation:
try {
return JSON.parse(JSON.stringify(result)) as JSONValue;
} catch (error) {
throw new Error(`Failed to parse tool result: ${error.message}`);
}
This would provide better error messages and make debugging easier.
Code Review SummaryOverall AssessmentThe implementation of LlamaIndex TypeScript framework support is well-structured and follows the established patterns in the codebase. The code is generally clean and functional, with good test coverage. Positive Aspects
Suggestions for Improvement
Code Quality Rating: 8/10The code is well-implemented and follows good practices, with minor improvements suggested for type safety and documentation. |
@abhishekpatil4 Couple of actions are failing, please fix them |
parameters: { | ||
type: "object", | ||
properties: schema.parameters.properties || {}, | ||
required: schema.parameters.required || [], | ||
}, |
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.
The getTools
function in llamaindex.ts
does not correctly handle the case when schema.parameters
is null or undefined, which could lead to a runtime error.
it("getools", async () => { | ||
const tools = await llamaindexToolSet.getTools({ | ||
apps: ["github"], | ||
}); | ||
expect(tools).toBeInstanceOf(Array); | ||
}); |
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.
The test getools
in llamaindex.spec.ts
doesn't assert anything meaningful. It only checks the type of the return, not the content.
it("Should create custom action to star a repository", async () => { | ||
await llamaindexToolSet.createAction({ | ||
actionName: "starRepositoryCustomAction", | ||
toolName: "github", | ||
description: "This action stars a repository", | ||
inputParams: z.object({ | ||
owner: z.string(), | ||
repo: z.string(), | ||
}), | ||
callback: async ( | ||
inputParams, | ||
_authCredentials, | ||
executeRequest | ||
): Promise<ActionExecuteResponse> => { | ||
const res = await executeRequest({ | ||
endpoint: `/user/starred/${inputParams.owner}/${inputParams.repo}`, | ||
method: "PUT", | ||
parameters: [], | ||
}); | ||
return res; | ||
}, | ||
}); | ||
|
||
const tools = await llamaindexToolSet.getTools({ | ||
actions: ["starRepositoryCustomAction"], | ||
}); | ||
|
||
await expect(tools.length).toBe(1); | ||
const actionOuput = await llamaindexToolSet.executeAction({ | ||
action: "starRepositoryCustomAction", | ||
params: { | ||
owner: "composioHQ", | ||
repo: "composio", | ||
}, | ||
entityId: "default", | ||
connectedAccountId: "9442cab3-d54f-4903-976c-ee67ef506c9b", | ||
}); | ||
|
||
expect(actionOuput).toHaveProperty("successful", true); | ||
}); |
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.
The test suite in llamaindex.spec.ts
is missing a test to verify that the createAction
method adds a new action correctly. The current test only checks if the number of tools increased but not the properties of the new tool/action.
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.
❌ Changes requested. Incremental review on 769477f in 59 seconds
More details
- Looked at
26
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
0
drafted comments based on config settings.
Workflow ID: wflow_bml1QyLngGW09OCX
Want Ellipsis to fix these issues? Tag @ellipsis-dev
in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
js/src/frameworks/llamaindex.ts
Outdated
type ToolSchema = { | ||
name: string; | ||
description: string; | ||
parameters: Record<string, unknown>; |
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.
The parameters
property in ToolSchema
is defined as Record<string, unknown>
, but it is accessed as if it has properties
and required
fields in _wrapTool
. Consider defining parameters
with a more specific type that includes properties
and required
fields to avoid potential runtime errors.
parameters: { | ||
type: "object", | ||
properties: schema.parameters.properties || {}, | ||
required: schema.parameters.required || [], | ||
}, |
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.
The getTools
function in llamaindex.ts
does not correctly handle the case where parameters
is undefined in the tool schema. This could lead to a runtime error when trying to access schema.parameters.properties
or schema.parameters.required
.
it("getools", async () => { | ||
const tools = await llamaindexToolSet.getTools({ | ||
apps: ["github"], | ||
}); | ||
expect(tools).toBeInstanceOf(Array); | ||
}); |
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.
The test getools
in llamaindex.spec.ts
doesn't assert anything meaningful. It only checks if the returned value is an array, but not its contents. This test should verify the expected tools are returned.
it("check if tools are coming", async () => { | ||
const tools = await llamaindexToolSet.getTools({ | ||
actions: ["GITHUB_GITHUB_API_ROOT"], | ||
}); | ||
expect(tools.length).toBe(1); | ||
}); | ||
it("check if getTools, actions are coming", async () => { | ||
const tools = await llamaindexToolSet.getTools({ | ||
actions: ["GITHUB_GITHUB_API_ROOT"], | ||
}); | ||
|
||
expect(tools.length).toBe(1); | ||
}); |
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.
Redundant tests in llamaindex.spec.ts
. The tests "check if tools are coming" and "check if getTools, actions are coming" are effectively identical. Consolidate these into a single test.
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.
👍 Looks good to me! Incremental review on 57535ff in 15 seconds
More details
- Looked at
19
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. js/src/frameworks/llamaindex.ts:13
- Draft comment:
The 'parameters' property in 'ToolSchema' is defined as 'Record<string, unknown>', but is accessed as if it has 'properties' and 'required' fields in '_wrapTool'. Ensure 'parameters' is correctly typed to avoid runtime errors. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_rovD7CEFRmRi9Sw4
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -40,6 +40,7 @@ | |||
"@langchain/openai": "^0.2.5", | |||
"ai": "^3.2.22", | |||
"langchain": "^0.2.11", | |||
"llamaindex": "^0.8.31", |
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.
The plus signs in the added line are redundant and should be removed.
📝 Committable Code Suggestion
‼️ Ensure you review the code suggestion before committing it to the branch. Make sure it replaces the highlighted code, contains no missing lines, and has no issues with indentation.
"llamaindex": "^0.8.31", | |
"llamaindex": "^0.8.31", |
import { beforeAll, describe, expect, it } from "@jest/globals"; | ||
import { z } from "zod"; | ||
import { getTestConfig } from "../../config/getTestConfig"; | ||
import { ActionExecuteResponse } from "../sdk/models/actions"; | ||
import { LlamaIndexToolSet } from "./llamaindex"; | ||
|
||
describe("Apps class tests", () => { | ||
let llamaindexToolSet: LlamaIndexToolSet; | ||
beforeAll(() => { | ||
llamaindexToolSet = new LlamaIndexToolSet({ | ||
apiKey: getTestConfig().COMPOSIO_API_KEY, | ||
baseUrl: getTestConfig().BACKEND_HERMES_URL, | ||
}); | ||
}); | ||
|
||
it("getools", async () => { | ||
const tools = await llamaindexToolSet.getTools({ | ||
apps: ["github"], | ||
}); | ||
expect(tools).toBeInstanceOf(Array); | ||
}); | ||
it("check if tools are coming", async () => { | ||
const tools = await llamaindexToolSet.getTools({ | ||
actions: ["GITHUB_GITHUB_API_ROOT"], | ||
}); | ||
expect(tools.length).toBe(1); | ||
}); | ||
it("check if getTools, actions are coming", async () => { | ||
const tools = await llamaindexToolSet.getTools({ | ||
actions: ["GITHUB_GITHUB_API_ROOT"], | ||
}); | ||
|
||
expect(tools.length).toBe(1); | ||
}); | ||
it("Should create custom action to star a repository", async () => { | ||
await llamaindexToolSet.createAction({ | ||
actionName: "starRepositoryCustomAction", | ||
toolName: "github", | ||
description: "This action stars a repository", | ||
inputParams: z.object({ | ||
owner: z.string(), | ||
repo: z.string(), | ||
}), | ||
callback: async ( | ||
inputParams, | ||
_authCredentials, | ||
executeRequest | ||
): Promise<ActionExecuteResponse> => { | ||
const res = await executeRequest({ | ||
endpoint: `/user/starred/${inputParams.owner}/${inputParams.repo}`, | ||
method: "PUT", | ||
parameters: [], | ||
}); | ||
return res; | ||
}, | ||
}); | ||
|
||
const tools = await llamaindexToolSet.getTools({ | ||
actions: ["starRepositoryCustomAction"], | ||
}); | ||
|
||
await expect(tools.length).toBe(1); | ||
const actionOuput = await llamaindexToolSet.executeAction({ | ||
action: "starRepositoryCustomAction", | ||
params: { | ||
owner: "composioHQ", | ||
repo: "composio", | ||
}, | ||
entityId: "default", | ||
connectedAccountId: "9442cab3-d54f-4903-976c-ee67ef506c9b", | ||
}); | ||
|
||
expect(actionOuput).toHaveProperty("successful", true); | ||
}); | ||
}); |
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.
Redundant plus signs should be removed.
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.
👍 Looks good to me! Incremental review on d602522 in 8 seconds
More details
- Looked at
12
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. js/package.json:43
- Draft comment:
'llamaindex' is listed in both 'dependencies' and 'peerDependencies'. It should be in one, not both, to avoid redundancy and potential version conflicts. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_o8WpdIgjNCaS3Ek6
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
import { FunctionTool, type JSONValue } from "llamaindex"; | ||
import { z } from "zod"; | ||
import { ComposioToolSet as BaseComposioToolSet } from "../sdk/base.toolset"; | ||
import { COMPOSIO_BASE_URL } from "../sdk/client/core/OpenAPI"; | ||
import { TELEMETRY_LOGGER } from "../sdk/utils/telemetry"; | ||
import { TELEMETRY_EVENTS } from "../sdk/utils/telemetry/events"; | ||
import { ZToolSchemaFilter } from "../types/base_toolset"; | ||
import { Optional, Sequence } from "../types/util"; | ||
|
||
type ToolSchema = { | ||
name: string; | ||
description: string; | ||
parameters: Record<string, unknown>; | ||
}; | ||
|
||
export class LlamaIndexToolSet extends BaseComposioToolSet { | ||
/** | ||
* Composio toolset for LlamaIndex framework. | ||
* | ||
*/ | ||
static FRAMEWORK_NAME = "llamaindex"; | ||
static DEFAULT_ENTITY_ID = "default"; | ||
fileName: string = "js/src/frameworks/llamaindex.ts"; | ||
|
||
constructor( | ||
config: { | ||
apiKey?: Optional<string>; | ||
baseUrl?: Optional<string>; | ||
entityId?: string; | ||
runtime?: string; | ||
} = {} | ||
) { | ||
super({ | ||
apiKey: config.apiKey || null, | ||
baseUrl: config.baseUrl || COMPOSIO_BASE_URL, | ||
runtime: config?.runtime || null, | ||
entityId: config.entityId || LlamaIndexToolSet.DEFAULT_ENTITY_ID, | ||
}); | ||
} | ||
|
||
private _wrapTool( | ||
schema: ToolSchema, | ||
entityId: Optional<string> = null | ||
): FunctionTool<Record<string, unknown>, JSONValue | Promise<JSONValue>> { | ||
return FunctionTool.from( | ||
async (params: Record<string, unknown>) => { | ||
const result = await this.executeAction({ | ||
action: schema.name, | ||
params, | ||
entityId: entityId || this.entityId, | ||
}); | ||
return JSON.parse(JSON.stringify(result)) as JSONValue; | ||
}, | ||
{ | ||
name: schema.name, | ||
description: schema.description, | ||
parameters: { | ||
type: "object", | ||
properties: schema.parameters.properties || {}, | ||
required: schema.parameters.required || [], | ||
}, | ||
} | ||
); | ||
} | ||
|
||
async getTools( | ||
filters: z.infer<typeof ZToolSchemaFilter> = {}, | ||
entityId: Optional<string> = null | ||
): Promise< | ||
Sequence< | ||
FunctionTool<Record<string, unknown>, JSONValue | Promise<JSONValue>> | ||
> | ||
> { | ||
TELEMETRY_LOGGER.manualTelemetry(TELEMETRY_EVENTS.SDK_METHOD_INVOKED, { | ||
method: "getTools", | ||
file: this.fileName, | ||
params: { filters, entityId }, | ||
}); | ||
|
||
const tools = await this.getToolsSchema(filters, entityId); | ||
return tools.map((tool) => { | ||
const wrappedTool = this._wrapTool(tool, entityId || this.entityId); | ||
Object.assign(wrappedTool, { | ||
name: tool.name, | ||
description: tool.description, | ||
parameters: tool.parameters, | ||
}); | ||
return wrappedTool; | ||
}); | ||
} | ||
} |
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.
Redundant plus signs should be removed.
@abhishekpatil4 https://github.com/ComposioHQ/composio/blob/d60252281a5705dbd7ccabe4bc51545ddf14a5a1/js/src/frameworks/langgraph.ts Can follow implementation like this |
Important
Add LlamaIndex framework support to JS SDK with new class and test cases.
LlamaIndexToolSet
class inllamaindex.ts
for LlamaIndex framework support.LlamaIndexToolSet
inindex.ts
.llamaindex.spec.ts
to validate LlamaIndex framework functionality.llamaindex
dependency inpackage.json
.This description was created by for d602522. It will automatically update as commits are pushed.