Skip to content

Conversation

@FindHao
Copy link
Member

@FindHao FindHao commented Dec 9, 2025

Summary

This PR implements on-demand nightly publishing for the tritonparse package. Instead of publishing a new nightly version every day regardless of changes, the workflow now checks if there are new commits since the last nightly release and skips publishing if there are none.

Changes

New File: .github/scripts/check_new_commits.sh

A standalone script that:

  1. Queries PyPI API to get the latest nightly version (e.g., 0.3.2.dev20251208071617)
  2. Extracts the timestamp from the version string
  3. Uses git log --since to check for new commits after that timestamp
  4. Outputs should_publish=true/false to GITHUB_OUTPUT

Key features:

  • Network retry mechanism: 3 retries with 5s delay between attempts
  • Fail-open design: If PyPI API is unreachable, proceeds with publish (never blocks releases due to network issues)
  • Edge case handling: Handles first nightly release, unparseable versions, etc.

Modified: .github/workflows/nightly-pypi.yml

  • Added a new step to run check_new_commits.sh before computing the nightly version
  • All subsequent steps (build, check, publish) are conditioned on should_publish != 'false'
  • Tag-based releases bypass the check entirely

Workflow Logic

Trigger (schedule/workflow_dispatch/tag)
         │
         ▼
    Is tag push? ───Yes──► Build and Publish directly
         │
         No
         ▼
    Run check_new_commits.sh
         │
         ▼
    Has new commits? ───No──► Skip (exit early)
         │
         Yes
         ▼
    Compute version → Build → Publish

Benefits

  • Reduces noise: No more daily releases with identical content
  • Saves resources: Skips unnecessary CI builds
  • Cleaner PyPI history: Only releases with actual changes
  • Robust: Network failures don't block legitimate releases

Testing

Run locally with:

cd tritonparse_oss
GITHUB_OUTPUT=/tmp/gh_output.txt bash .github/scripts/check_new_commits.sh
cat /tmp/gh_output.txt

- Add check_new_commits.sh script to detect if there are new commits since last nightly release
- Query PyPI API for latest nightly version and extract timestamp
- Skip publishing if no new commits, reducing unnecessary releases
- Include network retry mechanism (3 retries) with fail-open design
Copilot AI review requested due to automatic review settings December 9, 2025 01:11
@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Dec 9, 2025
@meta-codesync
Copy link

meta-codesync bot commented Dec 9, 2025

@FindHao has imported this pull request. If you are a Meta employee, you can view this in D88696841.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements an intelligent on-demand nightly publishing system for the tritonparse package. Instead of blindly publishing every day, the workflow now checks PyPI for the latest nightly release, extracts its timestamp, and only publishes if there are new commits since then. The implementation includes robust error handling with network retries and a fail-open design that prevents legitimate releases from being blocked by transient failures.

Key Changes:

  • Added a new bash script that queries PyPI API to check for commits since the last nightly release
  • Modified the workflow to conditionally skip build and publish steps when no new commits exist
  • Implemented network retry mechanism with 3 attempts and fail-open behavior for reliability

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 8 comments.

File Description
.github/scripts/check_new_commits.sh New script that checks PyPI for the latest nightly version, extracts its timestamp, and determines if new commits exist since then. Sets should_publish output variable.
.github/workflows/nightly-pypi.yml Added step to run the commit check script and conditioned all subsequent build/publish steps on the should_publish output to skip unnecessary releases.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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.
Comment on lines +34 to +36
latest=$(echo "$response" | \
jq -r '.releases | keys[] | select(contains(".dev"))' 2>/dev/null | \
sort -V | tail -1)
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.
Comment on lines 37 to 38

if [ $? -eq 0 ]; then
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 exit code check at line 38 captures the status of jq, not the entire pipeline. Due to set -o pipefail at line 13, the pipeline status should be captured correctly, but checking $? after subsequent operations may not work as expected.

The issue is that $? on line 38 is checking the exit status of the if command from line 38, not the jq pipeline. Since you're already checking if the curl succeeded and the response is non-empty, consider removing this redundant check or explicitly capture the pipeline status:

latest=$(echo "$response" | \
    jq -r '.releases | keys[] | select(contains(".dev"))' 2>/dev/null | \
    sort -V | tail -1)
local jq_exit=$?

if [ $jq_exit -eq 0 ] && [ -n "$latest" ]; then
    echo "$latest"
    return 0
fi
Suggested change
if [ $? -eq 0 ]; then
local pipeline_exit=$?
if [ $pipeline_exit -eq 0 ]; then

Copilot uses AI. Check for mistakes.
- 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.
id: check
if: github.ref_type != 'tag'
run: bash .github/scripts/check_new_commits.sh
continue-on-error: true
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.

Using continue-on-error: true can cause unexpected behavior when checking steps.check.outputs.should_publish. If the script fails with an error (not related to network issues), the step will be marked as successful, but should_publish might not be set at all, which could lead to skipped builds even when they should happen.

Consider one of these approaches:

  1. Remove continue-on-error: true and rely on the script's fail-open design (it already exits with 0 on errors)
  2. Add an additional condition to check if the step was successful: if: github.ref_type == 'tag' || steps.check.outcome == 'skipped' || steps.check.outputs.should_publish != 'false'

The current script already handles errors gracefully by exiting with 0 and setting should_publish=true, so continue-on-error: true may be redundant.

Suggested change
continue-on-error: true

Copilot uses AI. Check for mistakes.
echo "Latest nightly on PyPI: $LATEST_NIGHTLY"

# Step 4: Extract timestamp from version (format: X.Y.Z.devYYYYMMDDHHMMSS)
TIMESTAMP=$(echo "$LATEST_NIGHTLY" | grep -oP '\.dev\K[0-9]{14}' || true)
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 grep -oP command uses Perl-compatible regular expressions (PCRE) which may not be available in all environments. While it should work on ubuntu-latest, using a more portable approach would be more robust.

Consider using sed or awk instead:

TIMESTAMP=$(echo "$LATEST_NIGHTLY" | sed -n 's/.*\.dev\([0-9]\{14\}\).*/\1/p')

Alternatively, add a comment documenting the dependency on GNU grep with PCRE support.

Suggested change
TIMESTAMP=$(echo "$LATEST_NIGHTLY" | grep -oP '\.dev\K[0-9]{14}' || true)
TIMESTAMP=$(echo "$LATEST_NIGHTLY" | sed -n 's/.*\.dev\([0-9]\{14\}\).*/\1/p')

Copilot uses AI. Check for mistakes.
# 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.
FindHao and others added 2 commits December 8, 2025 17:22
Co-authored-by: Copilot <[email protected]>
- Add comments explaining why set -e is not used (fail-open design)
- Add comments documenting jq/curl dependencies
- Explicitly capture jq exit status and check for non-empty result
- Replace grep -oP with sed for better portability
- Remove unnecessary continue-on-error: true from workflow
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +13 to +15
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.
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.
Comment on lines +23 to +25
# Dependencies: This script requires 'jq' and 'curl' to be installed.
# These are pre-installed on ubuntu-latest GitHub Actions runners.

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.
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.
@meta-codesync
Copy link

meta-codesync bot commented Dec 9, 2025

@FindHao merged this pull request in 5159847.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Meta Open Source bot. Merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants