Skip to content
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

feat: add build-time execution #856

Closed
wants to merge 17 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
453 changes: 446 additions & 7 deletions package-lock.json

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions packages/myst-cli/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@
},
"dependencies": {
"@jupyterlab/nbformat": "3.5.2",
"@jupyterlab/services": "^7.0.0",
"@reduxjs/toolkit": "^1.7.2",
"adm-zip": "^0.5.10",
"boxen": "^7.1.1",
Expand Down
16 changes: 14 additions & 2 deletions packages/myst-cli/src/process/mdast.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
basicTransformationsPlugin,
htmlPlugin,
footnotesPlugin,
blockMetadataPlugin,
ReferenceState,
MultiPageReferenceState,
resolveReferencesTransform,
Expand Down Expand Up @@ -53,7 +54,6 @@
transformImageFormats,
transformThumbnail,
StaticFileTransformer,
inlineExpressionsPlugin,
propagateBlockDataToCode,
transformBanner,
reduceOutputs,
Expand All @@ -66,6 +66,7 @@
transformImagesToDisk,
transformFilterOutputStreams,
transformLiftCodeBlocksInJupytext,
renderInlineExpressionsTransform,
} from '../transforms/index.js';
import type { ImageExtensions } from '../utils/resolveExtension.js';
import { logMessagesFromVFile } from '../utils/logMessagesFromVFile.js';
Expand All @@ -74,6 +75,13 @@
import { loadIntersphinx } from './intersphinx.js';
import { frontmatterPartsTransform } from '../transforms/parts.js';
import { parseMyst } from './myst.js';
import {
findExistingJupyterServer,

Check warning on line 79 in packages/myst-cli/src/process/mdast.ts

View workflow job for this annotation

GitHub Actions / lint

'findExistingJupyterServer' is defined but never used
JupyterServerSettings,

Check warning on line 80 in packages/myst-cli/src/process/mdast.ts

View workflow job for this annotation

GitHub Actions / lint

'JupyterServerSettings' is defined but never used
launchJupyterServer,

Check warning on line 81 in packages/myst-cli/src/process/mdast.ts

View workflow job for this annotation

GitHub Actions / lint

'launchJupyterServer' is defined but never used
transformKernelExecution,
} from '../transforms/execute.js';
import { ServerConnection, KernelManager, SessionManager } from '@jupyterlab/services';

const LINKS_SELECTOR = 'link,card,linkBlock';

Expand Down Expand Up @@ -174,7 +182,7 @@

const pipe = unified()
.use(reconstructHtmlPlugin) // We need to group and link the HTML first
.use(inlineExpressionsPlugin) // Happens before math and images!
.use(blockMetadataPlugin) // Happens before math and images!
.use(htmlPlugin, { htmlHandlers }) // Some of the HTML plugins need to operate on the transformed html, e.g. figure caption transforms
.use(basicTransformationsPlugin, {
parser: (content: string) => parseMyst(session, content, file),
Expand Down Expand Up @@ -223,8 +231,11 @@
// Combine file-specific citation renderers with project renderers from bib files
const fileCitationRenderer = combineCitationRenderers(cache, ...rendererFiles);

await transformKernelExecution(session, mdast, frontmatter, false, vfile, false);

transformFilterOutputStreams(mdast, vfile, frontmatter.settings);
await transformOutputsToCache(session, mdast, kind, { minifyMaxCharacters });

transformCitations(mdast, fileCitationRenderer, references);
await unified()
.use(codePlugin, { lang: frontmatter?.kernelspec?.language })
Expand Down Expand Up @@ -348,6 +359,7 @@
altOutputFolder: simplifyFigures ? undefined : imageAltOutputFolder,
});
}
renderInlineExpressionsTransform(mdast, vfile);
transformOutputsToFile(session, mdast, imageWriteFolder, {
altOutputFolder: simplifyFigures ? undefined : imageAltOutputFolder,
vfile,
Expand Down
37 changes: 27 additions & 10 deletions packages/myst-cli/src/process/notebook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@
import type { ISession } from '../session/types.js';
import { BASE64_HEADER_SPLIT } from '../transforms/images.js';
import { parseMyst } from './myst.js';
import type { Code } from 'myst-spec-ext';
import type { Code, InlineExpression } from 'myst-spec-ext';
import {

Check failure on line 23 in packages/myst-cli/src/process/notebook.ts

View workflow job for this annotation

GitHub Actions / lint

Import "IUserExpressionMetadata" is only used as types
findExpression,
IUserExpressionMetadata,
metadataSection,
renderExpression,
} from '../transforms/index.js';

function blockParent(cell: ICell, children: GenericNode[]) {
const type = cell.cell_type === CELL_TYPES.code ? NotebookCell.code : NotebookCell.content;
Expand Down Expand Up @@ -86,6 +92,9 @@
end = -1;
}

const vfile = new VFile();
vfile.path = file;

const items = await cells?.slice(0, end).reduce(
async (P, cell: ICell, index) => {
const acc = await P;
Expand All @@ -100,7 +109,20 @@
if (omitBlockDivider) {
return acc.concat(...cellMdast.children);
}
return acc.concat(blockParent(cell, cellMdast.children));
const block = blockParent(cell, cellMdast.children) as GenericNode;

// Embed expression results into expression
const userExpressions = block.data?.[metadataSection] as IUserExpressionMetadata[];
const inlineNodes = selectAll('inlineExpression', block) as InlineExpression[];
let count = 0;
inlineNodes.forEach((inlineExpression) => {
const data = findExpression(userExpressions, inlineExpression.value);
if (!data) return;
count += 1;
inlineExpression.identifier = `eval-${index}-${count}`;
inlineExpression.data = data.result as unknown as Record<string, unknown>;
});
return acc.concat(block);
}
if (cell.cell_type === CELL_TYPES.raw) {
const raw: Code = {
Expand All @@ -118,19 +140,14 @@
value: ensureString(cell.source),
};

const output: { type: 'output'; id: string; data: MinifiedOutput[] } = {
// Embed outputs into output block
const output: { type: 'output'; id: string; data: IOutput[] } = {
agoose77 marked this conversation as resolved.
Show resolved Hide resolved
type: 'output',
id: nanoid(),
data: [],
};

if (cell.outputs && (cell.outputs as IOutput[]).length > 0) {
const minified: MinifiedOutput[] = await minifyCellOutput(
cell.outputs as IOutput[],
cache.$outputs,
{ computeHash, maxCharacters: opts?.minifyMaxCharacters },
agoose77 marked this conversation as resolved.
Show resolved Hide resolved
);
output.data = minified;
output.data = cell.outputs as IOutput[];
}
return acc.concat(blockParent(cell, [code, output]));
}
Expand Down
39 changes: 39 additions & 0 deletions packages/myst-cli/src/session/session.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@
import { rootReducer } from '../store/reducers.js';
import version from '../version.js';
import type { ISession } from './types.js';
import { KernelManager, ServerConnection, SessionManager } from '@jupyterlab/services';
import {

Check failure on line 23 in packages/myst-cli/src/session/session.ts

View workflow job for this annotation

GitHub Actions / lint

Import "JupyterServerSettings" is only used as types
findExistingJupyterServer,
JupyterServerSettings,
launchJupyterServer,
} from '../transforms/execute.js';

const CONFIG_FILES = ['myst.yml'];
const API_URL = 'https://api.mystmd.org';
Expand Down Expand Up @@ -62,6 +68,7 @@

_shownUpgrade = false;
_latestVersion?: string;
_jupyterSessionManager: SessionManager | undefined | null = null;

get log(): Logger {
return this.$logger;
Expand Down Expand Up @@ -157,4 +164,36 @@
});
return warnings;
}

async jupyterSessionManager(): Promise<SessionManager | undefined> {
if (this._jupyterSessionManager !== null) {
return Promise.resolve(this._jupyterSessionManager);
}
try {
const partialServerSettings = await new Promise<JupyterServerSettings>(
async (resolve, reject) => {

Check failure on line 174 in packages/myst-cli/src/session/session.ts

View workflow job for this annotation

GitHub Actions / lint

Promise executor functions should not be async
if (process.env.JUPYTER_BASE_URL === undefined) {
resolve(
findExistingJupyterServer() ||
(await launchJupyterServer(this.contentPath(), this.log)),
);
} else {
resolve({
baseUrl: process.env.JUPYTER_BASE_URL,
token: process.env.JUPYTER_TOKEN,
});
}
},
);
const serverSettings = ServerConnection.makeSettings(partialServerSettings);
const kernelManager = new KernelManager({ serverSettings });
const manager = new SessionManager({ kernelManager, serverSettings });
// TODO: this is a race condition, even though we shouldn't hit if if this promise is actually awaited
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await manager.ready in turn does await the kernelManager.ready if that's what you mean

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

callers of this function would need to know to await (await jupyterSessionManager()).ready unless you await manager.ready in here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this instance it's that there's no logic to avoid creating two managers if one doesn't properly await the promise to session.jupyterSessionManager(). i.e., between the first await and finally setting this._jupyterSessionManager.

this._jupyterSessionManager = manager;
return manager;
} catch {
this._jupyterSessionManager = undefined;
return undefined;
}
}
}
2 changes: 2 additions & 0 deletions packages/myst-cli/src/session/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import type { BuildWarning, RootState } from '../store/index.js';
import type { PreRendererData, RendererData, SingleCitationRenderer } from '../transforms/types.js';
import { SessionManager } from '@jupyterlab/services';

Check failure on line 11 in packages/myst-cli/src/session/types.ts

View workflow job for this annotation

GitHub Actions / lint

All imports in the declaration are only used as types. Use `import type`

export type ISession = {
API_URL: string;
Expand All @@ -24,6 +25,7 @@
plugins: MystPlugin | undefined;
loadPlugins(): Promise<MystPlugin>;
getAllWarnings(ruleId: RuleId): (BuildWarning & { file: string })[];
jupyterSessionManager(): Promise<SessionManager | undefined>;
};

export type ISessionWithCache = ISession & {
Expand Down
Loading
Loading