Skip to content

On-Demand Nightly PyPI Publishing #216

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

Closed
FindHao wants to merge 4 commits into from
Closed

On-Demand Nightly PyPI Publishing #216

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a9a0881
feat: implement on-demand nightly publishing
FindHao Dec 9, 2025
8e08eb7
remove extra space
FindHao Dec 9, 2025
bb60944
fix: address code review feedback
FindHao Dec 9, 2025
accb11d
change return value to 0
FindHao Dec 9, 2025
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
114 changes: 114 additions & 0 deletions .github/scripts/check_new_commits.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,114 @@
#!/bin/bash
# Copyright (c) Meta Platforms, Inc. and affiliates.
# Check if there are new commits since the last nightly PyPI release.
# This script is used by nightly-pypi.yml to implement on-demand nightly publishing.
#
# Exit codes:
# 0 - Should publish (new commits found or check skipped due to errors)
# 1 - Should skip publish (no new commits)
#
# Output:
# Sets GITHUB_OUTPUT variable: should_publish=true/false

set -o pipefail
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

This script only uses set -o pipefail without set -e, which is inconsistent with other bash scripts in the repository (e.g., .ci/setup.sh, .ci/install-project.sh, .ci/run-tests.sh) that all use set -e.

However, in this case, not using set -e appears to be intentional since the script has explicit error handling and wants to control when to fail vs. succeed. Consider adding a comment explaining why set -e is not used:

set -o pipefail
# Note: set -e is intentionally not used to allow explicit error handling
# The script implements fail-open behavior where errors should not block publishing
Suggested change
set -o pipefail
set -o pipefail
# Note: set -e is intentionally not used to allow explicit error handling
# The script implements fail-open behavior where errors should not block publishing

Copilot uses AI. Check for mistakes.
# Note: set -e is intentionally not used to allow explicit error handling.
# The script implements fail-open behavior where errors should not block publishing.
Comment on lines +13 to +15
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

[nitpick] The script uses set -o pipefail without set -e, which diverges from the repository convention (all scripts in .ci/ use set -e). While the comment explains that set -e is intentionally omitted for fail-open behavior, using pipefail alone is unusual. Consider either:

  1. Adding set -e and using explicit error handling with || true where needed for fail-open behavior
  2. Removing set -o pipefail since without set -e, it has limited effect in this script

The current approach works but may be confusing for maintainers familiar with the repository's bash script conventions.

Copilot uses AI. Check for mistakes.

# Configuration
MAX_RETRIES=3
RETRY_DELAY=5
CURL_TIMEOUT=10
PACKAGE_NAME="${PACKAGE_NAME:-tritonparse}"

# Dependencies: This script requires 'jq' and 'curl' to be installed.
# These are pre-installed on ubuntu-latest GitHub Actions runners.

Comment on lines +23 to +25
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The script assumes jq and curl are available (documented in lines 23-24) but doesn't verify this. While these tools are pre-installed on ubuntu-latest runners, adding explicit checks would make the script more robust and provide clearer error messages if run in an unexpected environment.

Consider adding at the start of main():

for cmd in jq curl git; do
    if ! command -v "$cmd" &> /dev/null; then
        echo "::error::Required command '$cmd' not found"
        exit 1
    fi
done

Copilot uses AI. Check for mistakes.
# Function: Fetch latest nightly version from PyPI with retry
fetch_latest_nightly() {
local retries=0

while [ $retries -lt $MAX_RETRIES ]; do
local response
response=$(curl -s --max-time $CURL_TIMEOUT \
"https://pypi.org/pypi/${PACKAGE_NAME}/json" 2>/dev/null)
local curl_exit=$?

if [ $curl_exit -eq 0 ] && [ -n "$response" ]; then
# Try to parse and extract latest dev version
local latest
latest=$(echo "$response" | \
jq -r '.releases | keys[] | select(contains(".dev"))' 2>/dev/null | \
sort -V | tail -1)
Comment on lines +39 to +41
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The script relies on jq but doesn't check if it's installed or install it if missing. While jq is pre-installed on ubuntu-latest runners, it's better to be explicit about this dependency.

Consider adding a check before using jq:

if ! command -v jq &>/dev/null; then
    echo "::error::jq is required but not installed"
    return 1
fi

Alternatively, add a comment documenting the dependency on jq being available in the GitHub Actions runner.

Copilot uses AI. Check for mistakes.
local jq_exit=$?

if [ $jq_exit -eq 0 ] && [ -n "$latest" ]; then
echo "$latest"
return 0
fi
fi

retries=$((retries + 1))
echo "::warning::PyPI API request failed (attempt $retries/$MAX_RETRIES)"

if [ $retries -lt $MAX_RETRIES ]; then
echo "Retrying in ${RETRY_DELAY}s..."
sleep $RETRY_DELAY
fi
done

echo "::warning::All $MAX_RETRIES attempts failed"
return 1
}

main() {
echo "Checking for new commits since last nightly release..."

# Step 1: Fetch latest nightly version (with retry)
LATEST_NIGHTLY=$(fetch_latest_nightly)
FETCH_STATUS=$?

# Step 2: Network request failed -> skip check, proceed with publish
if [ $FETCH_STATUS -ne 0 ]; then
echo "::warning::Failed to fetch PyPI data after $MAX_RETRIES attempts"
echo "::warning::Skipping commit check, proceeding with publish"
echo "should_publish=true" >> "$GITHUB_OUTPUT"
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The script writes to $GITHUB_OUTPUT without checking if the variable is set. If the script is run outside GitHub Actions or in testing, this could fail silently or write to an incorrect location.

Consider adding a check in main():

if [ -z "$GITHUB_OUTPUT" ]; then
    echo "::error::GITHUB_OUTPUT environment variable is not set"
    exit 1
fi

Copilot uses AI. Check for mistakes.
exit 0
fi

# Step 3: No nightly version exists -> first nightly release
if [ -z "$LATEST_NIGHTLY" ]; then
echo "No existing nightly version found, proceeding with publish"
echo "should_publish=true" >> "$GITHUB_OUTPUT"
exit 0
fi

echo "Latest nightly on PyPI: $LATEST_NIGHTLY"

# Step 4: Extract timestamp from version (format: X.Y.Z.devYYYYMMDDHHMMSS)
TIMESTAMP=$(echo "$LATEST_NIGHTLY" | sed -n 's/.*\.dev\([0-9]\{14\}\).*/\1/p')

if [ -z "$TIMESTAMP" ]; then
echo "::warning::Cannot parse timestamp from version, proceeding with publish"
echo "should_publish=true" >> "$GITHUB_OUTPUT"
exit 0
fi

# Step 5: Convert to date format for git log
SINCE_DATE="${TIMESTAMP:0:4}-${TIMESTAMP:4:2}-${TIMESTAMP:6:2} ${TIMESTAMP:8:2}:${TIMESTAMP:10:2}:${TIMESTAMP:12:2} UTC"
echo "Last nightly published at: $SINCE_DATE"

# Step 6: Check for new commits since last nightly
COMMITS_SINCE=$(git log --since="$SINCE_DATE" --oneline | wc -l)
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The git log --since comparison may exclude commits made at the exact same second as the last nightly release timestamp. While this is an edge case, it could theoretically cause a commit to be skipped if it was made in the same second the nightly was published.

Consider using --since with a 1-second offset to ensure inclusivity, or document this edge case behavior in the comments.

Copilot uses AI. Check for mistakes.

if [ "$COMMITS_SINCE" -eq 0 ]; then
echo "No new commits since last nightly, skipping publish"
echo "should_publish=false" >> "$GITHUB_OUTPUT"
exit 0
else
echo "Found $COMMITS_SINCE new commit(s) since last nightly"
echo "should_publish=true" >> "$GITHUB_OUTPUT"
exit 0
fi
}

main
12 changes: 10 additions & 2 deletions .github/workflows/nightly-pypi.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,9 +23,15 @@ jobs:
- uses: actions/setup-python@v5
with:
python-version: "3.11"

- name: Check for new commits since last nightly
id: check
if: github.ref_type != 'tag'
run: bash .github/scripts/check_new_commits.sh

- name: Compute nightly version from latest tag (next patch + timestamp)
id: ver
if: github.ref_type != 'tag'
if: github.ref_type != 'tag' && steps.check.outputs.should_publish != 'false'
run: |
# Get latest tag; allow 'v' prefix; fail if none
if ! TAG=$(git describe --tags --abbrev=0 2>/dev/null); then
Expand All @@ -46,6 +52,7 @@ jobs:
echo "Computed nightly version: ${NEXT}.dev${DATE}"

- name: Build sdist/wheel
if: github.ref_type == 'tag' || steps.check.outputs.should_publish != 'false'
run: |
python -m pip install --upgrade pip
pip install build setuptools-scm
Expand All @@ -55,12 +62,13 @@ jobs:
python -m build

- name: Check metadata
if: github.ref_type == 'tag' || steps.check.outputs.should_publish != 'false'
run: |
pip install twine
twine check dist/*

- name: Publish to PyPI (API token)
if: github.event_name == 'schedule' || github.ref_type == 'tag'
if: (github.event_name == 'schedule' || github.ref_type == 'tag') && steps.check.outputs.should_publish != 'false'
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

The publish step will never execute on workflow_dispatch events. The condition requires github.event_name == 'schedule' || github.ref_type == 'tag', but manual workflow runs trigger workflow_dispatch events.

According to the PR description and the comment at line 5, manual runs should not publish. However, if the intention is to allow manual publishing for testing purposes, you should either:

  1. Add workflow_dispatch to the condition: github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' || github.ref_type == 'tag'
  2. Or clarify in the comment that manual runs will build but not publish

If manual runs are intentionally prevented from publishing, the current implementation is correct but could be more explicit with a skip message.

Copilot uses AI. Check for mistakes.
uses: pypa/gh-action-pypi-publish@release/v1
with:
user: __token__
Expand Down