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

📦 Add myst-execute package #866

Merged
merged 29 commits into from
Feb 6, 2024
Merged

📦 Add myst-execute package #866

merged 29 commits into from
Feb 6, 2024

Conversation

fwkoch
Copy link
Collaborator

@fwkoch fwkoch commented Jan 23, 2024

This just adds a basic skeleton package for myst-execute - a sandbox for the jupyter execution work introduced in #856

@agoose77 - hopefully this helps get you started. Let me know if you run into problems or need a hand with anything...

@rowanc1 - I published to npm, ebp-bot should have an invite to enable CI publishing.

@rowanc1
Copy link
Collaborator

rowanc1 commented Jan 23, 2024

ebp-bot has been added so CI + deployment should work for this!


export type Options = {
cache: ICache<(IExpressionResult | IOutput[])[]>;
sessionFactory: () => Promise<SessionManager | undefined>;
Copy link
Contributor

Choose a reason for hiding this comment

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

This maybe should return SessionManager, but I don't have a feel for MyST's error handling yet, so erring on the side of "report this in the transform".

Copy link
Collaborator

Choose a reason for hiding this comment

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

Any error handling should be done through fileError and fileWarning and passing around a vfile.

Something like:
https://github.com/executablebooks/mystjs/blob/main/packages/myst-cli/src/transforms/code.ts#L159

This will likely also be adding a new RuleId to keep track of these for later processing/ignoring/filtering.

@agoose77 agoose77 mentioned this pull request Jan 24, 2024
4 tasks
Copy link
Collaborator

@rowanc1 rowanc1 left a comment

Choose a reason for hiding this comment

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

Taking another look through!

packages/mystmd-py/setup.cfg Outdated Show resolved Hide resolved
packages/myst-execute/src/execute.ts Outdated Show resolved Hide resolved
packages/myst-execute/src/execute.ts Outdated Show resolved Hide resolved
packages/myst-execute/src/manager.ts Outdated Show resolved Hide resolved
packages/myst-execute/tests/execute.yml Show resolved Hide resolved
packages/myst-execute/tests/run.spec.ts Show resolved Hide resolved
packages/myst-execute/src/cache.ts Show resolved Hide resolved
@rowanc1
Copy link
Collaborator

rowanc1 commented Jan 29, 2024

Looks like there is a merge conflict with the myst-common package and package.json after the release, we might need to rebase and just recommit the package-lock? Let me know if you need help!

@rowanc1
Copy link
Collaborator

rowanc1 commented Feb 1, 2024

Thanks for fixing this up @agoose77 -- do you think it is a state that it is ready to come in?

@agoose77
Copy link
Contributor

agoose77 commented Feb 1, 2024

Just want to tackle a few things RE cleanup first!

@agoose77
Copy link
Contributor

agoose77 commented Feb 1, 2024

@rowanc1 I removed the logging statements that were expected anyway --- this is good to go imo.

Copy link
Collaborator Author

@fwkoch fwkoch left a comment

Choose a reason for hiding this comment

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

Sorry to be slow hitting merge on this - it looks great! Nice to have a clean, separate package we can start pulling into the CLI.

@fwkoch fwkoch merged commit 01322e4 into main Feb 6, 2024
4 checks passed
@fwkoch fwkoch deleted the feat/myst-execute branch February 6, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants