-
Notifications
You must be signed in to change notification settings - Fork 316
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
Automatically start Embeddings indexing #4091
Changes from all commits
9dc9df3
971a6a1
2fdb153
970c33b
ab459c5
93a7016
44646f6
5f91266
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -313,6 +313,11 @@ const register = async ( | |
searchViewProvider.initialize() | ||
} | ||
|
||
if (localEmbeddings) { | ||
// kick-off embeddings initialization | ||
localEmbeddings.start() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Question to reviewers - is this the right place to start this? We start There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This sounds acceptable to me. |
||
} | ||
|
||
if (config.experimentalSupercompletions) { | ||
disposables.push(new SupercompletionProvider({ statusBar, chat: chatClient })) | ||
} | ||
|
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 am not an expert in this part of the code, so I need someone to verify my concern here.
It might be the case that
this.eagerlyLoad
will return true, even if there are partial embeddings present for the repo. In that case, we will need check the status and callthis.indexRetry
.I see it mentioned in the test plan that you have covered this case but I just wanted to double-check.
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.
Also a shameless plug: https://www.loom.com/share/4b2fb975f89447dcabeb2cce5c956ded?sid=3b0cd033-eb04-4453-9fe5-7f35f0c6ec20
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.
Thank you for checking and raising this - the existing treatment for partially indexed repos is that we require user action to continue indexing (screenshot), I tested that that this PR doesn't change that logic, I planned to have a separate PR for "automatic restart" once I understand why we don't automatically re-index in the first place (what comes to mind and what I saw in code is that some of the indexing processes can kill the agent, so automatic re-start could make Cody / agent crashloop?)
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.
@dominiccooney (as the original author of this) - is there a specific reason why we don't automatically re-start indexing after the extension is restarted? Would you be ok with making that change?
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 changed this PR to restart indexing on extension startup (when no errors occurred in the current run) if we're using Sourcegraph provider and the feature-flag is enabled.