-
Notifications
You must be signed in to change notification settings - Fork 9
Feature/faster ci notebooks #561
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
Conversation
- Only runs fast notebooks (~1 min total) - Slow notebooks are skipped Deploy job (releases only): - Runs fast notebooks first - Then runs slow notebooks (~10 min extra) - Full documentation with all notebooks executed Files changed: - .github/workflows/docs.yaml - updated both execution steps - docs/notebooks/slow_notebooks.txt - created with the 2 slow notebooks To add/remove slow notebooks in the future, just edit slow_notebooks.txt.
…clustering-storage-modes (5 min) 2. Created slow_notebooks.txt - config file to exclude them from regular CI 3. Updated CI - fast notebooks run on every build, slow ones only on release 4. Switched to Plotly CDN - notebooks now ~98% smaller (4.8MB → 79KB each)
📝 WalkthroughWalkthroughThe docs CI workflow now separates notebooks into fast (parallel) and slow (sequential, release-only) execution paths and updates the Plotly renderer env var handling; Plotly's default notebook renderer is only set when Changes
Sequence Diagram(s)(omitted — changes are workflow and configuration updates without a multi-component runtime control flow that benefits from a sequence diagram) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (6)
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. Comment |
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: 3
🤖 Fix all issues with AI agents
In @.github/workflows/docs.yaml:
- Around line 75-82: The workflow step "Execute fast notebooks" uses grep -vFf
slow_notebooks.txt which will fail if docs/notebooks/slow_notebooks.txt is
missing; either add the file at docs/notebooks/slow_notebooks.txt with entries
matching the find output (e.g., ./notebook-name.ipynb) or modify the step to
guard the grep call (e.g., only use -f when the file exists) so the sh command
(with set -eo pipefail) won't error out when the file is absent.
- Line 31: The PLOTLY_RENDERER environment variable in the workflow is set to an
invalid value 'cdn'; update the PLOTLY_RENDERER entry to a valid Plotly renderer
(e.g., revert to 'json' or use another supported name like
'notebook'/'png'/'browser') or, if 'cdn' is a custom renderer, add a comment and
ensure the custom renderer is registered in the runtime; modify the
PLOTLY_RENDERER line accordingly so it uses a documented renderer name or
documents and validates the custom registration.
In @flixopt/config.py:
- Around line 839-841: Update the docstring that currently states "Sets plotly
renderer to 'notebook'" (the docstring preceding the code that assigns
pio.renderers.default) to note that the renderer is only set to 'notebook' when
the PLOTLY_RENDERER environment variable is not present; explicitly document
that setting PLOTLY_RENDERER overrides this behavior and that
pio.renderers.default will be left as specified by the environment variable.
🧹 Nitpick comments (1)
.github/workflows/docs.yaml (1)
148-154: Slow notebooks only execute on releases.This step correctly executes slow notebooks sequentially and only on releases (via the deploy job condition at line 98). However, this means changes to slow notebooks in PRs won't be tested until release time.
Consider whether the trade-off between CI speed and validation completeness is acceptable for your workflow. If slow notebooks contain critical functionality, you may want to:
- Run slow notebooks on a schedule (e.g., nightly) to catch issues earlier
- Add a manual workflow dispatch option to execute slow notebooks on-demand for specific PRs
- Use separate workflow triggers for documentation-only changes vs. code changes
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/docs.yamlflixopt/config.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: test (3.12)
- GitHub Check: test (3.14)
- GitHub Check: test (3.13)
- GitHub Check: test (3.11)
| - name: Execute fast notebooks | ||
| if: steps.notebook-cache.outputs.cache-hit != 'true' | ||
| run: | | ||
| set -eo pipefail | ||
| # Execute all notebooks in parallel (4 at a time) | ||
| # Run from notebooks directory so relative imports work | ||
| cd docs/notebooks && find . -name '*.ipynb' -print0 | \ | ||
| xargs -0 -P 4 -I {} sh -c 'jupyter execute --inplace "$1" || exit 255' _ {} | ||
| # Execute fast notebooks in parallel (4 at a time), excluding slow ones | ||
| cd docs/notebooks && find . -name '*.ipynb' | \ | ||
| grep -vFf slow_notebooks.txt | \ | ||
| xargs -P 4 -I {} sh -c 'jupyter execute --inplace "$1" || exit 255' _ {} |
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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify slow_notebooks.txt exists and validate path format
echo "=== Checking slow_notebooks.txt existence ==="
if [ -f docs/notebooks/slow_notebooks.txt ]; then
echo "✓ File exists at docs/notebooks/slow_notebooks.txt"
else
echo "✗ File NOT found at docs/notebooks/slow_notebooks.txt"
exit 1
fi
echo -e "\n=== Contents of slow_notebooks.txt ==="
cat docs/notebooks/slow_notebooks.txt
echo -e "\n=== Sample find output (first 5 notebooks) ==="
cd docs/notebooks && find . -name '*.ipynb' | head -5
echo -e "\n=== Validating path format and notebook existence ==="
cd docs/notebooks
invalid_count=0
missing_count=0
while IFS= read -r line; do
# Skip empty lines
[ -z "$line" ] && continue
# Check format: should start with ./ and end with .ipynb
if [[ ! "$line" =~ ^\./.*\.ipynb$ ]]; then
echo "WARNING: Path '$line' doesn't match expected ./...ipynb format"
((invalid_count++))
fi
# Check if notebook file exists
if [ ! -f "$line" ]; then
echo "ERROR: Notebook '$line' doesn't exist"
((missing_count++))
fi
done < slow_notebooks.txt
echo -e "\n=== Summary ==="
echo "Invalid format count: $invalid_count"
echo "Missing notebooks count: $missing_count"
if [ $invalid_count -eq 0 ] && [ $missing_count -eq 0 ]; then
echo "✓ All paths are valid and notebooks exist"
exit 0
else
echo "✗ Issues found with slow_notebooks.txt"
exit 1
fiRepository: flixOpt/flixopt
Length of output: 158
slow_notebooks.txt is missing and will cause the workflow to fail.
The file docs/notebooks/slow_notebooks.txt does not exist, but the step attempts to use it with grep -vFf slow_notebooks.txt. This will cause grep to exit with an error, which will fail the entire step due to set -eo pipefail. The file must be created in the repository with paths matching the find output format (e.g., ./notebook-name.ipynb), or the step must be modified to handle its absence.
🤖 Prompt for AI Agents
In @.github/workflows/docs.yaml around lines 75 - 82, The workflow step "Execute
fast notebooks" uses grep -vFf slow_notebooks.txt which will fail if
docs/notebooks/slow_notebooks.txt is missing; either add the file at
docs/notebooks/slow_notebooks.txt with entries matching the find output (e.g.,
./notebook-name.ipynb) or modify the step to guard the grep call (e.g., only use
-f when the file exists) so the sh command (with set -eo pipefail) won't error
out when the file is absent.
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Closes #(issue number)
Testing
Checklist
Summary by CodeRabbit
Chores
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.