Upgrade to @pinecone-database/pinecone version 1.1.0 #14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
|
Hi As I've been actively working on an application that uses Pinecone 1.0, I encountered a particular issue related to the chunkedUpsert and createIndexIfNotExists functions. These functions have been deprecated in Pinecone 1.0, which prompted me to take the initiative to reimplement them as utilities that are compatible with Pinecone 1.0. Now, my question is: Do you have plans to integrate these updated utilities? it will ensure compatibility with Pinecone 1.0 Moreover, I'm more than willing to contribute to the integration process and help with any necessary modifications |
Hi @glody007, Thanks for the feedback and helpful pointers - do you have a fork or branch where you made those changes you could point me at? If those helper methods are indeed broken with the upgrade to 1.0, then I could take care of them as part of this pull request. Thank you! |
jhamon
reviewed
Sep 15, 2023
jhamon
reviewed
Sep 15, 2023
jhamon
reviewed
Sep 15, 2023
jhamon
reviewed
Sep 15, 2023
jhamon
reviewed
Sep 15, 2023
jhamon
approved these changes
Sep 15, 2023
I will PR your fork |
|
I made the code changes requested. Locally, I'm getting an error connecting to Pinecone, but I've supplied my own API key and an azure environment...still debugging... I've tried east azure and GCP and triple-cheked API keys...but I keep getting the same error. When I dump the pinecone config to the console I see the correct environment and API key values, and I'm setting them via:
- error node_modules/.pnpm/@pinecone-database+pinecone@1.0.1/node_modules/@pinecone-database/pinecone/dist/errors/handling.js (52:38) @ eval
- error Request failed to reach Pinecone. Verify you have the correct environment, project id, and index name configured.I've also tried:
🤔 @jhamon or @rschwabco LMK if you see something obvious that I've missed. |
zackproser
changed the title
I just submitted a pull request for the change here #zackproser#2 (comment) |
|
I suspect nextjs is doing something with fetch that causes a problem with the cross-fetch polyfill the client library is using to make network calls. Looking into it. |
|
Something like this should do the trick: |
I think @jhamon is right. If you comment out With without Sorry for the wall of text. Hope that helps. |
1 task
jhamon
added a commit
to pinecone-io/pinecone-ts-client
that referenced
this pull request
Sep 27, 2023
## Problem Next.js seems to stub out the cross-fetch polyfill which has the effect of silently breaking code which depends on cross-fetch to make API requests. Related bugs: - #124 - v1 migration problems in pinecone-io/pinecone-vercel-starter#14 ## Solution I have added a `getFetch` utility which looks for a `fetch` implementation already loaded in the global scope and returns the cross-fetch implementation only as a last resort. - Now when using our client library in a next.js project, this should result in the `@vercel/fetch` implementation being used in favor of the disabled polyfill. - When using this client library in other environments where next.js is not being used, it still activates the cross-fetch polyfill. - Also, I've exposed an experimental configuration option for anyone who wants to pass in a different fetch implementation to use. This could open up some flexibility for people with specific network configurations who need the ability to configure the http client being used in some way. If the user provides a fetch implementation through the `fetchApi` config option, it will take priority over a fetch in the global scope or the cross-fetch polyfill. ### Other changes - Added additional integration tests for `query` method - Added an explicit dependency on the `encoding` module to resolve a warning message [similar to this one](vercel/next.js#54109) ## Type of Change - [x] Bug fix (non-breaking change which fixes an issue) ## Test Plan I am currently testing this change in the context of work-in-progress to migrate our vercel-starter example to using the v1 version of the client. pinecone-io/pinecone-vercel-starter#14 - Checkout the PR branch from pinecone-vercel-starter in a sibling directory - Install my local changes to this client with `npm install ../pinecone-ts-client` - Try to run the starter project with `npm run dev` - Attempt to use the sample app and observe it progresses beyond the error related broken cross-fetch
|
Thanks for the fix @jhamon. Using version 1.1.0 seems to work and the upsert is working again. One thing, however, is that I don't seem to be getting the POST response log back, when When disabled, we can see this: When enabled: I can raise an issue, if this is related to this template. |
|
Thank you for the attention and helpful feedback @athrael-soju and @glody007 - we're getting closer here. This latest commit worked for me locally end to end and I was able to perform the expected demo. Requesting some more internal reviews now. |
zackproser
changed the title
jhamon
approved these changes
Sep 28, 2023
src/app/utils/pinecone.ts
Outdated
Comment on lines
11
to
21
| let exists = false | ||
| for (const index of indexes) { | ||
| if (index.name === process.env.PINECONE_INDEX!) { | ||
| exists = true | ||
| } | ||
| } | ||
|
|
||
| // Check if the desired index is present, else throw an error | ||
| if (!indexes.includes(process.env.PINECONE_INDEX!)) { | ||
| if (!exists) { | ||
| throw (new Error(`Index ${process.env.PINECONE_INDEX} does not exist`)) | ||
| } |
There was a problem hiding this comment.
I think this whole section lines 9-21 can be simplified as something like
const indexName: string = process.env.PINECONE_INDEX || '';
if (indexName === '') {
throw new Error('PINECONE_INDEX environment variable not set')
}
// Retrieve the list of indexes to check if expected index exists
const indexes = await pinecone.listIndexes()
if (indexes.filter(i => i.name === indexName).length !== 1) {
throw new Error(`Index ${indexName} does not exist`)
}I don't think having this exists variable around is doing anything helpful for readability.
src/app/utils/pinecone.ts
Outdated
| // Check if the desired index is present, else throw an error | ||
| if (!indexes.includes(process.env.PINECONE_INDEX!)) { | ||
| if (!exists) { | ||
| throw (new Error(`Index ${process.env.PINECONE_INDEX} does not exist`)) | ||
| } | ||
|
|
||
| // Get the Pinecone index | ||
| const index = pinecone!.Index(process.env.PINECONE_INDEX!); |
There was a problem hiding this comment.
If you use the generic type parameter in the method signature above, you also need to pass it here. Then typescript will understand types for metadata without the need to cast the query results elsewhere using as.
const index = pinecone.index<Metadata>(indexName);The capital version (pinecone.Index) should also work for backwards compatibility, but it's not the usage I typically show.
|
As I mentioned on slack, the 
|
|
@zackproser just a heads up that the next.js version used is outdated and may make sense to update next/react in this PR. If not, happy to raise a PR after this one is merged. I've done the update in my project and everything runs quite well, of not better. |
Co-authored-by: Jennifer Hamon <jhamon@pinecone.io>
Co-authored-by: Jennifer Hamon <jhamon@pinecone.io>
|
With the latest commit the unit tests defined via GitHub actions are passing on my fork: https://github.com/zackproser/pinecone-vercel-starter/actions/runs/6357115562/job/17267819591 |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
Describe the purpose of this change. What problem is being solved and why?
Blocked by pinecone-io/pinecone-ts-client#108
These changes upgrade the pinecone-vercel-starter template to use
@pinecone-database/pineconeversion 1.1.0Blocked on
Solution
Describe the approach you took. Link to any relevant bugs, issues, docs, or other resources.
These changes make the required signature changes and some minor logic changes to accommodate the new behavior of the v1 SDK.
Type of Change
Test Plan
Describe specific steps for validating this change.
Ran locally, but pinecone-io/pinecone-ts-client#108 is currently blocking this.