Skip to content

Publish to PyPI and GitHub upon tag - #52

Open
loriab wants to merge 8 commits into
mainfrom
addpub
Open

Publish to PyPI and GitHub upon tag#52
loriab wants to merge 8 commits into
mainfrom
addpub

Conversation

@loriab

@loriab loriab commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator

No description provided.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Not ready to approve

The new release workflow currently allows the TestPyPI publish job to run on pull_request events, which risks unintended publish attempts from PRs.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

This PR updates QCManyBody’s packaging/release automation and dependency/runtime baselines, including adding a GitHub Actions release workflow to build and publish distributions and aligning docs/CI metadata with the new minimum supported Python.

Changes:

  • Add a new GitHub Actions Release workflow to build distributions, publish to TestPyPI/PyPI, and create a signed GitHub Release.
  • Bump the minimum supported Python version to 3.10 and update dependency pins from RC/dev labels to stable versions.
  • Adjust CI workflow to remove the QCSchema-next matrix job and dev-label conda channels; small warning-call and README badge updates.
File summaries
File Description
README.md Updates the Python version badge to 3.10+.
qcmanybody/models/v1/generalized_optimization.py Fixes warnings.warn invocation to use category=FutureWarning.
pyproject.toml Raises requires-python to 3.10 and updates qcelemental/qcengine version constraints.
docs/changelog.rst Adds an unreleased v0.7.2 changelog section referencing recent PRs.
.github/workflows/release.yml Introduces automated build/publish/sign/release workflow for PyPI/TestPyPI and GitHub Releases.
.github/workflows/ci.yml Removes QCSk-next matrix/dev channels and updates CI dependency versions accordingly.
Review details
  • Files reviewed: 6/6 changed files
  • Comments generated: 2
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

Comment thread .github/workflows/release.yml
Comment thread .github/workflows/ci.yml
@loriab
loriab requested a review from Copilot August 3, 2026 06:55
@loriab loriab changed the title auto pub to pypi Publish to PyPI and GitHub upon tag Aug 3, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Not ready to approve

The new release workflow’s version-validation script will fail at runtime because it treats parsed distribution versions as strings but accesses version.local (needs conversion to packaging.version.Version).

Once you've addressed the issues Copilot identified, you can request another Copilot review.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Review details

Suppressed comments (1)

.github/workflows/release.yml:78

  • In the version-validation script, parse_wheel_filename / parse_sdist_filename return the distribution version as a string. The current code assigns that string to version and then accesses version.local, which will raise an AttributeError (and the subsequent comparison to tag_version will never succeed). Convert the parsed string to packaging.version.Version (with error handling) before checking .local or comparing to the tag.
          version = versions.pop()
          if version.local is not None:
              raise SystemExit(f"Public indexes reject local version {version}")

          if os.environ["GITHUB_REF_TYPE"] == "tag":
  • Files reviewed: 6/6 changed files
  • Comments generated: 0 new
  • Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

@loriab
loriab requested a review from bennybp August 3, 2026 07:17
@loriab

loriab commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

Claude's review (Sonnet 5.6)

I'll start by looking at the diff between this branch and main to understand what's being added.

Reviewed the branch diff against main, and did a real local build/publish-check to verify the workflow's assumptions rather than just reading YAML. Overall this is a clean, well-built release pipeline — it follows the current PyPA-recommended pattern (OIDC trusted publishing, artifact reuse across jobs, Sigstore signing, idempotent GitHub Release creation) and I'd be comfortable using it as the template for your other packages. One finding is worth fixing before you cut the first real tag, though.

Real issue — the wheel bundles ~1MB of test fixtures (84% of package content)

I ran python -m build locally against this branch. The resulting wheel includes all of qcmanybody/tests/ — reference .json.zst outputs, .inp files, etc. — alongside the actual library code:

total uncompressed: 1,260,766 bytes
qcmanybody/tests/*:  1,059,446 bytes (84.0%)

That's driven by [pyproject.toml:59](pyproject.toml:59) (packages = ["qcmanybody"], pre-existing, not touched by this PR) not excluding the tests subpackage. It's harmless today because nothing is published, but this PR is what turns on real PyPI publishing — once a tag ships, that wheel is permanent. Worth trimming (e.g. [tool.setuptools.packages.find] with exclude = ["qcmanybody.tests", "qcmanybody.tests.*"]) before the first v* tag goes out, so every future pip install qcmanybody doesn't ship quantum-chemistry reference data to end users.

Copilot's flagged "bug" is a false positive

The PR's automated review claims [release.yml:66](.github/workflows/release.yml:66) (version.local) will AttributeError because version is a string. I verified locally: packaging.utils.parse_wheel_filename/parse_sdist_filename already return Version objects, not strings — .local works fine. No change needed there.

Good patterns worth keeping for the rollout to other repos

  • id-token: write is scoped per-job, not workflow-wide; top-level permissions are contents: read — least privilege, no stored PyPI tokens.
  • if: github.repository == 'MolSSI/QCManyBody' guards every publish job — a fork building this same workflow won't accidentally publish to the real PyPI project. Copy this into the template.
  • The version-validation step (twine check + tag-vs-distribution version match + rejecting local versions) is a solid safety net — I confirmed it would hard-fail before publishing if a tag were malformed, which also covers a minor looseness in the tag-trigger glob ([0-9]* is glob, not regex, so it's more permissive than it looks — but any non-PEP440 tag gets caught here before publish, so it's not a real risk).
  • skip-existing: true on TestPyPI + gh release view idempotency check before creating a release both make reruns safe.

One thing to confirm outside the code

The PR's CI history shows TestPyPI has published successfully at least once, so that Trusted Publisher/Environment is presumably wired up. No tag has been pushed yet in this branch, so I'd do a dry-run tag (e.g. an rc tag) to confirm the pypi environment's trusted publisher is also configured on PyPI's project settings before relying on this for a real release.

Unrelated but bundled, both fine: the generalized_optimization.py fix corrects a genuine pre-existing SyntaxError (positional arg after keyword in warn()) that's currently broken on main — confirmed by testing the import; and the CI matrix cleanup (dropping the QCSk-next dev-channel job) looks like ordinary maintenance, already captured in the changelog under PR #50/#52.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants