From 7dd046a15dce96c75bbad70ac64f146055d7db71 Mon Sep 17 00:00:00 2001 From: Laurent Nguyen Date: Wed, 7 Oct 2020 08:39:04 +0200 Subject: [PATCH] Fix notebook kernel selection and auto-kernel-start (#254) * Fix bug: cannot select kernel. Properly plug-in kernelspecs middleware to redux store configuration * Properly auto-launch kernel with nteract's epic * Keep kernel lazy start for notebook viewer * Add unit tets --- src/Explorer/Notebook/NotebookClientV2.ts | 8 ++- .../NotebookComponentBootstrapper.tsx | 2 +- .../Notebook/NotebookComponent/epics.test.ts | 56 +------------------ .../Notebook/NotebookComponent/epics.ts | 34 ----------- .../Notebook/NotebookComponent/store.test.ts | 13 +++++ .../Notebook/NotebookComponent/store.ts | 51 ++++++++++------- .../Notebook/notebookClientV2.test.ts | 48 ++++++++++++++++ 7 files changed, 102 insertions(+), 110 deletions(-) create mode 100644 src/Explorer/Notebook/NotebookComponent/store.test.ts create mode 100644 src/Explorer/Notebook/notebookClientV2.test.ts diff --git a/src/Explorer/Notebook/NotebookClientV2.ts b/src/Explorer/Notebook/NotebookClientV2.ts index 29c62af89..e0643a05b 100644 --- a/src/Explorer/Notebook/NotebookClientV2.ts +++ b/src/Explorer/Notebook/NotebookClientV2.ts @@ -234,7 +234,13 @@ export class NotebookClientV2 { console.error(`${title}: ${message}`); }; - this.store = configureStore(initialState, params.contentProvider, traceErrorFct, [cacheKernelSpecsMiddleware]); + this.store = configureStore( + initialState, + params.contentProvider, + traceErrorFct, + [cacheKernelSpecsMiddleware], + !params.isReadOnly + ); // Additional configuration this.store.dispatch(configOption("editorType").action(params.cellEditorType ?? "monaco")); diff --git a/src/Explorer/Notebook/NotebookComponent/NotebookComponentBootstrapper.tsx b/src/Explorer/Notebook/NotebookComponent/NotebookComponentBootstrapper.tsx index 879753ee6..d7ed53c74 100644 --- a/src/Explorer/Notebook/NotebookComponent/NotebookComponentBootstrapper.tsx +++ b/src/Explorer/Notebook/NotebookComponent/NotebookComponentBootstrapper.tsx @@ -98,7 +98,7 @@ export class NotebookComponentBootstrapper { actions.fetchContentFulfilled({ filepath: undefined, model: NotebookComponentBootstrapper.wrapModelIntoContent(name, undefined, content), - kernelRef: undefined, // must be undefined or it will be auto-started by the epic + kernelRef: undefined, contentRef: this.contentRef }) ); diff --git a/src/Explorer/Notebook/NotebookComponent/epics.test.ts b/src/Explorer/Notebook/NotebookComponent/epics.test.ts index 97d63852a..1209bdd93 100644 --- a/src/Explorer/Notebook/NotebookComponent/epics.test.ts +++ b/src/Explorer/Notebook/NotebookComponent/epics.test.ts @@ -3,11 +3,11 @@ import { StateObservable } from "redux-observable"; import { Subject, of } from "rxjs"; import { toArray } from "rxjs/operators"; import { makeNotebookRecord } from "@nteract/commutable"; -import { actions, state } from "@nteract/core"; +import { actions, state, epics } from "@nteract/core"; import * as sinon from "sinon"; import { CdbAppState, makeCdbRecord } from "./types"; -import { launchWebSocketKernelEpic, autoStartKernelEpic } from "./epics"; +import { launchWebSocketKernelEpic } from "./epics"; import { NotebookUtil } from "../NotebookUtil"; import { sessions } from "rx-jupyter"; @@ -491,55 +491,3 @@ describe("launchWebSocketKernelEpic", () => { }); }); }); - -describe("autoStartKernelEpic", () => { - const contentRef = "fakeContentRef"; - const kernelRef = "fake"; - - it("automatically starts kernel when content fetch is successful if kernelRef is defined", async () => { - const state$ = new StateObservable(new Subject(), initialState); - - const action$ = of( - actions.fetchContentFulfilled({ - contentRef, - kernelRef, - filepath: "filepath", - model: {} - }) - ); - - const responseActions = await autoStartKernelEpic(action$, state$) - .pipe(toArray()) - .toPromise(); - - expect(responseActions).toMatchObject([ - { - type: actions.RESTART_KERNEL, - payload: { - contentRef, - kernelRef, - outputHandling: "None" - } - } - ]); - }); - - it("Don't start kernel when content fetch is successful if kernelRef is not defined", async () => { - const state$ = new StateObservable(new Subject(), initialState); - - const action$ = of( - actions.fetchContentFulfilled({ - contentRef, - kernelRef: undefined, - filepath: "filepath", - model: {} - }) - ); - - const responseActions = await autoStartKernelEpic(action$, state$) - .pipe(toArray()) - .toPromise(); - - expect(responseActions).toMatchObject([]); - }); -}); diff --git a/src/Explorer/Notebook/NotebookComponent/epics.ts b/src/Explorer/Notebook/NotebookComponent/epics.ts index 7b4feba18..c2f61d498 100644 --- a/src/Explorer/Notebook/NotebookComponent/epics.ts +++ b/src/Explorer/Notebook/NotebookComponent/epics.ts @@ -95,39 +95,6 @@ const addInitialCodeCellEpic = ( ); }; -/** - * Automatically start kernel if kernelRef is present. - * The kernel is normally lazy-started when a cell is being executed, but a running kernel is - * required for code completion to work. - * For notebook viewer, there is no kernel - * @param action$ - * @param state$ - */ -export const autoStartKernelEpic = ( - action$: Observable, - state$: StateObservable -): Observable<{} | actions.CreateCellBelow> => { - return action$.pipe( - ofType(actions.FETCH_CONTENT_FULFILLED), - mergeMap(action => { - const state = state$.value; - const { contentRef, kernelRef } = action.payload; - - if (!kernelRef) { - return EMPTY; - } - - return of( - actions.restartKernel({ - contentRef, - kernelRef, - outputHandling: "None" - }) - ); - }) - ); -}; - /** * Updated kernels.formWebSocketURL so we pass the userId as a query param */ @@ -879,7 +846,6 @@ const closeContentFailedToFetchEpic = ( export const allEpics = [ addInitialCodeCellEpic, - autoStartKernelEpic, focusInitialCodeCellEpic, notificationsToUserEpic, launchWebSocketKernelEpic, diff --git a/src/Explorer/Notebook/NotebookComponent/store.test.ts b/src/Explorer/Notebook/NotebookComponent/store.test.ts new file mode 100644 index 000000000..d0a0ad82d --- /dev/null +++ b/src/Explorer/Notebook/NotebookComponent/store.test.ts @@ -0,0 +1,13 @@ +import { getCoreEpics } from "./store"; +import { epics } from "@nteract/core"; + +describe("configure redux store", () => { + it("configures store with correct epic if based on autoStartKernelOnNotebookOpen", () => { + // For now, assume launchKernelWhenNotebookSetEpic is the last epic + let filteredEpics = getCoreEpics(true); + expect(filteredEpics.pop()).toEqual(epics.launchKernelWhenNotebookSetEpic); + + filteredEpics = getCoreEpics(false); + expect(filteredEpics.pop()).not.toEqual(epics.launchKernelWhenNotebookSetEpic); + }); +}); diff --git a/src/Explorer/Notebook/NotebookComponent/store.ts b/src/Explorer/Notebook/NotebookComponent/store.ts index 17594d710..924b81e2b 100644 --- a/src/Explorer/Notebook/NotebookComponent/store.ts +++ b/src/Explorer/Notebook/NotebookComponent/store.ts @@ -1,6 +1,6 @@ import { AppState, epics as coreEpics, reducers, IContentProvider } from "@nteract/core"; import { compose, Store, AnyAction, Middleware, Dispatch, MiddlewareAPI } from "redux"; -import { createEpicMiddleware, Epic } from "redux-observable"; +import { Epic } from "redux-observable"; import { allEpics } from "./epics"; import { coreReducer, cdbReducer } from "./reducers"; import { catchError } from "rxjs/operators"; @@ -15,7 +15,8 @@ export default function configureStore( initialState: Partial, contentProvider: IContentProvider, onTraceFailure: (title: string, message: string) => void, - customMiddlewares?: Middleware<{}, any, Dispatch>[] + customMiddlewares?: Middleware<{}, any, Dispatch>[], + autoStartKernelOnNotebookOpen?: boolean ): Store { /** * Catches errors in reducers @@ -54,6 +55,29 @@ export default function configureStore( return epics.map(epic => protect(epic)); }; + const filteredCoreEpics = getCoreEpics(autoStartKernelOnNotebookOpen); + + const mythConfigureStore = makeConfigureStore()({ + packages: [configuration], + reducers: { + app: reducers.app, + core: coreReducer as any, + cdb: cdbReducer + }, + epics: protectEpics([...filteredCoreEpics, ...allEpics]), + epicDependencies: { contentProvider }, + epicMiddleware: customMiddlewares.concat(catchErrorMiddleware), + enhancer: composeEnhancers + }); + + const store = mythConfigureStore(initialState as any); + + // TODO Fix typing issue here: createStore() output type doesn't quite match AppState + // return store as Store; + return store as any; +} + +export const getCoreEpics = (autoStartKernelOnNotebookOpen: boolean): Epic[] => { // This list needs to be consistent and in sync with core.allEpics until we figure // out how to safely filter out the ones we are overriding here. const filteredCoreEpics = [ @@ -79,22 +103,9 @@ export default function configureStore( coreEpics.sendInputReplyEpic ]; - const mythConfigureStore = makeConfigureStore()({ - packages: [configuration], - reducers: { - app: reducers.app, - core: coreReducer as any, - cdb: cdbReducer - }, - epics: protectEpics([...filteredCoreEpics, ...allEpics]), - epicDependencies: { contentProvider }, - epicMiddleware: [catchErrorMiddleware], - enhancer: composeEnhancers - }); + if (autoStartKernelOnNotebookOpen) { + filteredCoreEpics.push(coreEpics.launchKernelWhenNotebookSetEpic); + } - const store = mythConfigureStore(initialState as any); - - // TODO Fix typing issue here: createStore() output type doesn't quite match AppState - // return store as Store; - return store as any; -} + return filteredCoreEpics; +}; diff --git a/src/Explorer/Notebook/notebookClientV2.test.ts b/src/Explorer/Notebook/notebookClientV2.test.ts new file mode 100644 index 000000000..363d271b5 --- /dev/null +++ b/src/Explorer/Notebook/notebookClientV2.test.ts @@ -0,0 +1,48 @@ +jest.mock("./NotebookComponent/store"); +jest.mock("@nteract/core"); +import { NotebookClientV2 } from "./NotebookClientV2"; +import configureStore from "./NotebookComponent/store"; +import { defineConfigOption } from "@nteract/mythic-configuration"; + +describe("auto start kernel", () => { + it("configure autoStartKernelOnNotebookOpen properly depending whether notebook is/is not read-only", async () => { + (configureStore as jest.Mock).mockReturnValue({ + dispatch: () => { + /* noop */ + } + }); + + defineConfigOption({ + label: "editorType", + key: "editorType", + defaultValue: "foo" + }); + + defineConfigOption({ + label: "autoSaveInterval", + key: "autoSaveInterval", + defaultValue: 1234 + }); + + [true, false].forEach(isReadOnly => { + new NotebookClientV2({ + connectionInfo: { + authToken: "autToken", + notebookServerEndpoint: "notebookServerEndpoint" + }, + databaseAccountName: undefined, + defaultExperience: undefined, + isReadOnly, + contentProvider: undefined + }); + + expect(configureStore).toHaveBeenCalledWith( + expect.anything(), // initial state + undefined, // content provider + expect.anything(), // onTraceFailure + expect.anything(), // customMiddlewares + !isReadOnly + ); + }); + }); +});