-
Notifications
You must be signed in to change notification settings - Fork 160
chore: adds field embeddings validation for quantization "none" and warn when vectorSearch is not configured correctly #717
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
base: main
Are you sure you want to change the base?
Changes from all commits
198b36e
5cd2e51
fbb3d0f
6c3142b
d9bd870
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,18 +12,17 @@ import { collectCursorUntilMaxBytesLimit } from "../../../helpers/collectCursorU | |
| import { operationWithFallback } from "../../../helpers/operationWithFallback.js"; | ||
| import { AGG_COUNT_MAX_TIME_MS_CAP, ONE_MB, CURSOR_LIMITS_TO_LLM_TEXT } from "../../../helpers/constants.js"; | ||
| import { LogId } from "../../../common/logger.js"; | ||
| import { AnyVectorSearchStage, VectorSearchStage } from "../mongodbSchemas.js"; | ||
| import { AnyAggregateStage, VectorSearchStage } from "../mongodbSchemas.js"; | ||
| import { | ||
| assertVectorSearchFilterFieldsAreIndexed, | ||
| type VectorSearchIndex, | ||
| } from "../../../helpers/assertVectorSearchFilterFieldsAreIndexed.js"; | ||
|
|
||
| export const AggregateArgs = { | ||
| pipeline: z.array(z.union([AnyVectorSearchStage, VectorSearchStage])).describe( | ||
| `An array of aggregation stages to execute. | ||
| const pipelineDescription = `\ | ||
| An array of aggregation stages to execute. | ||
| \`$vectorSearch\` **MUST** be the first stage of the pipeline, or the first stage of a \`$unionWith\` subpipeline. | ||
| ### Usage Rules for \`$vectorSearch\` | ||
| - **Unset embeddings:** | ||
| - **Unset embeddings:** | ||
| Unless the user explicitly requests the embeddings, add an \`$unset\` stage **at the end of the pipeline** to remove the embedding field and avoid context limits. **The $unset stage in this situation is mandatory**. | ||
| - **Pre-filtering:** | ||
| If the user requests additional filtering, include filters in \`$vectorSearch.filter\` only for pre-filter fields in the vector index. | ||
|
|
@@ -32,20 +31,26 @@ If the user requests additional filtering, include filters in \`$vectorSearch.fi | |
| For all remaining filters, add a $match stage after $vectorSearch. | ||
| ### Note to LLM | ||
| - If unsure which fields are filterable, use the collection-indexes tool to determine valid prefilter fields. | ||
| - If no requested filters are valid prefilters, omit the filter key from $vectorSearch.` | ||
| ), | ||
| responseBytesLimit: z.number().optional().default(ONE_MB).describe(`\ | ||
| - If no requested filters are valid prefilters, omit the filter key from $vectorSearch.\ | ||
| `; | ||
|
|
||
| export const getAggregateArgs = (vectorSearchEnabled: boolean) => | ||
| ({ | ||
| pipeline: z | ||
| .array(vectorSearchEnabled ? z.union([AnyAggregateStage, VectorSearchStage]) : AnyAggregateStage) | ||
| .describe(pipelineDescription), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The pipeline description here is static and includes references to vector search - should we modify it so that we don't include them if the feature is disabled?
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's certainly an option but I think that the description applies in a generic sense to the pipeline and I don't think it would be confusing for the LLMs. Or do you feel otherwise?
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, most of the description is usage rules for the |
||
| responseBytesLimit: z.number().optional().default(ONE_MB).describe(`\ | ||
| The maximum number of bytes to return in the response. This value is capped by the server's configured maxBytesPerQuery and cannot be exceeded. \ | ||
| Note to LLM: If the entire aggregation result is required, use the "export" tool instead of increasing this limit.\ | ||
| `), | ||
| }; | ||
| }) as const; | ||
|
|
||
| export class AggregateTool extends MongoDBToolBase { | ||
| public name = "aggregate"; | ||
| protected description = "Run an aggregation against a MongoDB collection"; | ||
| protected argsShape = { | ||
| ...DbOperationArgs, | ||
| ...AggregateArgs, | ||
| ...getAggregateArgs(this.isFeatureEnabled("vectorSearch")), | ||
| }; | ||
| public operationType: OperationType = "read"; | ||
|
|
||
|
|
||
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.
You use warn in other places but
console.warnhere, is it intentional?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.
Ahh - that's because the place where we use
warnis where the console.warn is being provided through the function arguments. Likely for some tests. I had the tests covered differently so it should be fine.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.
Actually no, the new warning is not covered. I will write a test quickly.