Skip to content

feat(js): added dynamicTool factory function that does not depend on genkit instance #3026

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

feat(js): added dynamicTool factory function that does not depend on genkit instance #3026

wants to merge 6 commits into from

Conversation

pavelgj
Copy link
Collaborator

@pavelgj pavelgj commented Jun 4, 2025 •
edited
Loading 

import { dynamicTool } from 'genkit';

const dynamicGablorkenTool = dynamicTool(
  {
    name: 'dynamicGablorkenTool',
    inputSchema: z.object({
      value: z.number(),
    }),
    description: 'can be used to calculate gablorken value',
  },
  async (input) => {
    return input.value * 3 - 4;
  }
);

const { text } = ai.generateStream({
  model: googleAI.model('gemini-2.0-flash'),
  tools: [dynamicGablorkenTool],
  prompt: `what is a gablorken of ${input}`,
});

also added global (Genkit instance level) context:

const ai = genkit({
  context: { something: 'extra' }
});

Checklist (if applicable):

@github-actions github-actions bot added the js label Jun 4, 2025
@pavelgj pavelgj requested a review from mbleigh June 4, 2025 02:05
@pavelgj pavelgj marked this pull request as ready for review June 4, 2025 20:33
@pavelgj
Test: Add non-serializable data to generate context
b06388d 
Modify the generate test to include a non-serializable object in the context. This ensures that the serialization logic handles such cases correctly.
@pavelgj pavelgj requested a review from ifielker June 5, 2025 14:14
ifielker
ifielker approved these changes Jun 5, 2025
View reviewed changes
js/ai/src/generate.ts
@@ -271,8 +276,6 @@ async function toolsToActionRefs(
} else if (typeof (t as ExecutablePrompt).asTool === 'function') {
const promptToolAction = await (t as ExecutablePrompt).asTool();
tools.push(`/prompt/${promptToolAction.__action.name}`);
} else if (t.name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you explain why you removed this? I'm not sure I understand... is it to avoid registering dynamic tools in the non-ephemeral registry?

js/ai/src/generate.ts
@@ -375,6 +378,9 @@ function maybeRegisterDynamicTools<
(t as Action).__action.metadata?.type === 'tool' &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might this if statement be better as a general type guard? Like:
function isDynamicToolAction(t: unknown): t is DynamicToolAction {
if (!t || typeof t !== 'object' || ...

possibly stacked with a isToolAction typeguard to simplify it:
if (!isToolAction(t)) { return false; }
return !!(t.__action.metadata?.dynamic)

js/ai/src/tool.ts
};
};

/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comments for these two types should not be the same. You should explain the difference between them and when each is used.

js/ai/src/tool.ts
@@ -174,10 +202,11 @@ export async function resolveTools<
return asTool(registry, ref as Action);
} else if (typeof (ref as ExecutablePrompt).asTool === 'function') {
return await (ref as ExecutablePrompt).asTool();
} else if (ref.name) {
} else if ((ref as ToolDefinition).name) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This function would be nicer with type guards instead of all the casting

js/core/tests/action_test.ts
@@ -118,6 +118,48 @@ describe('action', () => {
assert.deepStrictEqual(chunks, [1, 2, 3]);
});

it('run the action with context plus registry global context', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/run/runs "it runs ..."

js/genkit/tests/flow_test.ts
@@ -78,4 +80,15 @@ describe('flow', () => {
// a "streaming" flow can be invoked in non-streaming mode.
assert.strictEqual(await streamingBananaFlow('banana2'), 'banana2');
});

it('pass thought the context', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/pass/passes "it passes...
s/thought/through "...through the context"

js/genkit/tests/generate_test.ts
async (_, { context }) => JSON.stringify(context)
);

// first response be tools call, the subsequent just text response from agent b.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The comment needs more English sentence structure. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ifielker ifielker ifielker approved these changes

@mbleigh mbleigh Awaiting requested review from mbleigh

Assignees
No one assigned
Labels
js
Projects
Genkit Backlog
Status: No status
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pavelgj @ifielker