-
Notifications
You must be signed in to change notification settings - Fork 3.3k
test:add text embedding function testcases in go client #43875
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: master
Are you sure you want to change the base?
Conversation
Signed-off-by: zhuwenxing <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zhuwenxing The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Signed-off-by: zhuwenxing <[email protected]>
Signed-off-by: zhuwenxing <[email protected]>
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.
Pull Request Overview
This PR adds comprehensive test coverage for text embedding functionality in the Go client by introducing a new test file with various test cases for TEI (Text Embedding Inference) integration.
Key Changes
- Test Suite Addition: Introduces comprehensive text embedding test cases covering collection creation, data operations, search functionality, and error handling scenarios.
- Helper Function Enhancements: Adds utility functions for TEI endpoint configuration, text embedding function creation, field generation, and embedding consistency validation.
- Configuration Support: Adds command-line flags for TEI endpoint and model dimension configuration to support different test environments.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
tests/go_client/testcases/text_embedding_test.go | Main test file containing 16 comprehensive test cases for text embedding functionality |
tests/go_client/testcases/helper/test_setup.go | Adds TEI endpoint and model dimension configuration flags |
tests/go_client/testcases/helper/schema_helper.go | Adds helper function for creating text embedding schema options |
tests/go_client/testcases/helper/function_helper.go | Adds helper function for creating text embedding functions with parameters |
tests/go_client/testcases/helper/field_helper.go | Adds TextEmbedding collection type and helper functions for field generation |
tests/go_client/testcases/helper/data_helper.go | Adds utility functions for text generation, similarity calculation, and TEI API interaction |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
@@ -557,7 +708,7 @@ func GenColumnsBasedSchema(schema *entity.Schema, option *GenDataOption) ([]colu | |||
if option.fieldName == "" { | |||
option.fieldName = field.Name | |||
} | |||
if slices.Contains(GetBm25FunctionsOutputFields(schema), field.Name) { | |||
if slices.Contains(GetAllFunctionsOutputFields(schema), field.Name) { |
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 function name changed from GetBm25FunctionsOutputFields to GetAllFunctionsOutputFields, but this change affects existing BM25-only functionality. Consider keeping the original function for backward compatibility or ensuring all callers are updated consistently.
if slices.Contains(GetAllFunctionsOutputFields(schema), field.Name) { | |
if slices.Contains(GetBm25FunctionsOutputFields(schema), field.Name) { |
Copilot uses AI. Check for mistakes.
} | ||
|
||
// CallTEIDirectly calls TEI endpoint directly to get embeddings | ||
func CallTEIDirectly(endpoint string, texts []string) ([][]float32, error) { |
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 function doesn't validate the HTTP response status code before attempting to unmarshal the response body. Non-200 status codes could contain error messages instead of embeddings, leading to confusing error messages.
Copilot uses AI. Check for mistakes.
} | ||
|
||
// Make HTTP request to TEI | ||
resp, err := http.Post(endpoint+"/embed", "application/json", bytes.NewBuffer(jsonData)) |
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 endpoint URL is constructed by string concatenation without proper URL path joining. If the endpoint already ends with a slash, this could result in double slashes. Consider using url.JoinPath or similar for proper URL construction.
resp, err := http.Post(endpoint+"/embed", "application/json", bytes.NewBuffer(jsonData)) | |
resp, err := http.Post(url.JoinPath(endpoint, "embed"), "application/json", bytes.NewBuffer(jsonData)) |
Copilot uses AI. Check for mistakes.
/kind improvement