FEATURE: Tool name validation #842

nvh0412 · 2024-10-18T12:09:29Z

Given that most LLM providers have their own function name formats (except for Ollama, as far as I have tested), they typically accept alphanumeric values (only including characters, numbers, and underscores). If we create a custom tool with the wrong name format, we won’t be able to invoke these tools. Other built-in tools we have won’t encounter this problem since their names are valid.

Ref:

https://ai.google.dev/gemini-api/docs/function-calling#key-parameters-best-practices
https://meta.discourse.org/t/validate-the-name-field-for-ai-tools/330830

In this PR, we introduce the tool name validation in:

The ActiveRecord class - the validation would be kicked in if we call the API directly.
Use the tool_name to build the function payload to send to LLM instead of using the name.
Add a validation on the AiPersona model which helps us prevent duplicate added tools to a Persona
Add the unique constraint to the name field of the AiTool model.

xfalcox · 2024-10-18T18:24:53Z

app/services/discourse_ai/tool_name_checker.rb

+
+    private
+
+    def check_name_availability


Should we also add a unique index to ensure uniqueness ?

Thanks, I'll double-check it later, but we did have that unique option on that column in the first place https://github.com/discourse/discourse-ai/blob/main/db/migrate/20240618080148_create_ai_tools.rb#L6, it's highly likely that we used the wrong option syntax for it, it should be

t.string :name, index: { unique: true }

SamSaffron · 2024-10-21T00:03:43Z

I think we may need a bit more of a refactor here, technically you may want 2 tools called "search" but then at the persona level pick the particular search you are after.

So maybe we want an extra field here (tool_name tool name presented to LLM):

name: search elastic docs 
tool_name: search

Then the "persona" save should ensure that the subset of tools you picked have distinct names. This also fixes "built-in" and "byo" clashes.

I am happy though with being more strict about what letters we allow for "tool_name" cause it will generally be safer to keep to a stricter amount of rules here.

nvh0412 · 2024-10-21T20:22:14Z

config/locales/client.en.yml

        name: "Name"
        name_help: "The unique name of the tool as used by the language model"
+        tool_name: "Tool Name"
+        tool_name_help: "Tool name, only include numbers, letters, and underscores"


Let me know if you think there is a better copy for this tool_name_help and also the name_help copy as its value isn't unique actually.

SamSaffron · 2024-10-21T23:06:13Z

app/models/ai_tool.rb

@@ -2,6 +2,7 @@

 class AiTool < ActiveRecord::Base
  validates :name, presence: true, length: { maximum: 100 }
+  validates :tool_name, presence: true, length: { maximum: 100 }, uniqueness: true


tool_name should not be unique... it is a constraint on "persona" not on tool, the same tool name can exist multiple times with no problem

SamSaffron · 2024-10-21T23:07:13Z

app/models/ai_tool.rb

@@ -202,7 +222,8 @@ def self.presets
      },
      {
        preset_id: "stock_quote",
-        name: "stock_quote",
+        name: "Stock Quote",


Stock Quote (AlphaVantage) - feels like a better name.

SamSaffron · 2024-10-21T23:10:59Z

its getting there, but we need to be clearer on what "name" means and what "tool_name" means.

Name will show up in the Discourse UI and is the short identifier you will use to find the tool in various settings, it should be distinct (it is required)

Tool Name is presented to the large language model. It is not distinct, but it is distinct per persona. (persona validates on save)

I would say we don't allow nulls in either columns and during the migration just copy over old Name into Tool Name.

We probably want a distinct constraint on Name which means we need to append (1,2,3 etc) onto name during the migration if there are dupes.

db/migrate/20241020010245_add_tool_name_to_ai_tools.rb

SamSaffron · 2024-10-23T04:37:35Z

I am not sure we need any of the "check_name" stuff though? we will just pick up issues on save. ''

the big thing missing though is the validation on save for ai_persona, that is where we want to make sure we did not add any duplicate tool names.

nvh0412 · 2024-10-23T04:51:12Z

I’m on board with you regarding the check_name issue, it’s now redundant if there’s no uniqueness validation on the tool_name. I’ll remove it shortly.

The purpose of this PR is to ensure we don’t have any invalid function names when invoking them via the LLM provider. As for the uniqueness tool validation on save for ai_persona, that feels like a separate matter to me.

But challenge accepted! I’ll take care of it—it’s actually quite fun.

nvh0412 · 2024-10-23T08:05:41Z

lib/ai_bot/personas/general.rb

@@ -10,7 +10,6 @@ def tools
            Tools::Google,
            Tools::Image,
            Tools::Read,
-            Tools::Image,


gotcha moment.

nvh0412 · 2024-10-25T00:40:04Z

Hello @SamSaffron, would it be possible for you to review my latest changes in this PR once again? This would ensure that we're on the same page about what we've been discussing above.

SamSaffron · 2024-10-25T04:26:58Z

app/models/ai_tool.rb

@@ -251,7 +272,8 @@ def self.presets
      },
      {
        preset_id: "image_generation",
-        name: "image_generation",
+        name: "Image Generation",


Image Generation (Flux)

SamSaffron · 2024-10-25T04:28:40Z

Its looking great to me... it is getting a bit late on a Friday, will merge it first thing next week.

- Add unique index to the name column of the ai_tools table - correct our tests for AiToolController - tool_name field which will be used to represent to LLM - Add tool_name to Tools's presets - Add duplicate tools validation for AiPersona - Add unique constraint to the name column of the ai_tools table

SamSaffron · 2024-10-29T22:36:08Z

was about to merge this but the validation is not looking right to me.

The "name" stored in Persona should be:

custom:123 - for custom tools
Search - for buit in tools

The duplication check is against tool_name.

So in the case of something like ['custom:1', 'custom:2'] we would load up the custom tool and check tool_name... if both of them are tool_name "search" then ... trouble... this will not work. Similarly ... don't allow a custom tool called "search" with built in "search".

The UI should show Name everywhere.

We should have a test to cover this so it is clear how the validation works.

nvh0412 · 2024-10-29T23:04:34Z

Could we adjust inner_name (the first element in the tool array of the persona) of a Custom tool to include tool_name? This way, the validation logic can parse it and handle its validation logic. At the ai_persona level, inner_name seems to be the only reference point for meeting this requirement, lmk if you think there's a better place.

SamSaffron · 2024-10-29T23:09:36Z

we can migrate the information on tools so it is clearer, but its a very painful refactor:

Eg:

[{
   type: "custom",
   id: 123,
   force_tool: true,
   params: { ... ... }
}]

Storing name though is a no-go cause we would too easily drift (when you rename in the tools UI we would be stuck amending internal cached refs in persona.)

I think the only easy way of validating easily is loading up the information from custom tool in a batch during validate.

nvh0412 · 2024-10-29T23:33:30Z

Even if we refactor the custom tool data structure saved in the Persona, as in your example, we would still need to load the custom tool data to retrieve their tool_name, wouldn’t we?

You're right that adjusting the inner_name to include tool_name is a no-go. I’ll load the custom tool data in batches and handle the validation that way. Does that work for you? However, I’m also open to a more robust solution in this PR since there’s no rush to merge it.

SamSaffron · 2024-10-29T23:35:51Z

Yes sounds good!

nvh0412 · 2024-10-30T07:37:33Z

I think it would improve the user experience if we add the tool name to the selection box. This way, users can easily identify any duplicates.

nvh0412 force-pushed the feat-tool-name-validation branch 2 times, most recently from 32252ee to 0d8c3a4 Compare October 18, 2024 12:27

nvh0412 marked this pull request as ready for review October 18, 2024 12:37

xfalcox reviewed Oct 18, 2024

View reviewed changes

nvh0412 commented Oct 21, 2024

View reviewed changes

SamSaffron reviewed Oct 21, 2024

View reviewed changes

nvh0412 commented Oct 22, 2024

View reviewed changes

db/migrate/20241020010245_add_tool_name_to_ai_tools.rb Show resolved Hide resolved

nvh0412 commented Oct 23, 2024

View reviewed changes

SamSaffron reviewed Oct 25, 2024

View reviewed changes

nvh0412 force-pushed the feat-tool-name-validation branch from 2a8b067 to 5f797ed Compare October 27, 2024 02:00

nvh0412 added 2 commits October 30, 2024 16:15

DEV: Validate duplicate tool_name between builin tools and custom tools

4146a5d

lint

439bf5b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEATURE: Tool name validation #842

FEATURE: Tool name validation #842

nvh0412 commented Oct 18, 2024 •

edited

Loading

xfalcox Oct 18, 2024

nvh0412 Oct 18, 2024

SamSaffron commented Oct 21, 2024

nvh0412 Oct 21, 2024

SamSaffron Oct 21, 2024

SamSaffron Oct 21, 2024

SamSaffron commented Oct 21, 2024

SamSaffron commented Oct 23, 2024

nvh0412 commented Oct 23, 2024

nvh0412 Oct 23, 2024

nvh0412 commented Oct 25, 2024

SamSaffron Oct 25, 2024

SamSaffron commented Oct 25, 2024

SamSaffron commented Oct 29, 2024

nvh0412 commented Oct 29, 2024

SamSaffron commented Oct 29, 2024

nvh0412 commented Oct 29, 2024

SamSaffron commented Oct 29, 2024

nvh0412 commented Oct 30, 2024

FEATURE: Tool name validation #842

Are you sure you want to change the base?

FEATURE: Tool name validation #842

Conversation

nvh0412 commented Oct 18, 2024 • edited Loading

xfalcox Oct 18, 2024

Choose a reason for hiding this comment

nvh0412 Oct 18, 2024

Choose a reason for hiding this comment

SamSaffron commented Oct 21, 2024

nvh0412 Oct 21, 2024

Choose a reason for hiding this comment

SamSaffron Oct 21, 2024

Choose a reason for hiding this comment

SamSaffron Oct 21, 2024

Choose a reason for hiding this comment

SamSaffron commented Oct 21, 2024

SamSaffron commented Oct 23, 2024

nvh0412 commented Oct 23, 2024

nvh0412 Oct 23, 2024

Choose a reason for hiding this comment

nvh0412 commented Oct 25, 2024

SamSaffron Oct 25, 2024

Choose a reason for hiding this comment

SamSaffron commented Oct 25, 2024

SamSaffron commented Oct 29, 2024

nvh0412 commented Oct 29, 2024

SamSaffron commented Oct 29, 2024

nvh0412 commented Oct 29, 2024

SamSaffron commented Oct 29, 2024

nvh0412 commented Oct 30, 2024

nvh0412 commented Oct 18, 2024 •

edited

Loading