Conversation
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughMultiple GitHub Actions workflows now include concurrency policies to cancel in-progress runs, and gallery scripts are updated to support backup data URLs via Zenodo alongside primary GitHub sources. The underlying Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing touches
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.
Pull request overview
This PR addresses documentation build failures caused by Zenodo rate limiting by switching to GitHub as the primary download source with Zenodo as a fallback. This improves reliability when building documentation and executing notebooks.
Changes:
- Enhanced
download_file()utility function to support backup URLs with retry logic and detailed error messaging - Updated all gallery script templates to use GitHub raw/release URLs as primary source with Zenodo as backup
- Added concurrency controls to GitHub Actions workflows to cancel in-progress runs when new pushes occur
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pyopenms_viz/util.py | Refactored download_file function to support backup URLs, retry logic, and enhanced error handling |
| docs/gallery_scripts_template/*.py | Updated 13 gallery scripts to use GitHub URLs as primary source with Zenodo backup |
| .github/workflows/static.yml | Added cancel-in-progress to concurrency settings |
| .github/workflows/execute_notebooks.yml | Added concurrency control with cancel-in-progress |
| .github/workflows/ci.yml | Added concurrency control with cancel-in-progress |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if i < len(urls_to_try) - 1: | ||
| print(f"Warning: {error_msg}\n Trying backup URL...") | ||
| else: | ||
| print(f"Error: {error_msg}") |
There was a problem hiding this comment.
The error messages on lines 56 and 58 will include the detailed error_msg which is already marked as either "Warning" or "Error" based on whether there are more URLs to try. However, the prefix in the print statements adds another "Warning:" or "Error:" label, resulting in duplicate labeling. Consider simplifying the print statements to avoid redundancy, such as using just print(error_msg) when i == len(urls_to_try) - 1, since error_msg doesn't include the "Warning:" or "Error:" prefix itself.
| error_msg = ( | ||
| f"Failed to download from URL ({i + 1}/{len(urls_to_try)}): {try_url}\n" | ||
| f" HTTP Status: {e.response.status_code}\n" | ||
| f" Reason: {e.response.reason}" | ||
| ) | ||
| # Check for common error codes and provide helpful messages | ||
| if e.response.status_code == 403: | ||
| error_msg += ( | ||
| "\n Note: 403 Forbidden often means the server is blocking automated requests " | ||
| "(rate limiting, bot detection, or access restrictions)." | ||
| ) | ||
| elif e.response.status_code == 404: |
There was a problem hiding this comment.
When catching requests.exceptions.HTTPError, the code accesses e.response.status_code and e.response.reason without checking if e.response is not None. While HTTPError typically has a response attribute populated, it's better to add a defensive check to prevent potential AttributeError if the response attribute is None in edge cases.
| error_msg = ( | |
| f"Failed to download from URL ({i + 1}/{len(urls_to_try)}): {try_url}\n" | |
| f" HTTP Status: {e.response.status_code}\n" | |
| f" Reason: {e.response.reason}" | |
| ) | |
| # Check for common error codes and provide helpful messages | |
| if e.response.status_code == 403: | |
| error_msg += ( | |
| "\n Note: 403 Forbidden often means the server is blocking automated requests " | |
| "(rate limiting, bot detection, or access restrictions)." | |
| ) | |
| elif e.response.status_code == 404: | |
| resp = e.response | |
| status_code = resp.status_code if resp is not None else "N/A" | |
| reason = resp.reason if resp is not None else "No response available" | |
| error_msg = ( | |
| f"Failed to download from URL ({i + 1}/{len(urls_to_try)}): {try_url}\n" | |
| f" HTTP Status: {status_code}\n" | |
| f" Reason: {reason}" | |
| ) | |
| # Check for common error codes and provide helpful messages | |
| if resp is not None and resp.status_code == 403: | |
| error_msg += ( | |
| "\n Note: 403 Forbidden often means the server is blocking automated requests " | |
| "(rate limiting, bot detection, or access restrictions)." | |
| ) | |
| elif resp is not None and resp.status_code == 404: |
Fix issues with docs build. Downloads with zenodo links now throws a 403 error, either due to reduce rate limits from excessive requests when building the notebooks.
We will now first try download from the github repo itself since it already has the test files, and then try zenodo as a backup if it also fails.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.