From a543e5194a48ee4a8f9922698587badd4f05288e Mon Sep 17 00:00:00 2001 From: Paul Fitzpatrick Date: Wed, 15 Sep 2021 19:35:21 -0400 Subject: [PATCH] (core) add a python3 button Summary: This adds a dropdown to the document settings model in staging/dev to set the python engine to Python2 or Python3. The setting is saved in `_grist_DocInfo.documentSettings.engine`. Test Plan: tested manually for now - separate diff needed to add runsc to jenkins setup and make this testable Reviewers: dsagal, alexmojaki Reviewed By: alexmojaki Differential Revision: https://phab.getgrist.com/D3014 --- app/client/ui/DocumentSettings.ts | 52 ++++++++++++---- app/common/AsyncCreate.ts | 2 +- app/common/DocumentSettings.ts | 7 +++ app/common/gristUrls.ts | 5 ++ app/server/lib/ActiveDoc.ts | 98 ++++++++++++++++++++++--------- app/server/lib/Sharing.ts | 10 ++++ app/server/lib/sendAppPage.ts | 2 + app/server/lib/serverUtils.ts | 18 ++++++ 8 files changed, 154 insertions(+), 40 deletions(-) diff --git a/app/client/ui/DocumentSettings.ts b/app/client/ui/DocumentSettings.ts index 6f8fc37f..a38371d2 100644 --- a/app/client/ui/DocumentSettings.ts +++ b/app/client/ui/DocumentSettings.ts @@ -12,11 +12,14 @@ import {loadMomentTimezone} from 'app/client/lib/imports'; import {DocInfoRec} from 'app/client/models/DocModel'; import {DocPageModel} from 'app/client/models/DocPageModel'; import {testId, vars} from 'app/client/ui2018/cssVars'; +import {select} from 'app/client/ui2018/menus'; import {saveModal} from 'app/client/ui2018/modals'; import {buildTZAutocomplete} from 'app/client/widgets/TZAutocomplete'; +import {EngineCode} from 'app/common/DocumentSettings'; +import {GristLoadConfig} from 'app/common/gristUrls'; import {locales} from "app/common/Locales"; import {buildCurrencyPicker} from 'app/client/widgets/CurrencyPicker'; -import * as LocaleCurrency from "locale-currency"; +import * as LocaleCurrency from 'locale-currency'; /** * Builds a simple saveModal for saving settings. @@ -26,9 +29,14 @@ export async function showDocSettingsModal(docInfo: DocInfoRec, docPageModel: Do return saveModal((ctl, owner) => { const timezoneObs = Observable.create(owner, docInfo.timezone.peek()); - const {locale, currency} = docInfo.documentSettingsJson.peek(); + const docSettings = docInfo.documentSettingsJson.peek(); + const {locale, currency, engine} = docSettings; const localeObs = Observable.create(owner, locale); const currencyObs = Observable.create(owner, currency); + const engineObs = Observable.create(owner, engine); + + // Check if server supports engine choices - if so, we will allow user to pick. + const canChangeEngine = getSupportedEngineChoices().length > 0; return { title: 'Document Settings', @@ -44,23 +52,37 @@ export async function showDocSettingsModal(docInfo: DocInfoRec, docPageModel: Do dom.create(buildCurrencyPicker, currencyObs, (val) => currencyObs.set(val), {defaultCurrencyLabel: `Local currency (${LocaleCurrency.getCurrency(l)})`}) )), + canChangeEngine ? [ + cssDataRow('Engine:'), + select(engineObs, getSupportedEngineChoices()), + ] : null, ], - saveFunc: () => docInfo.updateColValues({ - timezone: timezoneObs.get(), - documentSettings: JSON.stringify({ - ...docInfo.documentSettingsJson.peek(), - locale: localeObs.get(), - currency: currencyObs.get() - }) - }), + // Modal label is "Save", unless engine is changed. If engine is changed, the document will + // need a reload to switch engines, so we replace the label with "Save and Reload". + saveLabel: dom.text((use) => (use(engineObs) === docSettings.engine) ? 'Save' : 'Save and Reload'), + saveFunc: async () => { + await docInfo.updateColValues({ + timezone: timezoneObs.get(), + documentSettings: JSON.stringify({ + ...docInfo.documentSettingsJson.peek(), + locale: localeObs.get(), + currency: currencyObs.get(), + engine: engineObs.get() + }) + }); + // Reload the document if the engine is changed. + if (engineObs.get() !== docSettings.engine) { + await docPageModel.appModel.api.getDocAPI(docPageModel.currentDocId.get()!).forceReload(); + } + }, // If timezone, locale, or currency hasn't changed, disable the Save button. saveDisabled: Computed.create(owner, (use) => { - const docSettings = docInfo.documentSettingsJson.peek(); return ( use(timezoneObs) === docInfo.timezone.peek() && use(localeObs) === docSettings.locale && - use(currencyObs) === docSettings.currency + use(currencyObs) === docSettings.currency && + use(engineObs) === docSettings.engine ); }) }; @@ -105,3 +127,9 @@ const cssDataRow = styled('div', ` margin: 16px 0px; font-size: ${vars.largeFontSize}; `); + +// Check which engines can be selected in the UI, if any. +export function getSupportedEngineChoices(): EngineCode[] { + const gristConfig: GristLoadConfig = (window as any).gristConfig || {}; + return gristConfig.supportEngines || []; +} diff --git a/app/common/AsyncCreate.ts b/app/common/AsyncCreate.ts index 4405626c..18e73cff 100644 --- a/app/common/AsyncCreate.ts +++ b/app/common/AsyncCreate.ts @@ -4,7 +4,7 @@ * On failure, the result is cleared, so that subsequent calls attempt the creation again. * * Usage: - * this._obj = AsyncCreate(asyncCreateFunc); + * this._obj = new AsyncCreate(asyncCreateFunc); * obj = await this._obj.get(); // calls asyncCreateFunc * obj = await this._obj.get(); // uses cached object if asyncCreateFunc succeeded, else calls it again. * diff --git a/app/common/DocumentSettings.ts b/app/common/DocumentSettings.ts index 3a50fce2..54239cd6 100644 --- a/app/common/DocumentSettings.ts +++ b/app/common/DocumentSettings.ts @@ -1,4 +1,11 @@ export interface DocumentSettings { locale: string; currency?: string; + engine?: EngineCode; } + +/** + * The back-end will for now support at most two engines, a pynbox-backed python2 and + * a gvisor-backed python3. + */ +export type EngineCode = 'python2' | 'python3'; diff --git a/app/common/gristUrls.ts b/app/common/gristUrls.ts index f0b5f90b..3ed9ace7 100644 --- a/app/common/gristUrls.ts +++ b/app/common/gristUrls.ts @@ -1,5 +1,6 @@ import {BillingPage, BillingSubPage, BillingTask} from 'app/common/BillingAPI'; import {OpenDocMode} from 'app/common/DocListAPI'; +import {EngineCode} from 'app/common/DocumentSettings'; import {encodeQueryParams, isAffirmative} from 'app/common/gutil'; import {localhostRegex} from 'app/common/LoginState'; import {LocalPlugin} from 'app/common/plugin'; @@ -426,6 +427,10 @@ export interface GristLoadConfig { // If set, enable anonymous sharing UI elements. supportAnon?: boolean; + // If set, allow selection of the specified engines. + // TODO: move this list to a separate endpoint. + supportEngines?: EngineCode[]; + // Max upload allowed for imports (except .grist files), in bytes; 0 or omitted for unlimited. maxUploadSizeImport?: number; diff --git a/app/server/lib/ActiveDoc.ts b/app/server/lib/ActiveDoc.ts index dd409f2d..2f063980 100644 --- a/app/server/lib/ActiveDoc.ts +++ b/app/server/lib/ActiveDoc.ts @@ -26,7 +26,7 @@ import { ServerQuery } from 'app/common/ActiveDocAPI'; import {ApiError} from 'app/common/ApiError'; -import {mapGetOrSet, MapWithTTL} from 'app/common/AsyncCreate'; +import {AsyncCreate, mapGetOrSet, MapWithTTL} from 'app/common/AsyncCreate'; import { BulkColValues, CellValue, @@ -38,8 +38,9 @@ import { } from 'app/common/DocActions'; import {DocData} from 'app/common/DocData'; import {DocSnapshots} from 'app/common/DocSnapshot'; +import {DocumentSettings} from 'app/common/DocumentSettings'; import {FormulaProperties, getFormulaProperties} from 'app/common/GranularAccessClause'; -import {byteString, countIf} from 'app/common/gutil'; +import {byteString, countIf, safeJsonParse} from 'app/common/gutil'; import {InactivityTimer} from 'app/common/InactivityTimer'; import * as marshal from 'app/common/marshal'; import {UploadResult} from 'app/common/uploads'; @@ -73,7 +74,7 @@ import {DocStorage} from './DocStorage'; import {expandQuery} from './ExpandedQuery'; import {GranularAccess, GranularAccessForBundle} from './GranularAccess'; import {OnDemandActions} from './OnDemandActions'; -import {getLogMetaFromDocSession} from './serverUtils'; +import { getLogMetaFromDocSession, supportsEngineChoices} from './serverUtils'; import {findOrAddAllEnvelope, Sharing} from './Sharing'; import cloneDeep = require('lodash/cloneDeep'); import flatten = require('lodash/flatten'); @@ -129,7 +130,7 @@ export class ActiveDoc extends EventEmitter { // result). protected _modificationLock: Mutex = new Mutex(); - private readonly _dataEngine: ISandbox; + private _dataEngine: AsyncCreate|undefined; private _activeDocImport: ActiveDocImport; private _onDemandActions: OnDemandActions; private _granularAccess: GranularAccess; @@ -146,13 +147,14 @@ export class ActiveDoc extends EventEmitter { // Timer for shutting down the ActiveDoc a bit after all clients are gone. private _inactivityTimer = new InactivityTimer(() => this.shutdown(), Deps.ACTIVEDOC_TIMEOUT * 1000); private _recoveryMode: boolean = false; + private _shuttingDown: boolean = false; - constructor(docManager: DocManager, docName: string, options?: { + constructor(docManager: DocManager, docName: string, private _options?: { safeMode?: boolean, docUrl?: string }) { super(); - if (options?.safeMode) { this._recoveryMode = true; } + if (_options?.safeMode) { this._recoveryMode = true; } this._docManager = docManager; this._docName = docName; this.docStorage = new DocStorage(docManager.storageManager, docName); @@ -165,22 +167,15 @@ export class ActiveDoc extends EventEmitter { // user-defined python code including formula calculations. It maintains all document data and // metadata, and applies translates higher-level UserActions into lower-level DocActions. - // HACK: If doc title as a slug contains "activate-python3-magic", and we are - // in an environment with GRIST_EXPERIMENTAL_PLUGINS=1 (dev or staging but not - // prod), use Python3. This is just for experimentation at this point. - // TODO: use a less hacky way to say we want to use py3 in a document. - const preferredPythonVersion = - (options?.docUrl?.match(/activate-python3-magic/) && process.env.GRIST_EXPERIMENTAL_PLUGINS === '1') - ? '3' : undefined; - - this._dataEngine = this._docManager.gristServer.create.NSandbox({ - comment: docName, - logCalls: false, - logTimes: true, - logMeta: {docId: docName}, - docUrl: options?.docUrl, - preferredPythonVersion, - }); + // Creation of the data engine currently needs to be deferred if we support a choice of + // engines, since we need to look at the document to see what kind of engine it needs. + // If we don't offer a choice, go ahead and start creating the data engine now, so it + // is created in parallel to fetching the document from external storage (if needed). + // TODO: cache engine requirement for doc in home db so we can retain this parallelism + // when offering a choice of data engines. + if (!supportsEngineChoices()) { + this._getEngine().catch(e => this.logError({client: null}, `engine for ${docName} failed to launch: ${e}`)); + } this._activeDocImport = new ActiveDocImport(this); @@ -194,6 +189,8 @@ export class ActiveDoc extends EventEmitter { public get recoveryMode(): boolean { return this._recoveryMode; } + public get isShuttingDown(): boolean { return this._shuttingDown; } + public async getUserOverride(docSession: OptDocSession) { return this._granularAccess.getUserOverride(docSession); } @@ -312,10 +309,12 @@ export class ActiveDoc extends EventEmitter { } try { + const dataEngine = this._dataEngine ? await this._getEngine() : null; + this._shuttingDown = true; // Block creation of engine if not yet in existence. await Promise.all([ this.docStorage.shutdown(), this.docPluginManager.shutdown(), - this._dataEngine.shutdown() + dataEngine?.shutdown() ]); // The this.waitForInitialization promise may not yet have resolved, but // should do so quickly now we've killed everything it depends on. @@ -1362,7 +1361,9 @@ export class ActiveDoc extends EventEmitter { this.logDebug(docSession, `loaded in ${loadMs} ms, InactivityTimer set to ${closeTimeout} ms`); return true; } catch (err) { - this.logWarn(docSession, "_finishInitialization stopped with %s", err); + if (!this._shuttingDown) { + this.logWarn(docSession, "_finishInitialization stopped with %s", err); + } this._fullyLoaded = true; return false; } @@ -1392,7 +1393,10 @@ export class ActiveDoc extends EventEmitter { const now = Date.now(); if (now >= this._lastMemoryMeasurement + MEMORY_MEASUREMENT_INTERVAL_MS) { this._lastMemoryMeasurement = now; - await this._dataEngine.reportMemoryUsage(); + if (this._dataEngine && !this._shuttingDown) { + const dataEngine = await this._getEngine(); + await dataEngine.reportMemoryUsage(); + } } } @@ -1427,8 +1431,9 @@ export class ActiveDoc extends EventEmitter { * Call a method in the sandbox, without checking the _modificationLock. Calls to * the sandbox are naturally serialized. */ - private _rawPyCall(funcName: string, ...varArgs: unknown[]): Promise { - return this._dataEngine.pyCall(funcName, ...varArgs); + private async _rawPyCall(funcName: string, ...varArgs: unknown[]): Promise { + const dataEngine = await this._getEngine(); + return dataEngine.pyCall(funcName, ...varArgs); } /** @@ -1439,6 +1444,45 @@ export class ActiveDoc extends EventEmitter { private _pyCall(funcName: string, ...varArgs: unknown[]): Promise { return this._modificationLock.runExclusive(() => this._rawPyCall(funcName, ...varArgs)); } + + private async _getEngine(): Promise { + if (this._shuttingDown) { throw new Error('shutting down, data engine unavailable'); } + this._dataEngine = this._dataEngine || new AsyncCreate(async () => { + + // Figure out what kind of engine we need for this document. + let preferredPythonVersion: '2' | '3' = '2'; + + // Currently only respect engine preference on experimental deployments (staging/dev). + if (process.env.GRIST_EXPERIMENTAL_PLUGINS === '1') { + // Careful, migrations may not have run on this document and it may not have a + // documentSettings column. Failures are treated as lack of an engine preference. + const docInfo = await this.docStorage.get('SELECT documentSettings FROM _grist_DocInfo').catch(e => undefined); + const docSettingsString = docInfo?.documentSettings; + if (docSettingsString) { + const docSettings: DocumentSettings|undefined = safeJsonParse(docSettingsString, undefined); + const engine = docSettings?.engine; + if (engine) { + if (engine === 'python2') { + preferredPythonVersion = '2'; + } else if (engine === 'python3') { + preferredPythonVersion = '3'; + } else { + throw new Error(`engine type not recognized: ${engine}`); + } + } + } + } + return this._docManager.gristServer.create.NSandbox({ + comment: this._docName, + logCalls: false, + logTimes: true, + logMeta: {docId: this._docName}, + docUrl: this._options?.docUrl, + preferredPythonVersion, + }); + }); + return this._dataEngine.get(); + } } // Helper to initialize a sandbox action bundle with no values. diff --git a/app/server/lib/Sharing.ts b/app/server/lib/Sharing.ts index 19d2feef..d92b3f7b 100644 --- a/app/server/lib/Sharing.ts +++ b/app/server/lib/Sharing.ts @@ -246,6 +246,16 @@ export class Sharing { // when we come to it. For now we only log skipped envelopes as "alien" in _logActionBundle(). const ownActionBundle: LocalActionBundle = this._filterOwnActions(localActionBundle); + // If the document has shut down in the meantime, and this was just a "Calculate" action, + // return a trivial result. This is just to reduce noisy warnings in migration tests. + if (this._activeDoc.isShuttingDown && isCalculate) { + return { + actionNum: localActionBundle.actionNum, + retValues: [], + isModification: false + }; + } + // Apply the action to the database, and record in the action log. if (!trivial) { await this._activeDoc.docStorage.execTransaction(async () => { diff --git a/app/server/lib/sendAppPage.ts b/app/server/lib/sendAppPage.ts index dd87ff5e..6c9ebf65 100644 --- a/app/server/lib/sendAppPage.ts +++ b/app/server/lib/sendAppPage.ts @@ -2,6 +2,7 @@ import {GristLoadConfig} from 'app/common/gristUrls'; import {isAnonymousUser} from 'app/server/lib/Authorizer'; import {RequestWithOrg} from 'app/server/lib/extractOrg'; import {GristServer} from 'app/server/lib/GristServer'; +import {getSupportedEngineChoices} from 'app/server/lib/serverUtils'; import * as express from 'express'; import * as fse from 'fs-extra'; import * as path from 'path'; @@ -35,6 +36,7 @@ export function makeGristConfig(homeUrl: string|null, extra: Partial