Skip to content

[CI] Fix empty wheel artifact checks - #8434

Merged
huydhn merged 2 commits into
pytorch:mainfrom
dulinriley:codex/fix-optional-artifact-upload
Aug 12, 2026
Merged

[CI] Fix empty wheel artifact checks#8434
huydhn merged 2 commits into
pytorch:mainfrom
dulinriley:codex/fix-optional-artifact-upload

Conversation

@dulinriley

@dulinriley dulinriley commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

There was a bug in artifact preparation where find "dist/" -name "*.whl" returned success whenever dist/ existed, even if it contained no wheels. The following mv dist/*.whl then failed and obscured the earlier build failure. This was observed in https://github.com/meta-pytorch/monarch/actions/runs/30938873470/job/92093926645.

This change iterates over dist/*.whl and moves entries only when [[ -f "${wheel}" ]] succeeds. Missing and empty dist/ directories are no-ops, while an actual mv error still fails the artifact preparation step.

Test plan:

  • Ran the repository actionlint on the five changed workflows.
  • Verified missing, empty, and populated dist/ behavior under Bash, including a wheel filename containing spaces.
  • Verified that a real mv failure is propagated.

@vercel

vercel Bot commented Aug 4, 2026

Copy link
Copy Markdown

@dulinriley is attempting to deploy a commit to the Meta Open Source Team on Vercel.

A member of the Team first needs to authorize it.

@meta-cla meta-cla Bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Aug 4, 2026
@dulinriley

Copy link
Copy Markdown
Contributor Author

How do I merge this? I don't see the merge button, and I can't see a way to import it. Is it via pytorchbot?

@huydhn

huydhn commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

@claude are there any tests we can add for this change

@claude

claude Bot commented Aug 12, 2026

Copy link
Copy Markdown

Claude finished @huydhn's task in 1m 44s —— View job


Tests for the wheel-artifact fix

  • Read the PR change
  • Look for existing shell/workflow test infrastructure
  • Answer: what tests can we add?

Short answer: yes, but the logic is currently inline shell inside the workflow YAML, which isn't unit-testable as-is. There's no shell-test harness in this repo today (only actionlint), so meaningful tests require extracting the snippet into a script. Two options:


Option A — Extract to a script + add a test (recommended)

Pull the wheel-collection logic into .github/scripts/prepare_wheel_artifacts.sh:

#!/usr/bin/env bash
set -euo pipefail
# Usage: prepare_wheel_artifacts.sh <src_dir> <dest_dir>
src_dir="${1:-dist}"
dest_dir="${2:-${RUNNER_ARTIFACT_DIR}}"
for wheel in "${src_dir}"/*.whl; do
  if [[ -f "${wheel}" ]]; then
    mv -v "${wheel}" "${dest_dir}/"
  fi
done

Each of the 5 workflows then calls the script instead of duplicating the loop (the block is currently identical in linux_job.yml, linux_job_v2.yml, linux_job_v3.yml, macos_job.yml, windows_job.yml), which also removes the copy-paste drift risk.

Then a bats test (or plain shunit2) covers exactly the cases called out in the PR test plan:

#!/usr/bin/env bats

setup() {
  SRC="$(mktemp -d)"; DEST="$(mktemp -d)"
}
teardown() { rm -rf "${SRC}" "${DEST}"; }

@test "missing dist dir is a no-op" {
  rm -rf "${SRC}"
  run .github/scripts/prepare_wheel_artifacts.sh "${SRC}" "${DEST}"
  [ "$status" -eq 0 ]
  [ -z "$(ls -A "${DEST}")" ]
}

@test "empty dist dir is a no-op (does not move literal *.whl)" {
  run .github/scripts/prepare_wheel_artifacts.sh "${SRC}" "${DEST}"
  [ "$status" -eq 0 ]
  [ -z "$(ls -A "${DEST}")" ]
}

@test "moves wheels, including filenames with spaces" {
  touch "${SRC}/torch-2.0.whl" "${SRC}/my package.whl"
  run .github/scripts/prepare_wheel_artifacts.sh "${SRC}" "${DEST}"
  [ "$status" -eq 0 ]
  [ -f "${DEST}/torch-2.0.whl" ]
  [ -f "${DEST}/my package.whl" ]
}

@test "propagates a real mv failure" {
  touch "${SRC}/torch-2.0.whl"
  chmod 000 "${DEST}"                       # unwritable dest
  run .github/scripts/prepare_wheel_artifacts.sh "${SRC}" "${DEST}"
  chmod 755 "${DEST}"
  [ "$status" -ne 0 ]
}

These four cases map 1:1 to your manual test plan (missing / empty / populated / space-in-name / propagated failure). A tiny CI job (bats-core/bats-action) runs them on PRs.

Option B — Keep it inline, add no test

If extraction feels heavier than the fix warrants, it's reasonable to keep the inline loop and rely on the manual verification already in the PR description. actionlint won't catch this class of bug, so there'd be no automated regression guard.


Note: GitHub App permissions don't allow me to modify files under .github/workflows/, so I can't wire the workflow run: steps to call a new script myself. I can add the standalone script + bats test (they live under .github/scripts/ and a test dir) if you'd like — just say the word and confirm you're OK with the extraction approach.
· branch codex/fix-optional-artifact-upload

@huydhn huydhn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!

@huydhn

huydhn commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

How do I merge this? I don't see the merge button, and I can't see a way to import it. Is it via pytorchbot?

Let me merge it

@huydhn
huydhn merged commit fedf511 into pytorch:main Aug 12, 2026
52 of 61 checks passed
@dulinriley
dulinriley deleted the codex/fix-optional-artifact-upload branch August 12, 2026 22:08
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 Facebook bot. Authors need to sign the CLA before a PR can be reviewed.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants