Notebook search based on data model

[fcollonval]

References

This is a preliminary work for #11574 that switches the search feature in the notebook from view (aka CodeMirror and DOM nodes) search to data model (aka modelDB) for the inputs. The search on outputs is disabled by default and will looked at the rendered DOM tree if activated (when using virtual rendering this will trigger a warning that it may takes some time as outputs rendering will be needed).

Code changes

• Refactor @jupyterlab/documentsearch:
  • Simplify the public API
  • Standardize how to define filters (only boolean type supported)
  • Move search provider to their respective package (notebook in @jupyterlab/notebook, text editor in @jupyterlab/fileeditor)
• Update search when cell list changes
• Don't change hidden state of input cells nor rendered state of markdown cells (this improve user experience and reduce performance related issue due to CodeMirror refresh).
  On Markdown cells, they are unrendered when the user is highlighting next or previous match that are in a markdown cell.
• Simplify the OutputArea widget (and correct some bugs) as if trimmed the output area will only show the first outputs (following Only show the head of the outputs and ensure iopub outputs are correctly displayed #11457)

User-facing changes

Backwards-incompatible changes

ISearchProvider API is modified.

[jupyterlab-probot]

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

[fcollonval]

Performance of this PR has been tested manually on the large notebook not_so_obvious_python_stuff.ipynb mentioned in #6756

In the issue, it was mentioned that the gain for the tested notebook is (presumably gain obtained by the associated PR #6824):

I managed to cut the time of searching for t in the large notebook from about 14s to 7s

In the screencast above we can see that the search for t or test takes a couple of seconds. So I consider we are good to go regarding performance.

[echarles]

Awesome @fcollonval

Will a mimetype renderer be obliged to support this new search capability? In other words, what are the impacts for existing renderers such as e.g. https://github.com/jupyterlab/jupyter-renderers?

[fcollonval]

Will a mimetype renderer be obliged to support this new search capability? In other words, what are the impacts for existing renderers such as e.g. https://github.com/jupyterlab/jupyter-renderers?

No, the additional API has two folds:

• In rendermime interfaces, new optional API:

    /**
     * Render a mime model.
     *
     * @param model - The mime model to render.
     * @param highlights - The highlights to render.
     *
     * @returns A promise which resolves when rendering is complete.
     *
     * #### Notes
     * This method may be called multiple times during the lifetime
     * of the widget to update it if and when new data is available.
     */
    renderModel(model: IMimeModel, highlights?: IHighlight[]): Promise<void>;

    /**
     * Highlights text fragment on the widget.
     *
     * @param highlights - The highlights to render.
     *
     * @returns A promise which resolves when rendering is complete.
     *
     * #### Notes
     * The highlights are sorted from the lowest to the highest position.
     *
     * This method may be called multiple times during the lifetime
     * of the widget to update it if and when new data is available.
     */
    renderHighlights?(highlights: IHighlight[]): Promise<void>;

• Register a search engine for additional mimetype:

/**
 * A plugin providing search engine for cell outputs.
 */
const search: JupyterFrontEndPlugin<void> = {
  id: '@jupyterlab/rendermime-extension:search',
  requires: [ISearchProviderRegistry],
  activate: (app: JupyterFrontEnd, searchRegistry: ISearchProviderRegistry) => {
    textRendererFactory.mimeTypes.forEach(mimeType => {
      searchRegistry.registerMimeTypeSearchEngine(mimeType, TextSearchEngine);
    });

    [
      ...markdownRendererFactory.mimeTypes,
      'text/x-ipythongfm',
      'text/x-markdown',
      'text/x-gfm'
    ].forEach(mimeType => {
      searchRegistry.registerMimeTypeSearchEngine(
        mimeType,
        MarkdownSearchEngine
      );
    });

    htmlRendererFactory.mimeTypes.forEach(mimeType => {
      searchRegistry.registerMimeTypeSearchEngine(
        mimeType,
        HTMLStringSearchEngine
      );
    });
  },
  autoStart: true
};

---

Test using jupyterlab-geojson (only requires upgrading the dependencies to v4 alpha):

[fcollonval]

Comments from the performance meeting:

• Document decision done: like ignoring the link markdown
  william: encourage you to document the choices related to the subtle things like links, math in markdown, ansi output messages. I would be thrilled if there was an official jupyter specification of what search should do, and then client implementors could try to follow that spec.
• Keep the GenericSearch for not supported outputs
• Look at other tools like VSCode to see what they are doing:
  • VSCode: search in the editor source only
    • No search on outputs
    • If there is a hit in markdown cell, it is switched from rendered to the editor when the user is pressing next/previous button

Edit for VSCode 1.64 update: it is now possible to search for in rendered markdown cells and cell outputs (see announcement). For markdown cell, this duplicate the search in the rendered version (for example if the search matches in both the source and the rendered version, the counter hit will be 2 not one). The feature is optional (new filters available) and turn off by default.

The code is there. It search on the model. Then if filter requires it, it will render the outputs. And finally it will search on the rendered markup and outputs (if requested).

[fcollonval]

Thanks @afshin

[fcollonval]

Indeed this is not needed anymore as the implementation provides a number in all cases. So I removed it.

[fcollonval]

That will actually be called in super.dispose().

[fcollonval]

Signal.clearData(this); is call is the base class (similarly to Widget)

[fcollonval]

Signal.clearData(this); is called by the base class VDomModel.dispose. So I'm only reflecting the disconnection as I like the practice to explicit set up and clean up of signal as we do for event listener.

[fcollonval]

please update galata snapshots

[afshin]

Thank you @fcollonval! This is awesome 🚀

[github-actions]

Benchmark report

The execution time (in milliseconds) are grouped by test file, test type and browser.
For each case, the following values are computed: min <- [1st quartile - median - 3rd quartile] -> max.

The mean relative comparison is computed with 95% confidence.

Results table

Test file	large_code_notebook	large_md_notebook
open
chromium
actual	16375 <- [17317 - 17890 - 18602] -> 20214	2836 <- [3016 - 3052 - 3112] -> 3359
expected	15746 <- [17772 - 18162 - 18766] -> 30749	2698 <- [2878 - 2990 - 3107] -> 6525
Mean relative change	-2.3% ± 1.9%	1.0% ± 2.6%
switch-from
chromium
actual	639 <- [687 - 710 - 736] -> 976	458 <- [486 - 498 - 515] -> 712
expected	633 <- [685 - 703 - 732] -> 938	424 <- [472 - 487 - 503] -> 1029
Mean relative change	1.6% ± 2.8%	4.3% ± 4.1%
switch-to
chromium
actual	363 <- [448 - 486 - 496] -> 554	294 <- [318 - 326 - 336] -> 372
expected	394 <- [448 - 483 - 492] -> 1040	277 <- [300 - 313 - 328] -> 698
Mean relative change	-0.3% ± 3.0%	2.9% ± 3.0%
close
chromium
actual	814 <- [1127 - 1144 - 1167] -> 3220	504 <- [529 - 538 - 551] -> 623
expected	1054 <- [1118 - 1137 - 1166] -> 2800	459 <- [506 - 522 - 554] -> 631
Mean relative change	-1.0% ± 6.1%	2.6% ± 1.8%

Changes are computed with expected as reference.

[krassowski]

@fcollonval it seems to me that .jp-mimeType-highlight is not used, is this correct or am I missing something?

[fcollonval]

This is correct - this is an artifact of my trial for searching the unrendered output.