pinecone-io / pinecone-vercel-starter

Pinecone + Vercel AI SDK Starter
https://pinecone-vercel-example.vercel.app
418 stars 127 forks source link

Minor refactoring #10

Closed athrael-soju closed 11 months ago

athrael-soju commented 1 year ago

Problem

Addition of a couple of environment variables, for ease of use. OPENAI_API_MODEL, OPENAI_API_EMBEDDING_MODEL Update of svelte library to ^4.0.5, as it causes npm install to fail with version ^3.29.0. we can always use npm i --legacy-peer-deps, but could cause issues down the road.

Solution

Describe the approach you took. Link to any relevant bugs, issues, docs, or other resources.

Type of Change

Test Plan

Describe specific steps for validating this change.

HarounAns commented 11 months ago

Hi @athrael-soju,

Thank you for the PR. I've reviewed the changes and have a few questions and comments:

  1. winkNLP Integration: I see that you've added winkNLP and its model wink-eng-lite-web-model. While these are potentially valuable additions, they seem to be a separate concern from the main intent of this PR, which is around environment variables and updating Svelte. For clarity and traceability, it might be best to introduce winkNLP in its own PR. This helps us review, test, and revert (if necessary) changes more efficiently.

  2. pnpm-lock.yaml Deletion: I noticed that the pnpm-lock.yaml file was deleted. Was this intentional? If so, could you elaborate on the reason? This lock file is crucial for ensuring consistent installations across different environments.

  3. .history: You added /.history/ to the .gitignore. What's the purpose of .history? Is it a tool or extension you're using locally? Just trying to understand its context in this project.

Thanks for your contributions! Looking forward to your clarifications.

athrael-soju commented 11 months ago

@HarounAns

The first 2 are actually not intentional and I will revert them. I think this may have happened due to switching between branches.

The addition of .history is related to Visual Studio code adding the history folder locally, when making changes.

I will revert all 3, as they are not directly related to the intent of this PR.

athrael-soju commented 11 months ago

Closing this, as not needed.