Skip to content

ci: retry PyPI install smoke test to tolerate index propagation #437

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

Merged
bigcat88 merged 1 commit into from
Apr 24, 2026
Merged

ci: retry PyPI install smoke test to tolerate index propagation #437

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
21 changes: 18 additions & 3 deletions .github/workflows/publish_package.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,10 @@ jobs:

- name: Extract version from tag
id: get_version
run: echo "VERSION=${GITHUB_REF#refs/tags/}" >> $GITHUB_ENV
run: |
VERSION="${GITHUB_REF#refs/tags/}"
VERSION="${VERSION#v}"
echo "VERSION=${VERSION}" >> $GITHUB_ENV
Comment on lines +29 to +32
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Tiny DRY nit: the v-stripping dance is duplicated in both jobs.

No bug here — just a pinch of repetition. You could compute VERSION once in build-n-publish-pypi and expose it via jobs.<id>.outputs, then consume it in test-pip-installation through needs.build-n-publish-pypi.outputs.version. One tag, one truth — no copy-paste to chase.

Also worth noting: ${VERSION#v} strips only a lowercase leading v. If a maintainer ever tags V1.2.3, it'll sneak through un-stripped. Unlikely given your tagging convention, but a ${VERSION#[vV]} would slam that door shut.

♻️ Optional refactor sketch (share the version across jobs) 
   build-n-publish-pypi:
     ...
+    outputs:
+      version: ${{ steps.get_version.outputs.version }}
     steps:
       ...
       - name: Extract version from tag
         id: get_version
         run: |
           VERSION="${GITHUB_REF#refs/tags/}"
-          VERSION="${VERSION#v}"
+          VERSION="${VERSION#[vV]}"
           echo "VERSION=${VERSION}" >> $GITHUB_ENV
+          echo "version=${VERSION}" >> $GITHUB_OUTPUT
   ...
   test-pip-installation:
     needs: build-n-publish-pypi
     ...
     steps:
       ...
-      - name: Extract version from tag
-        id: get_version
-        run: |
-          VERSION="${GITHUB_REF#refs/tags/}"
-          VERSION="${VERSION#v}"
-          echo "VERSION=${VERSION}" >> $GITHUB_ENV
+      - name: Resolve version from upstream job
+        run: echo "VERSION=${{ needs.build-n-publish-pypi.outputs.version }}" >> $GITHUB_ENV

Also applies to: 78-81

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/publish_package.yml around lines 29 - 32, Compute and
expose the canonical VERSION once in the build-n-publish-pypi job and consume it
in test-pip-installation via job outputs to avoid duplicating the v-stripping
logic: move the VERSION computation (use parameter expansion that strips both
lowercase and uppercase prefixes, e.g. `${VERSION#[vV]}`) into the
build-n-publish-pypi step, set it as an output
(jobs.build-n-publish-pypi.outputs.version), and update test-pip-installation to
read needs.build-n-publish-pypi.outputs.version instead of recomputing; update
any other duplicated occurrences (e.g., the block around lines 78-81) to use the
shared output as well.


- name: Update version in pyproject.toml
run: |
Expand Down Expand Up @@ -72,10 +75,22 @@ jobs:

- name: Extract version from tag
id: get_version
run: echo "VERSION=${GITHUB_REF#refs/tags/}" >> $GITHUB_ENV
run: |
VERSION="${GITHUB_REF#refs/tags/}"
VERSION="${VERSION#v}"
echo "VERSION=${VERSION}" >> $GITHUB_ENV

- name: Install Comfy CLI via pip and Test
run: pip install comfy-cli==${{env.VERSION}}
run: |
# PyPI's index can lag behind a successful upload by a minute or
# two, so retry before failing the job.
for i in 1 2 3 4 5 6 7 8; do
pip install --no-cache-dir "comfy-cli==${VERSION}" && exit 0
echo "Attempt $i: package not yet available on PyPI, waiting 15s..."
sleep 15
done
echo "::error::Failed to install comfy-cli==${VERSION} after 8 attempts"
exit 1
Comment on lines 83 to +93
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 24, 2026

Choose a reason for hiding this comment

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

🧹 Nitpick | 🔵 Trivial

Retry loop looks solid — here's a small polish to keep diagnostics crisp.

The logic holds up (thanks to GitHub's default bash -eo pipefail, the failing pip install inside a && list won't abort the script, so the retry actually retries — nice). Two small, optional tweaks:

  1. Capture why pip bailed. Right now any failure — stale index, network hiccup, resolver conflict — looks identical. Teeing pip's output (or echoing its exit code) on each attempt makes post-mortems far less mysterious when the loop exhausts.
  2. Guard against a pip-hang. pip install has no built-in per-attempt deadline, so in the pathological case you could burn the runner's job timeout on a single stuck attempt. A --timeout (network socket timeout) plus a wrapping timeout 60s gives the retry loop teeth.
🛠️ Proposed polish for the retry loop 
       - name: Install Comfy CLI via pip and Test
         run: |
           # PyPI's index can lag behind a successful upload by a minute or
           # two, so retry before failing the job.
           for i in 1 2 3 4 5 6 7 8; do
-            pip install --no-cache-dir "comfy-cli==${VERSION}" && exit 0
-            echo "Attempt $i: package not yet available on PyPI, waiting 15s..."
+            if timeout 60s pip install --no-cache-dir --timeout 30 "comfy-cli==${VERSION}"; then
+              exit 0
+            fi
+            echo "Attempt $i failed (exit=$?); waiting 15s for PyPI index to catch up..."
             sleep 15
           done
           echo "::error::Failed to install comfy-cli==${VERSION} after 8 attempts"
           exit 1

A rhyme to sum it up: the loop is sound and tightly bound; log the cause so the fault is found. 🐇

🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/publish_package.yml around lines 83 - 93, In the "Install
Comfy CLI via pip and Test" step update the retry loop to capture and surface
pip's failure details and to bound each attempt with a timeout: run pip install
"comfy-cli==${VERSION}" under a per-attempt timeout (e.g. with the timeout
utility) and use pip's network timeout flag (--timeout) as well; on a failed
attempt capture pip's exit code and stderr/stdout (or at least echo the exit
code) before sleeping so the final "::error::" contains diagnostic info; ensure
the loop still exits on success (exit 0) and preserves the existing retry count
and sleep behavior.


- name: Test Comfy CLI Help
run: comfy --help
Loading