-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
build examples readmes with quarto #3046
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
📝 WalkthroughWalkthroughThe changes introduce automated documentation generation for usage examples. A new script processes a YAML allowlist to convert example README files into Quarto markdown documents, handling assets and generating an index. Project configuration is updated to run this script pre-render and to add a new sidebar section for usage examples. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
📖 Documentation Preview: https://689644a3fd6b0383be07d346--resonant-treacle-0fd729.netlify.app Deployed on Netlify from commit 6b60e0a |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/scripts/examples-allowlist.yml (1)
1-5: Keep allow-list alphabetically sorted for simpler diffsMinor, but ordering the entries (
distributed-parallel,gpt-oss,llama-3,slurm) alphabetically avoids churn in future PRs and makes merge-conflicts less likely.docs/scripts/generate_examples_docs.py (1)
82-85: Avoid double H1 by stripping it before writing.qmdBecause the title is also injected into front-matter, leaving the original
# Headingproduces two identical H1s on the page.- md = readme.read_text(encoding="utf-8") + md = readme.read_text(encoding="utf-8") + # drop first H1 so front-matter title is the only top-level heading + md = "\n".join(line for line in md.splitlines() if not line.startswith("# ") or line.count("#") > 1)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
_quarto.yml(2 hunks)docs/scripts/examples-allowlist.yml(1 hunks)docs/scripts/generate_examples_docs.py(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: SalmanMohammadi
PR: axolotl-ai-cloud/axolotl#2994
File: index.qmd:7-8
Timestamp: 2025-07-31T11:48:46.313Z
Learning: In documentation workflow testing PRs, temporary test content like "test test" may be intentionally added to trigger documentation preview actions and validate workflow fixes, rather than being accidental debug text.
📚 Learning: 2025-07-31T11:48:46.313Z
Learnt from: SalmanMohammadi
PR: axolotl-ai-cloud/axolotl#2994
File: index.qmd:7-8
Timestamp: 2025-07-31T11:48:46.313Z
Learning: In documentation workflow testing PRs, temporary test content like "test test" may be intentionally added to trigger documentation preview actions and validate workflow fixes, rather than being accidental debug text.
Applied to files:
_quarto.yml
🔇 Additional comments (2)
_quarto.yml (1)
3-5: Confirm Quarto accepts list syntax inpre-renderSwitching from a scalar to a sequence is correct, but Quarto only picks up scripts that are executable in the build environment. Double-check that
docs/scripts/generate_examples_docs.pyhas the executable bit set (or is invoked throughpython).docs/scripts/generate_examples_docs.py (1)
83-85: Quote style in front-matter is YAML, not Python repr
!rmay emit single quotes with escapes that YAML doesn’t expect (e.g., backslashes). Use plain strings:- fm = f"---\ntitle: {title!r}\nformat:\n html:\n toc: true\n---\n\n" + fm = f"---\ntitle: \"{title}\"\nformat:\n html:\n toc: true\n---\n\n"
| - title: Usage Examples | ||
| desc: Example YAML files for training different models | ||
| contents: | ||
| - docs/examples/*.qmd | ||
|
|
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.
🛠️ Refactor suggestion
Use section: instead of title: for sidebar groups
In the sidebar config every other group uses section:. Quarto’s schema doesn’t recognise title: at this level, so the whole “Usage Examples” block may be ignored.
- - title: Usage Examples
+ - section: "Usage Examples"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - title: Usage Examples | |
| desc: Example YAML files for training different models | |
| contents: | |
| - docs/examples/*.qmd | |
| - section: "Usage Examples" | |
| desc: Example YAML files for training different models | |
| contents: | |
| - docs/examples/*.qmd |
🤖 Prompt for AI Agents
In _quarto.yml around lines 291 to 295, replace the key `title:` with `section:`
for the sidebar group labeled "Usage Examples" because Quarto's schema requires
`section:` at this level for sidebar groups to be recognized. Update the YAML to
use `section:` instead of `title:` to ensure the block is properly included in
the sidebar.
| if not src_path.exists(): | ||
| return m.group(0) # leave as-is if not found | ||
| rel = src_path.relative_to(src_dir) | ||
| dest_path = dest_assets / rel |
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.
🛠️ Refactor suggestion
Handle assets that live outside the example directory
src_path.relative_to(src_dir) raises ValueError if the README references an asset like ../shared/logo.png. That aborts the whole run.
- rel = src_path.relative_to(src_dir)
+ try:
+ rel = src_path.relative_to(src_dir)
+ except ValueError:
+ # fallback: flatten to basename to keep the run alive
+ rel = src_path.name📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not src_path.exists(): | |
| return m.group(0) # leave as-is if not found | |
| rel = src_path.relative_to(src_dir) | |
| dest_path = dest_assets / rel | |
| if not src_path.exists(): | |
| return m.group(0) # leave as-is if not found | |
| try: | |
| rel = src_path.relative_to(src_dir) | |
| except ValueError: | |
| # fallback: flatten to basename to keep the run alive | |
| rel = src_path.name | |
| dest_path = dest_assets / rel |
🤖 Prompt for AI Agents
In docs/scripts/generate_examples_docs.py around lines 70 to 73, the code uses
src_path.relative_to(src_dir) which raises a ValueError if the asset path is
outside the example directory. To fix this, add a try-except block around the
relative_to call to catch ValueError and handle such cases gracefully, for
example by skipping those assets or processing them differently to avoid
aborting the entire run.
| ROOT = THIS.parents[2] # repo root (docs/scripts -> docs -> ROOT) | ||
| EXAMPLES_DIR = ROOT / "examples" | ||
| OUTPUT_DIR = ROOT / "docs" / "examples" | ||
| ALLOWLIST_YML = THIS.parent / "examples-allowlist.yml" |
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.
Is an allowlist necessary? I think anything matching README.md within the examples dir seem to be a good check?
We just need to make sure the resulting qmd has execute: false
WIP
Summary by CodeRabbit
New Features
Chores