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.

Sign up for GitHub

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

New examples for the updated documentation #495

Merged
merged 39 commits into from
Nov 20, 2024
Merged

New examples for the updated documentation #495

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
73dc0de
new examples
jan-janssen Nov 12, 2024
b5bf962
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 12, 2024
a31700b
Build notebooks as tests
jan-janssen Nov 12, 2024
362a25a
add executor bit
jan-janssen Nov 12, 2024
8cb4dd7
extend notebook environment
jan-janssen Nov 12, 2024
b2088a0
Update 3-hpc-allocation.ipynb
jan-janssen Nov 12, 2024
a35430e
Add key features
jan-janssen Nov 13, 2024
3ab770e
Merge remote-tracking branch 'origin/main' into examples
jan-janssen Nov 15, 2024
ee2a158
update key arguments
jan-janssen Nov 15, 2024
f688059
Merge remote-tracking branch 'origin/main' into examples
jan-janssen Nov 15, 2024
e8a9987
Work in progress for the readme
jan-janssen Nov 15, 2024
faa2c62
Update readme
jan-janssen Nov 15, 2024
9d688df
add new lines
jan-janssen Nov 15, 2024
936fa62
Merge remote-tracking branch 'origin/main' into examples
jan-janssen Nov 15, 2024
acede13
Change Backend Names
jan-janssen Nov 15, 2024
ee532a7
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 15, 2024
a1359d4
Update __init__.py
jan-janssen Nov 15, 2024
74bee15
Merge remote-tracking branch 'origin/main' into examples
jan-janssen Nov 19, 2024
294db24
update readme
jan-janssen Nov 19, 2024
8fa5d7b
Update installation
jan-janssen Nov 19, 2024
307fb40
Fix init
jan-janssen Nov 19, 2024
52d9723
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 19, 2024
7a6b068
update local notebook
jan-janssen Nov 19, 2024
59f6b97
Merge remote-tracking branch 'origin/examples' into examples
jan-janssen Nov 19, 2024
948ea0e
Merge remote-tracking branch 'origin/main' into examples
jan-janssen Nov 19, 2024
4432c65
update local example notebook
jan-janssen Nov 19, 2024
e209d8c
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 19, 2024
103ff06
Explain jupyter kernel installation
jan-janssen Nov 19, 2024
1481110
Merge remote-tracking branch 'origin/examples' into examples
jan-janssen Nov 19, 2024
bca3c32
copy existing kernel
jan-janssen Nov 19, 2024
b8ab12f
Add HPC submission
jan-janssen Nov 19, 2024
6fabda8
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 19, 2024
a6ca0f5
execute HPC notebook once
jan-janssen Nov 20, 2024
21b7099
hpc allocation
jan-janssen Nov 20, 2024
ee386f7
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 20, 2024
1632ba1
Update documentation
jan-janssen Nov 20, 2024
99e5126
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Nov 20, 2024
0b239e2
replace HPC submission notebook
jan-janssen Nov 20, 2024
a11e378
Merge remote-tracking branch 'origin/examples' into examples
jan-janssen Nov 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions .ci_support/build_notebooks.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
#!/bin/bash
# execute notebooks
i=0;
for notebook in $(ls notebooks/*.ipynb); do
papermill ${notebook} ${notebook%.*}-out.${notebook##*.} -k python3 || i=$((i+1));
done;
Comment on lines +4 to +6
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Fix fragile file iteration and improve error logging

The current implementation has several issues:

  1. Using ls for iteration is fragile and will break with spaces in filenames
  2. Failed notebook executions aren't logged
  3. No progress indication during execution

Apply this diff to make the script more robust:

-for notebook in $(ls notebooks/*.ipynb); do
-    papermill ${notebook} ${notebook%.*}-out.${notebook##*.} -k python3 || i=$((i+1));
-done;
+# Create a log directory
+mkdir -p logs
+
+# Use shell globbing instead of ls
+for notebook in notebooks/*.ipynb; do
+    if [ ! -f "$notebook" ]; then
+        continue  # Skip if no notebooks found
+    fi
+    
+    echo "Processing: $notebook"
+    base_name=$(basename "$notebook" .ipynb)
+    output_file="${notebook%.*}-out.${notebook##*.}"
+    log_file="logs/${base_name}.log"
+    
+    if ! papermill "$notebook" "$output_file" -k python3 2>&1 | tee "$log_file"; then
+        echo "Failed to execute: $notebook" >&2
+        i=$((i+1))
+    fi
+done
📝 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.

Suggested change
for notebook in $(ls notebooks/*.ipynb); do
papermill ${notebook} ${notebook%.*}-out.${notebook##*.} -k python3 || i=$((i+1));
done;
# Create a log directory
mkdir -p logs
# Use shell globbing instead of ls
for notebook in notebooks/*.ipynb; do
if [ ! -f "$notebook" ]; then
continue # Skip if no notebooks found
fi
echo "Processing: $notebook"
base_name=$(basename "$notebook" .ipynb)
output_file="${notebook%.*}-out.${notebook##*.}"
log_file="logs/${base_name}.log"
if ! papermill "$notebook" "$output_file" -k python3 2>&1 | tee "$log_file"; then
echo "Failed to execute: $notebook" >&2
i=$((i+1))
fi
done
🧰 Tools
🪛 Shellcheck

[error] 4-4: Iterating over ls output is fragile. Use globs.

(SC2045)


# push error to next level
if [ $i -gt 0 ]; then
exit 1;
fi;
Comment on lines +8 to +11
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance error reporting with execution summary

The current error handling doesn't provide enough information about the execution results.

Apply this diff to improve error reporting:

-# push error to next level
-if [ $i -gt 0 ]; then
-    exit 1;
-fi;
+# Print execution summary
+total=$(find notebooks -name "*.ipynb" | wc -l)
+successful=$((total - i))
+
+echo "Notebook Execution Summary:"
+echo "-------------------------"
+echo "Total notebooks: $total"
+echo "Successfully executed: $successful"
+echo "Failed executions: $i"
+
+if [ $i -gt 0 ]; then
+    echo "Error: $i notebook(s) failed execution. Check logs directory for details." >&2
+    exit 1
+fi
+
+echo "All notebooks executed successfully!"
📝 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.

Suggested change
# push error to next level
if [ $i -gt 0 ]; then
exit 1;
fi;
# Print execution summary
total=$(find notebooks -name "*.ipynb" | wc -l)
successful=$((total - i))
echo "Notebook Execution Summary:"
echo "-------------------------"
echo "Total notebooks: $total"
echo "Successfully executed: $successful"
echo "Failed executions: $i"
if [ $i -gt 0 ]; then
echo "Error: $i notebook(s) failed execution. Check logs directory for details." >&2
exit 1
fi
echo "All notebooks executed successfully!"

2 changes: 1 addition & 1 deletion .github/workflows/notebooks.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,4 @@ jobs:
timeout-minutes: 5
run: >
flux start
papermill notebooks/examples.ipynb examples-out.ipynb -k "python3"
.ci_support/build_notebooks.sh
Copy link
Contributor

Choose a reason for hiding this comment

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

💡 Codebase verification

Increase the workflow timeout.

The current 5-minute timeout may be insufficient for executing all 5 notebooks. Consider increasing the timeout-minutes value to ensure successful completion.

🔗 Analysis chain

Consider increasing the workflow timeout.

The workflow is now executing multiple notebooks through the shell script, but still uses a 5-minute timeout. This might be insufficient when running all notebooks sequentially, especially those involving HPC and resource allocation examples.

Consider:

  1. Increasing the timeout-minutes value
  2. Adding timeouts per notebook in the shell script instead
🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Count notebooks and estimate required time
echo "Number of notebooks to be executed:"
fd .ipynb$ notebooks/ | wc -l

Length of output: 535

Script:

#!/bin/bash
# Count the number of Jupyter notebooks
echo "Total number of notebooks:"
fd .ipynb$ notebooks/ | wc -l

Length of output: 98

Loading
Loading