-
Notifications
You must be signed in to change notification settings - Fork 78
Feat/subset of input schema #210
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
Conversation
|
@MichalKalita please ask for review only once it is ready for a review. For example, I'm getting bunch of errors when running |
|
@jirispilka It's some kind of mistake. I haven't asked for review yet. This pull request is a draft. |
ok, no problem :) . I was just eager to check and then a bit disappointed that it is not working |
- Introduced `McpOptions` type for input parameters. - Replaced the previous input processing logic with Zod schema validation for better type safety and clarity. - Updated related functions and types to align with the new schema. - Added a new constant `ADVANCED_INPUT_KEY` in `const.ts` for future use.
MQ37
left a comment
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.
Went through it and I like the refactored and simplified parts - mainly the ToolEntry type. There is couple of nits and one important question regarding the caching that needs clarification before merging.
Note: Please also be sure to document this in docs.apify.com and this repo README.md.
| expectToolNamesToContain(names, actors.map((actor) => actorNameToToolName(actor))); | ||
| expect(names).toMatchInlineSnapshot(` | ||
| [ | ||
| "get-actor-details", |
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.
If we change the tool names we need to update the test, this is fragile, I prefer the previous logic - it was not nice but was definitely more robust.
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.
I have a problem with this tests when I worked on this issue. Tests failed and I don't know why. This test is really simple and if fails, is easy to find where is a problem, what missing and what should not be here.
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.
All snapshots can be updated using the command npx vitest tests --update.
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.
I don't like this added complexity. But if you are both @jirispilka and @MichalKalita for using this then I am fine with keeping that.
| const schemaID = getToolSchemaID(actorDefinitionPruned.actorFullName); | ||
| if (actorDefinitionPruned.input && 'properties' in actorDefinitionPruned.input && actorDefinitionPruned.input) { | ||
| actorDefinitionPruned.input.properties = transformActorInputSchemaProperties(actorDefinitionPruned.input); | ||
| actorDefinitionPruned.input.properties = transformActorInputSchemaProperties( |
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.
Minor comment—no need to fix now, maybe later.
We currently have three things there: basic, full, and pruned (though “pruned” is a bit misleading). I know it’s a modified schema based on some transformation, but it could also be a schema without advancedInput.
jirispilka
left a comment
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.
(somehow IDE crashed when submitting the review)
I like it.
But apart from the comments from MQ and my own comments, IMO we’re missing some functionality.
When fullSchema is false, the LLM doesn’t know there are advanced parameters available.
I think we discussed the following flow:
- Provide the basic schema, but include the advanced attributes in the description.
- Let the LLM know that other attributes exist and that it can fetch the full schema to call an Actor with them.
- Essentially, the LLM would first do
get-schemaand thencall-actor. This is inefficient, but it would solve some cases.
And the assumption was that:
- 90% use-cases are solved with basic schema
- 10% use-cases, either you opt for
fullSchemaor you will doget-schemaandcall-actor(yes, you need to have this feature enabled)
|
One more thing, we should document it in the README (query parameter + description) And we'll need to include it in |
|
Closing this PR, more details in #184 (comment) |
closes #184