Skip to content
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

Remove OpenAI embedding model support #4747

Merged
merged 4 commits into from
Jul 2, 2024

Conversation

janhartman
Copy link
Contributor

Test plan

N/A

@janhartman janhartman requested a review from rafax July 2, 2024 08:00
@@ -32,7 +32,7 @@ export interface RemoteSearchProvider {
isIgnored: boolean
}

export type EmbeddingsProvider = 'sourcegraph' | 'openai'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decided not to delete this enum in case we add more providers later on.

Copy link
Contributor

@rafax rafax left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question about index file clean-up, other than that LGTM

provider: 'openai',
endpoint: CODY_GATEWAY_PROD_ENDPOINT,
// empty prefix to keep backwards compatibility
indexPath: getIndexLibraryPath(''),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have some code that deletes the old library here @janhartman ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, good point. However, do we handle cleanups at all in Cody? I think we're just littering the users' machines all over the place 😓

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but seeing that this is the "last clean-up", I think it would make sense to try?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image So this is the structure of the library... The files are from the OpenAI model (we don't use a prefix for backwards compatibility) while the dirs correspond to SG-provided models. So we have to delete only the files. Honestly, I think we can wait until we drop local embeddings entirely and then just nuke this whole dir.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, in the meantime we can tell the users to delete everything (we will regenerate if needed)

@janhartman
Copy link
Contributor Author

@rafax should I update the changelog with this change? It shouldn't really affect users but still...

@rafax
Copy link
Contributor

rafax commented Jul 2, 2024

@rafax should I update the changelog with this change? It shouldn't really affect users but still...

I don't think we need to do that (we're removing a feature that is not available).

@janhartman janhartman merged commit 55bd34e into main Jul 2, 2024
18 of 19 checks passed
@janhartman janhartman deleted the jan/remove-openai-embedding-model branch July 2, 2024 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants