-
Notifications
You must be signed in to change notification settings - Fork 152
Remove uninstallable revisions from lock files #929
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: master
Are you sure you want to change the base?
Changes from 13 commits
d550002
0b0e5a6
9d953a3
6263fb6
f926b66
732e484
ac1664c
8c920f8
c7696df
84418c8
7991eb4
d30bb8d
7548fad
b976dbc
c499cd0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,103 @@ | ||||||
| name: Fix Outdated Tools | ||||||
|
|
||||||
| on: | ||||||
| workflow_dispatch: | ||||||
| schedule: | ||||||
| - cron: '0 9 1 * *' | ||||||
|
|
||||||
| jobs: | ||||||
| get-lockfiles: | ||||||
| runs-on: ubuntu-latest | ||||||
| outputs: | ||||||
| lockfiles: ${{ steps.set-matrix.outputs.lockfiles }} | ||||||
| steps: | ||||||
| - name: Checkout repository | ||||||
| uses: actions/checkout@v5 | ||||||
|
|
||||||
| - name: Get all lock files | ||||||
| id: set-matrix | ||||||
| run: | | ||||||
| lockfiles=$(ls *.yaml.lock | jq -R -s -c 'split("\n")[:-1]') | ||||||
| echo "lockfiles=$lockfiles" >> $GITHUB_OUTPUT | ||||||
|
|
||||||
| fix-outdated: | ||||||
| needs: get-lockfiles | ||||||
| runs-on: ubuntu-latest | ||||||
| strategy: | ||||||
| matrix: | ||||||
| lockfile: ${{ fromJson(needs.get-lockfiles.outputs.lockfiles) }} | ||||||
| fail-fast: false | ||||||
| permissions: | ||||||
| contents: write | ||||||
| pull-requests: write | ||||||
| steps: | ||||||
| - name: Checkout repository | ||||||
| uses: actions/checkout@v5 | ||||||
|
|
||||||
| - name: Set up Python | ||||||
| uses: actions/setup-python@v6 | ||||||
| with: | ||||||
| python-version: '3.13' | ||||||
|
|
||||||
| - name: Install uv | ||||||
| uses: astral-sh/setup-uv@v7 | ||||||
| with: | ||||||
| github-token: ${{ secrets.GITHUB_TOKEN }} | ||||||
|
|
||||||
| - name: Install dependencies | ||||||
| run: uv pip install --system -r requirements.txt | ||||||
|
|
||||||
| - name: Fix ${{ matrix.lockfile }} | ||||||
| run: python scripts/fix_outdated.py "${{ matrix.lockfile }}" | ||||||
|
|
||||||
| - name: Upload changes | ||||||
| uses: actions/upload-artifact@v4 | ||||||
| if: always() | ||||||
| with: | ||||||
| name: ${{ matrix.lockfile }} | ||||||
| path: | | ||||||
| ${{ matrix.lockfile }} | ||||||
| *.not-installable-revisions.yaml | ||||||
|
||||||
| *.not-installable-revisions.yaml | |
| ${{ matrix.lockfile }}.not-installable-revisions.yaml |
Copilot
AI
Dec 3, 2025
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.
[nitpick] Inconsistent indentation: this line uses 4 spaces while other lines in the same block use 2 spaces. Consider using 2 spaces for consistency with the rest of the workflow file.
| fetch-depth: 0 | |
| fetch-depth: 0 |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,186 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import argparse | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import logging | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import sys | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import time | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from pathlib import Path | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from concurrent.futures import ThreadPoolExecutor, as_completed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from collections import defaultdict | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import yaml | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from bioblend import toolshed | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logging.basicConfig( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| level=logging.INFO, format="%(asctime)s - %(levelname)s - %(message)s" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger = logging.getLogger(__name__) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def retry_with_backoff(func, *args, **kwargs): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| MAX_RETRIES = 5 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
arash77 marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| backoff = 2 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for attempt in range(MAX_RETRIES): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return func(*args, **kwargs) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| error_msg = str(e) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if any( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| code in error_msg | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for code in ["502", "503", "504", "timed out", "timeout", "Connection"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if attempt < MAX_RETRIES - 1: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| f"Attempt {attempt + 1}/{MAX_RETRIES} failed: {error_msg}. Retrying in {backoff}s..." | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| time.sleep(backoff) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| backoff = min(backoff * 2, 60) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| continue | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise e | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| raise Exception("Retry failed after max attempts") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def get_tool_versions(ts, name, owner, revision): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| versions = set() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| repo_metadata = retry_with_backoff( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ts.repositories.get_repository_revision_install_info, name, owner, revision | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if isinstance(repo_metadata, list) and len(repo_metadata) > 1: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| for tool in repo_metadata[1].get("valid_tools", []): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| if "id" in tool and "version" in tool: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| versions.add((tool["id"], tool["version"])) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| except Exception as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning(f"{name},{owner}: failed to fetch {revision} ({e})") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
arash77 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| logger.warning(f"{name},{owner}: failed to fetch {revision} ({e})") | |
| logger.warning(f"{name},{owner}: failed to fetch {revision} ({type(e).__name__}: {e})") |
Copilot
AI
Dec 3, 2025
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.
Calling sys.exit(1) on a tool fetch failure will terminate the entire script, preventing other tools from being processed. Consider logging the error and continuing with the next tool, or collecting errors and exiting at the end if any critical failures occurred.
arash77 marked this conversation as resolved.
Show resolved
Hide resolved
Copilot
AI
Dec 3, 2025
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.
Catching a bare Exception is too broad. Consider catching more specific exception types or at minimum logging the exception type to aid debugging.
| logger.warning(f"{name},{owner}: error fetching {rev} ({e})") | |
| logger.warning(f"{name},{owner}: error fetching {rev} ({type(e).__name__}: {e})") |
Copilot
AI
Dec 3, 2025
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.
Calling sys.exit(1) inside the thread pool executor will terminate the script immediately when any revision fetch fails. This is inconsistent with the exception handling at line 54 and prevents other revisions from being processed. Consider collecting errors and handling them after all futures complete.
Copilot
AI
Dec 3, 2025
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 string replacement .replace(".yaml.lock", ".not-installable-revisions.yaml") assumes the lockfile name ends with ".yaml.lock". If the lockfile has a different naming pattern or doesn't contain this exact string, the replacement will fail silently and create an incorrectly named file. Consider using a more robust path manipulation approach, such as lockfile_path.with_suffix('') or checking that the expected suffix exists before replacing.
| not_installable_file = lockfile_path.with_name( | |
| lockfile_path.name.replace(".yaml.lock", ".not-installable-revisions.yaml") | |
| ) | |
| # Robustly generate the not-installable file name | |
| if lockfile_path.name.endswith(".yaml.lock"): | |
| not_installable_file = lockfile_path.with_name( | |
| lockfile_path.name.replace(".yaml.lock", ".not-installable-revisions.yaml") | |
| ) | |
| else: | |
| logger.warning( | |
| f"Lockfile name '{lockfile_path.name}' does not end with '.yaml.lock'. Using fallback naming." | |
| ) | |
| not_installable_file = lockfile_path.with_name( | |
| lockfile_path.name + ".not-installable-revisions.yaml" | |
| ) |
Copilot
AI
Dec 3, 2025
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.
'except' clause does nothing but pass and there is no explanatory comment.
| except FileNotFoundError: | |
| except FileNotFoundError: | |
| # If the file does not exist, proceed with an empty removed_map. |
Copilot
AI
Dec 3, 2025
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.
[nitpick] The magic number 10 (progress logging interval) should be defined as a constant for clarity and maintainability (e.g., PROGRESS_LOG_INTERVAL = 10).
Copilot
AI
Dec 3, 2025
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.
Catching a bare Exception is too broad. Consider catching more specific exception types related to API calls or at minimum logging the full exception type to aid debugging.
Copilot
AI
Dec 3, 2025
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 error message could be more helpful by providing actionable context. Consider adding information about what tool versions were found in the uninstallable revision and what the available installable signatures are, to help diagnose why no match was found.
| f"{name},{owner}: no matching installable revision for {cur}" | |
| f"{name},{owner}: no matching installable revision for {cur}\n" | |
| f" Signature of uninstallable revision: {sorted(cur_sig)}\n" | |
| f" Available installable signatures: {[sorted(sig) for sig in installable_signatures.keys()]}\n" | |
| f" Installable revisions: {installable_list}" |
Copilot
AI
Dec 3, 2025
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.
Calling sys.exit(1) when no matching installable revision is found will terminate the script and prevent other tools from being processed. Consider logging a warning and continuing with the next tool, or collecting critical errors to handle at the end.
| sys.exit(1) | |
| continue |
Copilot
AI
Dec 3, 2025
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 variable name nxt is unclear. Consider using a more descriptive name like matching_revision or installable_match to improve code readability.
| nxt = installable_list[-1] | |
| logger.info(f"{name},{owner}: unverifiable {cur}, keeping {nxt}") | |
| to_remove.add(cur) | |
| continue | |
| nxt = installable_signatures.get(cur_sig) | |
| if not nxt: | |
| logger.warning( | |
| f"{name},{owner}: no matching installable revision for {cur}" | |
| ) | |
| sys.exit(1) | |
| logger.info(f"{name},{owner}: removing {cur} in favor of {nxt}") | |
| if nxt not in current_revisions: | |
| tool["revisions"].append(nxt) | |
| matching_revision = installable_list[-1] | |
| logger.info(f"{name},{owner}: unverifiable {cur}, keeping {matching_revision}") | |
| to_remove.add(cur) | |
| continue | |
| matching_revision = installable_signatures.get(cur_sig) | |
| if not matching_revision: | |
| logger.warning( | |
| f"{name},{owner}: no matching installable revision for {cur}" | |
| ) | |
| sys.exit(1) | |
| logger.info(f"{name},{owner}: removing {cur} in favor of {matching_revision}") | |
| if matching_revision not in current_revisions: | |
| tool["revisions"].append(matching_revision) |
Copilot
AI
Dec 3, 2025
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 variable name nxt is unclear. Consider using a more descriptive name like replacement_revision or installable_revision to improve code readability.
| nxt = installable_list[-1] | |
| logger.info(f"{name},{owner}: unverifiable {cur}, keeping {nxt}") | |
| to_remove.add(cur) | |
| continue | |
| nxt = installable_signatures.get(cur_sig) | |
| if not nxt: | |
| logger.warning( | |
| f"{name},{owner}: no matching installable revision for {cur}" | |
| ) | |
| sys.exit(1) | |
| logger.info(f"{name},{owner}: removing {cur} in favor of {nxt}") | |
| if nxt not in current_revisions: | |
| tool["revisions"].append(nxt) | |
| replacement_revision = installable_list[-1] | |
| logger.info(f"{name},{owner}: unverifiable {cur}, keeping {replacement_revision}") | |
| to_remove.add(cur) | |
| continue | |
| replacement_revision = installable_signatures.get(cur_sig) | |
| if not replacement_revision: | |
| logger.warning( | |
| f"{name},{owner}: no matching installable revision for {cur}" | |
| ) | |
| sys.exit(1) | |
| logger.info(f"{name},{owner}: removing {cur} in favor of {replacement_revision}") | |
| if replacement_revision not in current_revisions: | |
| tool["revisions"].append(replacement_revision) |
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 artifact name uses
${{ matrix.lockfile }}which may contain special characters or path separators (e.g., "tools.yaml.lock"). Artifact names have restrictions and should not contain certain characters. Consider using a sanitized name or a unique identifier that replaces problematic characters.