-
Notifications
You must be signed in to change notification settings - Fork 104
Add notebooks to docs download #652
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
base: main
Are you sure you want to change the base?
Conversation
Preview the changes: https://turinglang.org/docs/pr-previews/652 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds notebook generation and download functionality to the documentation site by creating Jupyter notebooks from Quarto .qmd files and providing download links in the rendered HTML pages.
- Introduces a Python script to convert .qmd files to .ipynb format with proper cell structure
- Adds shell scripts to generate notebooks and inject download links into HTML pages
- Integrates notebook generation into both publish and preview GitHub workflows
Reviewed Changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
assets/scripts/qmd_to_ipynb.py | Python converter that parses .qmd files and creates Jupyter notebooks with appropriate cell types |
assets/scripts/generate_notebooks.sh | Shell script that finds .qmd files and converts them to notebooks in the _site directory |
assets/scripts/add_notebook_links.sh | Shell script that adds download links to HTML pages using Perl regex replacement |
.github/workflows/publish.yml | Adds notebook generation and link injection steps to the publish workflow |
.github/workflows/preview.yml | Adds notebook generation and link injection steps to the preview workflow |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
assets/scripts/qmd_to_ipynb.py
Outdated
"display_name": "Julia 1.11", | ||
"language": "julia", | ||
"name": self.kernel_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded display name 'Julia 1.11' doesn't match the dynamic kernel name when using Python. Consider making the display name dynamic based on the kernel.
Copilot uses AI. Check for mistakes.
"language_info": { | ||
"file_extension": ".jl", | ||
"mimetype": "application/julia", | ||
"name": "julia", | ||
"version": "1.11.0" | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The language_info is hardcoded for Julia even when the kernel is Python. This should be conditional based on the detected engine/kernel type.
Copilot uses AI. Check for mistakes.
echo "Generating Jupyter notebooks from .qmd files..." | ||
|
||
# Find all .qmd files in tutorials, usage, and developers directories | ||
find tutorials usage developers -name "index.qmd" | while read qmd_file; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using 'while read' with 'find' can fail with filenames containing spaces or special characters. Use 'find ... -print0 | while IFS= read -r -d "" qmd_file; do' for safer file handling.
find tutorials usage developers -name "index.qmd" | while read qmd_file; do | |
find tutorials usage developers -name "index.qmd" -print0 | while IFS= read -r -d '' qmd_file; do |
Copilot uses AI. Check for mistakes.
![]() This PR adds a download button to get jupyter notebooks from the docs. Some notes:
@shravanngoswamii if you'd have some time to check these out, I'd appreciate it. |
I should also mention, I had a few issues with the CI versions of bash/posix vs deploying on my mac, so some of the scripting is done to appease github actions moreso than how users may run locally. |
I think this partially satisfies some of the points from #641 ~ it might be worth adding some note about these being available in the docs / have a quick way to export into online systems, as discussed in that issue. |
assets/scripts/qmd_to_ipynb.py
Outdated
def __init__(self, qmd_path: str): | ||
self.qmd_path = Path(qmd_path) | ||
self.cells: List[Dict[str, Any]] = [] | ||
self.kernel_name = "julia-1.11" # Default kernel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generated notebooks don't work in Google Colab, or am I doing something wrong? I think the defined kernel might be the issue since Colab only supports the lts
version of Julia by default. Also, we'll need to change this hardcoded version every time we update the docs to a new version. Maybe we should add a note in the README or change it to use the version from an env file or some variable that we can use everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think latest push should fix this. Lemme verify and confirm once it builds
Notebooks work fine with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall thoughts:
-
Looks very nice! I like the little icon and it fits in thematically with the rest of the website very nicely.
-
When you download the notebook, the first line is
using Pkg; Pkg.instantiate()
. I attempted to run this in a VSCode and this loads the global Julia environment. So if you subsequently runusing Turing
that will also add Turing to the global environment, which is probably not ideal for users.
Is it possible to modify the notebook generation such that the very top of the notebook contains a new code block saying using Pkg; Pkg.activate(; temp=true)
, before the call to instantiate()?
Yes! I'll need to modify the new file added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My main other question is: if the qmd-to-ipynb script is already in Python, why not write the Bash scripts in Python as well?
I don't mind the use of Python (it's way better for quick scripts than Julia is), but I think reducing the number of moving parts that need to work together will make for easier maintenance in the future. Especially because all of these scripts serve a common purpose and you're unlikely to want to run one without running the others.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, personally I find any bash stuff gets harder to read the more complex it gets, I can do that though, not a problem. Let me do the current changes and verify it works then I can explore this fully.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh yes bash is definitely hard to read but I meant moving the bash to python - not the other way round!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aha, gotchu. tbh, I originally wrote the bash as a quick fix with the CI, it was only because the default convert didn't work that I did the python too.
assets/scripts/add_notebook_links.sh
Outdated
if ! grep -q 'Download notebook' "$html_file"; then | ||
# Insert the notebook link AFTER the "Report an issue" link | ||
# This ensures it goes in the right place in the sidebar toc-actions | ||
# The download="index.ipynb" attribute forces browser to download instead of navigate | ||
perl -i -pe 's/(<a href="[^"]*issues\/new"[^>]*><i class="bi[^"]*"><\/i>Report an issue<\/a><\/li>)/$1<li><a href="index.ipynb" class="toc-action" download="index.ipynb"><i class="bi bi-journal-code"><\/i>Download notebook<\/a><\/li>/g' "$html_file" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you make the string 'Download notebook'
a variable?
Overall very excited about this, very happy that I clicked the download link and it just worked (tm) :) |
Looks good -- it is very helpful to add an "Open in Colab" button too. URLs like will automatically open a notebook from the given Github repo. |
This was to try and fix #642 as I was afraid of a caching issue, please see that PR for previous discussions