implement closed-notebook search #174287

andreamah · 2023-02-13T22:14:24Z

An introductory PR for closed-notebook search. This only searches inputs in closed notebooks, and might have a few extra bugs with opening at the correct position, but it is behind an experimental flag (off by default for now).

andreamah · 2023-06-23T16:42:31Z

src/vs/workbench/contrib/search/browser/searchModel.ts

@@ -247,6 +258,10 @@ export class CellMatch {
 	}


+	setCellModel(cell: ICellViewModel) {


The idea is that the model for the cell will be CellSearchModel until it's opened. Then, you can set the proper ICellViewModel. CellSearchModel just has simple info from deserialization like inputs.

andreamah · 2023-06-23T16:45:30Z

src/vs/workbench/contrib/search/browser/searchModel.ts

+				const fileMatch = cellMatches.length > 0 ? {
+					resource: uri, cellResults: cellMatches
+				} : null;
+				results.set(uri, fileMatch);


This shouldn't create a race condition, since all URIs are unique

roblourens

Overall it looks like the right approach to me, nice work

src/vs/workbench/contrib/search/browser/search.contribution.ts

roblourens · 2023-06-28T18:36:24Z

src/vs/workbench/contrib/search/browser/searchModel.ts

+		const uris = new ResourceSet(result, uri => this.uriIdentityService.extUri.getComparisonKey(uri));
+		return Array.from(uris.keys());
+	}
+	private async getClosedNotebookResults(textQuery: ITextQuery, scannedFiles: ResourceSet, token: CancellationToken): Promise<{ results: ResourceMap<IFileMatchWithCells | null>; limitHit: boolean }> {


Just a code organization note, I feel like we should try to move code that does the search out of SearchModel and into some service (SearchService or a NotebookSearchService?), and try to keep SearchModel as just a model for the results. We were already doing the local file update from here since forever which maybe we should have avoided. This is fine for now but something to think about

roblourens · 2023-06-28T18:37:40Z

src/vs/workbench/contrib/search/browser/searchModel.ts

+				filePattern: include,
+				folderQueries: textQuery.folderQueries
+			};
+			return this.searchService.fileSearch(


How many file searches does this trigger? One per notebook type?

Does this apply the max result count by default?

iirc, yes, it is once per notebook. Each of them provide their own glob pattern. I was originally planning on concatenating all the globs into one glob using brace expansion, but ripgrep doesn't support nested braces in glob patterns.

Maybe there's a smart way to parse the globs and build one single pattern than matches the others

The most straightforward way of doing this would likely be with using braces ({ globA, globB, globC }), but if the individual globs have braces in them, ripgrep won't support this because it doesn't support nested alternate groups.

We could perform brace expansion, but I was thinking that it'd be an optimization for later.

roblourens · 2023-06-28T18:38:40Z

src/vs/workbench/contrib/search/browser/searchModel.ts

+
+		const start = Date.now();
+
+		const filesToScan = await this.runFileQueries(includes, token, textQuery);


This would be better if we could stream file results back, alas file search does not support it. But you could structure it so that you can handle results from each query as they complete, rather than wait for all queries to finish

roblourens · 2023-06-28T18:40:18Z

src/vs/workbench/contrib/search/browser/searchModel.ts

@@ -2004,21 +2130,33 @@ export class SearchModel extends Disposable {
 	}

 	async notebookSearch(query: ITextQuery, token: CancellationToken, onProgress?: (result: ISearchProgressItem) => void): Promise<{ completeData: ISearchComplete; scannedFiles: ResourceSet }> {


Ideally the notebook search could be done in parallel with the basic text search

Yes, that'd be ideal. I think there can be a lot more parallel code than what's there now, but we also need to have a list of open notebooks that the basic text search shouldn't find results for. Either that, or it does the whole search in parallel and then we dispose of results we don't need. I'll have to see what performs better.

roblourens · 2023-06-28T18:40:32Z

src/vs/workbench/contrib/search/browser/searchModel.ts

+		let closedResults = undefined;
+		if (experimentalNotebooksEnabled) {
+			const locallyScannedNotebookFiles = new ResourceSet([...localResults.results.keys()], uri => this.uriIdentityService.extUri.getComparisonKey(uri));
+			closedResults = await this.getClosedNotebookResults(query, locallyScannedNotebookFiles, token);


Can closed notebooks be searched in parallel with open notebooks?

I think that it currently needs a list of notebooks that are already open, but that shouldn't be too bad to provide ahead of time.

The same applies to this as #174287 (comment)

roblourens · 2023-06-28T18:45:22Z

src/vs/workbench/contrib/search/browser/searchModel.ts

+		const deserializedNotebooks = new ResourceMap<NotebookTextModel>();
+		const textModels = this.notebookService.getNotebookTextModels();
+		for (const notebook of textModels) {
+			deserializedNotebooks.set(notebook.uri, notebook);


Are these notebooks that are already open? But won't you find them from the local notebook search?

These are notebooks that have already been deserialized and the notebook side has cached the deserialized info.

roblourens · 2023-06-28T18:46:08Z

src/vs/workbench/contrib/search/browser/searchModel.ts

+		this._entries = new ResourceMap<INotebookDataEditInfo>(uri => this.uriIdentityService.extUri.getComparisonKey(uri));
+	}
+
+	async getSerializer(notebookUri: URI): Promise<INotebookSerializer | undefined> {


Should be private

roblourens · 2023-06-28T18:48:40Z

src/vs/workbench/contrib/search/browser/searchModel.ts

+	}
+
+	async getNotebookData(notebookUri: URI): Promise<NotebookData> {
+		const mTime = (await this.fileService.stat(notebookUri)).mtime;


There are a lot of back and forth calls to the EH for this code to run and I'm wondering whether more of it could actually live in the extension host process. That could hugely cut down on your latency. However it might be hard to interact with notebook serializers from the EH (even though that's where they live) and the code that does things like checking whether a notebook is already open and using its model of course has to stay in the renderer process.

This is something that I talked about with @rebornix. I'll probably move it over in a future PR (or at least try to reduce the number of calls)

andreamah · 2023-07-01T02:13:37Z

I probably want to merge in what I have now for this, but my next work forward would look something like:

Improve performance (move closed notebook search to extension host, stream results back while we get them in closed notebook search, make text search and notebook search in parallel).
Possibly search in outputs for closed notebooks (would need to think a bit more about how filtering would work for this).
unit tests

deserialize script to measure perf

e22a86e

VSCodeTriageBot assigned andreamah Feb 13, 2023

andreamah added 13 commits April 11, 2023 15:36

Merge branch 'main' into andreamah/notebook-deserialize-experiment

514ac0a

move deserialize experiment

2271b82

print cell source text

6088a72

Merge branch 'main' into andreamah/notebook-deserialize-experiment

3765abf

show closed-notebook matches in search view

2abfedc

add ability to open notebook matches

7b4703e

Merge branch 'main' into andreamah/notebook-deserialize-experiment

8c96a2a

add deserialize timing and per-session caching

cc6cae4

only initialize serializer info once

bc39111

merge main

51a02d3

deserialize in async

9cdaaf9

cleanup and add todos

1808216

fix tests

6e9c3c3

andreamah changed the title ~~deserialize script to measure perf~~ implement closed-notebook search Jun 21, 2023

andreamah added 8 commits June 22, 2023 14:03

find more than just ipynb in notebook search

b47572c

Merge branch 'main' into andreamah/notebook-deserialize-experiment

e1ea6e8

remove outdated conditional

b121ca9

remove need to nested braces in search glob

7c70de4

add experimental setting

d2a995d

remove test deserialization script

63ef65e

fix tests

4991c1d

reduce log serverities

fe955fa

andreamah commented Jun 23, 2023

View reviewed changes

remove outdated comments

3672fd5

andreamah requested review from roblourens and rebornix June 23, 2023 16:52

andreamah marked this pull request as ready for review June 23, 2023 16:55

VSCodeTriageBot added this to the June 2023 milestone Jun 23, 2023

andreamah modified the milestones: June 2023, July 2023 Jun 27, 2023

roblourens reviewed Jun 28, 2023

View reviewed changes

andreamah added 4 commits June 29, 2023 17:44

run closed and open notebook search in parallel

35bd5bf

mention rich content in experimental setting

d65a730

add maxResults to notebook fileSearch

cbbe5a9

move notebook search to INotebookSearchService

09fe8b1

andreamah closed this Jul 1, 2023

andreamah reopened this Jul 1, 2023

fix searchmodel tests

5101980

andreamah requested a review from roblourens July 4, 2023 18:01

Merge branch 'main' into andreamah/notebook-deserialize-experiment

b916d94

roblourens approved these changes Jul 11, 2023

View reviewed changes

andreamah merged commit 60578ed into main Jul 11, 2023
5 checks passed

andreamah deleted the andreamah/notebook-deserialize-experiment branch July 11, 2023 19:11

github-actions bot locked and limited conversation to collaborators Aug 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement closed-notebook search #174287

implement closed-notebook search #174287

andreamah commented Feb 13, 2023 •

edited

Loading

andreamah Jun 23, 2023

andreamah Jun 23, 2023

roblourens left a comment

roblourens Jun 28, 2023

roblourens Jun 28, 2023

roblourens Jun 28, 2023

andreamah Jun 28, 2023

roblourens Jul 6, 2023

andreamah Jul 6, 2023

roblourens Jun 28, 2023

roblourens Jun 28, 2023

andreamah Jun 28, 2023

roblourens Jun 28, 2023

andreamah Jun 28, 2023

andreamah Jun 28, 2023

roblourens Jun 28, 2023

andreamah Jun 28, 2023

roblourens Jun 28, 2023

roblourens Jun 28, 2023

andreamah Jun 28, 2023

andreamah commented Jul 1, 2023 •

edited

Loading

		@@ -247,6 +258,10 @@ export class CellMatch {
		}


		setCellModel(cell: ICellViewModel) {


		const start = Date.now();

		const filesToScan = await this.runFileQueries(includes, token, textQuery);

		@@ -2004,21 +2130,33 @@ export class SearchModel extends Disposable {
		}

		async notebookSearch(query: ITextQuery, token: CancellationToken, onProgress?: (result: ISearchProgressItem) => void): Promise<{ completeData: ISearchComplete; scannedFiles: ResourceSet }> {

implement closed-notebook search #174287

implement closed-notebook search #174287

Conversation

andreamah commented Feb 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roblourens left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreamah commented Jul 1, 2023 • edited Loading

andreamah commented Feb 13, 2023 •

edited

Loading

andreamah commented Jul 1, 2023 •

edited

Loading