-
Notifications
You must be signed in to change notification settings - Fork 13
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
Add integrated inference #181
base: main
Are you sure you want to change the base?
Conversation
HashMap<String, String> inputsMap = new HashMap<>(); | ||
inputsMap.put("text", "Disease prevention"); | ||
SearchRecordsRequestQuery query = new SearchRecordsRequestQuery() | ||
.topK(4) | ||
.inputs(inputsMap); | ||
|
||
List<String> fields = new ArrayList<>(); | ||
fields.add("category"); | ||
fields.add("chunk_text"); | ||
|
||
// Wait for vectors to be upserted | ||
Thread.sleep(5000); | ||
|
||
SearchRecordsResponse recordsResponse = index.searchRecords(namespace, query, fields, null); |
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 think we should consider creating some API abstractions over the SearchRecordsRequestQuery
interface.
For example, it might feel more natural if the user could query with something like:
RecordsQuery query = new TextRecordsQuery("Disease prevention")
.topK(4);
index.searchRecords(namespace, query, ["category", "chunk_text"]);
We could also support ID and vector queries in a similar way?
vectorOperations.setCustomBaseUrl(protocol + config.getHost()); | ||
} | ||
|
||
public void upsertRecords(String namespace, List<UpsertRecord> upsertRecord) throws ApiException { |
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.
Can we add an overload that accepts List<Map<String, Object>>
?
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.
This feels kind of cumbersome:
UpsertRecord record1 = new UpsertRecord();
record1.id("rec1");
record1.putAdditionalProperty("category", "digestive system");
record1.putAdditionalProperty("chunk_text", "Apples are a great source of dietary fiber, which supports digestion and helps maintain a healthy gut.");
ArrayList<UpsertRecord> records = new ArrayList<>();
UpsertRecord record2 = new UpsertRecord();
record2.id("rec2");
record2.putAdditionalProperty("category", "cultivation");
record2.putAdditionalProperty("chunk_text", "Apples originated in Central Asia and have been cultivated for thousands of years, with over 7,500 varieties available today.");
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.
Overall this looks good, nice work! I've left a few comments around handling of ids in records, and a possibly breaking change to configureServerlessIndex
.
@@ -931,6 +931,18 @@ public RequestBody serialize(Object obj, String contentType) throws ApiException | |||
return RequestBody.create((File) obj, MediaType.parse(contentType)); | |||
} else if ("text/plain".equals(contentType) && obj instanceof String) { | |||
return RequestBody.create((String) obj, MediaType.parse(contentType)); | |||
} else if ("application/x-ndjson".equals(contentType)) { |
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.
This is fine for now, but you'll need to remember to redo this change every time you regenerate.
Alternatively, there's a couple options we've used in other clients:
- Updating the Java templates to inject this logic at code generation.
- Using some combination of bash/sed to manipulate the output after generation, as a part of the build script process.
for(Map<String, String> record: upsertRecords) { | ||
UpsertRecord upsertRecord = new UpsertRecord(); | ||
for (Map.Entry<String, String> entry : record.entrySet()) { | ||
if(entry.getKey().equals("_id")) { |
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.
Is it possible for an existing id
to be overwritten by this logic? The server will respond with a 400 if you try and send both id
and _id
:
{"error":{"code":"INVALID_ARGUMENT","message":"Error parsing request: Invalid input: Document cannot have both _id and id fields (_id='rec1', id='rec1')"},"status":400}
Maybe instead of mapping one to the other in the client we just pass through what the user has provided, and if they need to clean up ids they can? I just checked for the presence of one or the other on the record in typescript: https://github.com/pinecone-io/pinecone-ts-client/blob/e5b39d7a6c77836b5b0ae1f696ce7e6f6020f82e/src/data/vectors/upsertRecords.ts#L29-L37
* @throws ApiException If there is an issue with the search operation. This could include network errors, | ||
* invalid input data, or issues communicating with the Pinecone service. | ||
*/ | ||
public SearchRecordsResponse searchRecordsByText(String text, |
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.
Nice job handling the lack of optionals in all of these methods. 👍
* @return {@link IndexModel} representing the configured index. | ||
* @throws PineconeException if an error occurs during the operation, the index does not exist, or if any of the arguments are invalid. | ||
*/ | ||
public IndexModel configureServerlessIndex(String indexName, DeletionProtection deletionProtection, Map<String, String> tags) throws PineconeException { | ||
public IndexModel configureServerlessIndex(String indexName, |
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.
This would be a breaking change, right? In terms of it altering the method signature for configureServerlessIndex
.
If we're intending for these changes to release as a major version bump that's fine, but otherwise we may want to look at overloading this method too to keep things backwards compatible.
Problem
Add integrated inference to Java SDK.
Solution
The code was already generated since integrated inference was a part of 2025-01 api spec. So as a part of this PR, I have added the following features:
Example:
Type of Change
Test Plan
Added integration test that creates an index associated with a model, upserts and queries records.