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

🪐 Allow for Jupyter execution (including inline) directly in mystmd CLI #873

Merged
merged 39 commits into from
Feb 12, 2024

Conversation

agoose77
Copy link
Contributor

@agoose77 agoose77 commented Jan 24, 2024

This PR adds execution support to the mystmd CLI. A sketch at some docs below from @rowanc1!

  • add health testing to Jupyter Server selection logic
  • prewarm kernels?

@agoose77 agoose77 mentioned this pull request Jan 24, 2024
4 tasks
@agoose77 agoose77 force-pushed the agoose77/feat-use-execute-transform branch from 2199916 to 4012be5 Compare January 29, 2024 20:05
@agoose77 agoose77 requested a review from rowanc1 January 29, 2024 20:05
@rowanc1

This comment was marked as resolved.

@rowanc1
Copy link
Collaborator

rowanc1 commented Jan 30, 2024

Figured out my problem, there was a dead server hanging about that could not be removed. I went into the runtime using jupyter --paths and deleted it manually.

As part of that testing I went through and changed a few things:

  • improved some of the logging
  • For errors: I made things that were happening (jupyter starting) be in log and things that were node/execution errors part of the fileError. The logs trigger immediately, while the ones in the vfile don't get logged for a while and are stored. I think this is a decent pattern, and a bit of a nuance on the advice I gave earlier.
  • You now need to pass the --execute flag from the CLI, the default is false.
  • You can clean the execution cache using myst clean --execute

Things I noticed:

  • The jupyter server stays around and isn't shut down when running myst build --execute. Or myst build --execute --html this means that the process doesn't stop and just hangs. Need to investigate how to kill the server.
  • I put the execute flag in, but this should maybe also be in the myst.yml?
  • I don't think we are passing the minification flags around anymore? This needs to come back.

@agoose77
Copy link
Contributor Author

agoose77 commented Feb 2, 2024

There's something wacky going on with Node - it refuses to quit even when the child process has been killed. There are tools to ask node why it's not quitting, but I don't have time right now to dig into it. It might be that we want to remove the use of closures and move this logic into a class. That might be less opaque to node, if refcycles are the problem.

Base automatically changed from feat/myst-execute to main February 6, 2024 07:23
@agoose77 agoose77 force-pushed the agoose77/feat-use-execute-transform branch from dafd4ef to 84290d9 Compare February 6, 2024 21:08
docs/execute-notebooks.md Outdated Show resolved Hide resolved
@agoose77
Copy link
Contributor Author

agoose77 commented Feb 9, 2024

@fwkoch I changed the Python package from using setuptools to using the Hatch build backend. The benefits of this are fewer config files, more declarative config, and the ability to use plugins to pull metadata from mystmd's package.json. Are you comfortable with this change?

I can split it into a new PR if we need.

"version": "changeset version && npm install && cd packages/mystmd-py && bash scripts/bump-version.sh",
"version": "changeset version && npm install",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh cool, this is nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear here, the version is only updated at pypi deployment time (because it's taken from package.json, which is copied in the deploy bash scipt).

Comment on lines -286 to +290
...pages.map((page) =>
loadFile(session, page.file, siteProject.path, undefined, { minifyMaxCharacters }),
),
...pages.map((page) => loadFile(session, page.file, siteProject.path, undefined)),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am still a bit concerned that we are no longer passing this option in at all? I think this actually might be breaking JATS - @fwkoch I would love another set of eyes here.

@agoose77 agoose77 force-pushed the agoose77/feat-use-execute-transform branch from 049ecc0 to 16314d4 Compare February 9, 2024 20:54
@agoose77
Copy link
Contributor Author

@rowanc1 I've updated the cache key struct to include the raises-exception tag that allows notebooks to ignore errors. I haven't included a cache-version in the key; my rationale is that this is implicitly part of the hash - if we change the structure, the hash will change.

Copy link
Collaborator

@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.

Ok - putting my approval on this. PR looks great! Lots of tedium passing the execute flag around - maybe those CLI options could be a refactor target some time.

Regarding the minifyMaxCharacter stuff - I played around with static outputs and I think it's all fine...? During mdast processing we call transformOutputsToCache - this does the exact same minification we were also doing during notebook loading. So now (since minification was removed from loadFile) it just always happens in that transform instead of both places. I still have some vague worries that there was an edge case causing us to do this in two places, but no test coverage and I couldn't recreate it 🤷‍♀️ Good to land, and we can just keep an eye on it.

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.

Thanks @fwkoch for your review -- and @agoose77 for going back and forth on this. I am hugely excited to get this out and start testing it in the wild. I have been using it for the last several days and it is working well!

🚀

@rowanc1 rowanc1 merged commit 08a70ff into main Feb 12, 2024
4 checks passed
@rowanc1 rowanc1 deleted the agoose77/feat-use-execute-transform branch February 12, 2024 21:42
@rowanc1
Copy link
Collaborator

rowanc1 commented Feb 12, 2024

Ah -- we didn't add changesets @agoose77! :)

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.

None yet

3 participants