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

🆙 Upgrade Jupyter Books in myst init #1223

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

agoose77
Copy link
Collaborator

@agoose77 agoose77 commented May 22, 2024

With the goal of running Jupyter Book on top of MyST, we need the ability to upgrade a book's configuration and TOC to myst.yml.

This PR adds a Jupyter-Book upgrade path when:

  1. myst init is invoked and
  2. the current directory does not contain a myst.yml and
  3. the current directory contains a _config.yml.

This PR depends upon #1188.

I've introduced zod for this PR, with the intention that we'll slowly phase out simple-validators. There's a challenge with lazy inference and circular references detailed here: colinhacks/zod#3510

  • Do not clobber existing myst.yml
  • Upgrade glossaries
  • Add unit tests

@agoose77 agoose77 self-assigned this May 22, 2024
@stevejpurves
Copy link
Member

Should we really begin phase out simple-validators? there is a significant cost in doing so...
and it's better to not to start a change on that scale unless we intend to follow through. Using zod in isolation for an upgrade command seems ok though, and a chance to see how it might be applied.

@agoose77
Copy link
Collaborator Author

@stevejpurves I agree. My suggestion is that we evaluate zod here, and look to make better use of it if it transpires that it's a good fit internally.

@choldgraf
Copy link
Member

@agoose77 if the goal is to use this to learn about zod and whether it's a good idea for future adoption, I'd recommend:

  • Opening an issue like 'evaluate the use of zod in the myst CLI' and link to the module where we've trialed it here
  • Add the questions that you'd want to answer to decide whether it should be adopted more broadly
  • Define a date by which you want to revisit the issue to make a decision
  • Add a comment in the codebase that references that issue and notes the date to look at it

That way you can structure the information gathering rather than have it as a one-off tool that might confuse us in N months when we've forgotten about this conversation

@choldgraf
Copy link
Member

One more recommendation here (or in a follow up pr if it's out of scope)

If we detect a config.yml file, I assume we want to note the upgrade command. But for a while, I think we should also note that people can run the v1 version of jupyter book by installing the version ==1. Alternatively what do you think about making a pypi package like jupyter-book-sphinx and pinning that to the 1.x series (similar to nbclassic)

@agoose77
Copy link
Collaborator Author

agoose77 commented Jun 11, 2024

I tried this on CMIP6:

  • toc was placed in the root indentation
  • toc missed the README.
  • We should be smarter about parsing author lists, e.g. things split with commas.

@agoose77
Copy link
Collaborator Author

@fwkoch your thoughts on this would be welcome. I've decided that instead of a myst upgrade command, this should just be myst init when we detect a Jupyter Book. This has some nice ergonomics, mainly

  1. myst/myst --init directly upgrades un-ported Jupyter Books (nice short UX)
  2. Repeated myst/myst --init does not, because myst.yml exists
  3. myst upgrade is reserved for actual MyST-specific scoped upgrades

I tested the WIP on The Turing Way, and it seems to handle it fairly well.

@agoose77 agoose77 marked this pull request as ready for review June 19, 2024 21:14
@agoose77
Copy link
Collaborator Author

The line between "upgrading" a JB and upgrading MyST syntax is a little blurry. My take is that the JB upgrade is special, and we should treat it as such -- users will have an "old" project with known legacy aspects, vs users with one year old MyST projects that use some slightly deprecated syntax.

@agoose77 agoose77 changed the title 🆙 Add new upgrade command for Jupyter Books 🆙 Upgrade Jupyter Books in myst init Jun 19, 2024
@agoose77 agoose77 requested a review from rowanc1 June 19, 2024 21:29
@agoose77 agoose77 requested review from fwkoch and rowanc1 and removed request for rowanc1 June 19, 2024 21:29
@agoose77
Copy link
Collaborator Author

I need some help setting up unit tests for this (I am hoping someone else will make my life easier...)!

@rowanc1 rowanc1 mentioned this pull request Jun 19, 2024
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