diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..9b15406 --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,9 @@ +# The release workflow pins its actions to commit SHAs so that a moved tag in +# someone else's repository cannot run new code in the job that holds the PyPI +# OIDC identity. Dependabot is what keeps those pins from going stale. +version: 2 +updates: + - package-ecosystem: "github-actions" + directory: "/" + schedule: + interval: "monthly" diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 0000000..66fe86e --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,145 @@ +name: Release + +# Builds and validates the distributions on every change that can affect +# packaging, and publishes to PyPI when a v* tag is pushed. Publishing uses +# PyPI Trusted Publishing (OIDC), so there is no API token stored in this repo. +# See RELEASING.md for the one-time PyPI-side setup and the release steps. +# +# Third-party actions are pinned to a commit SHA rather than a tag or branch: +# the publish job holds an OIDC identity that PyPI trusts to upload as `dion`, +# so a moved tag in someone else's repository must not be able to run new code +# inside it. .github/dependabot.yml keeps the pins current. + +on: + push: + tags: + - "v*" + pull_request: + paths: + - "setup.py" + - "pyproject.toml" + - "MANIFEST.in" + - "requirements_*.txt" + # long_description, and the files MANIFEST.in ships. + - "README.md" + - "LICENSE" + - "NOTICE.md" + - "CHANGELOG.md" + # find_packages() decides what lands in the wheel, so the package layout + # is packaging input too -- a new subdirectory without an __init__.py is + # silently dropped from the distribution. + - "dion/**" + - "tests/test_packaging.py" + - ".github/workflows/release.yml" + workflow_dispatch: + +permissions: + contents: read + +jobs: + build: + name: Build and validate distributions + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@11d5960a326750d5838078e36cf38b85af677262 # v4.4.0 + with: + persist-credentials: false + + - uses: actions/setup-python@a26af69be951a213d495a4c3e4e4022e16d87065 # v5.6.0 + with: + python-version: "3.12" + + - name: Install build tooling + run: python -m pip install --upgrade build twine pytest packaging + + - name: Check packaging metadata + run: python -m pytest tests/test_packaging.py -v + + # The bare `python -m build` is load-bearing: with no flags it builds the + # sdist from the source tree and then the wheel *from that sdist*, which + # is the round trip that exposes an sdist missing its requirements files. + # `python -m build --sdist --wheel` builds both from the source tree + # instead and would leave the wheel checks below passing on a broken sdist. + - name: Build sdist and wheel + run: python -m build + + - name: Check distribution metadata + run: python -m twine check --strict dist/* + + # twine check only renders the long description. These re-run the + # packaging tests against the actual artifacts, which is what catches an + # sdist that dropped its requirements files. + - name: Check built artifacts + env: + DION_DIST_DIR: dist + run: python -m pytest tests/test_packaging.py -v + + # Checks the version baked into the built filenames -- the one that will + # actually be uploaded -- rather than re-reading setup.py, so the gate + # cannot drift from what the publish job pushes to PyPI. + - name: Verify the tag matches the built distributions + if: startsWith(github.ref, 'refs/tags/v') + env: + TAG_NAME: ${{ github.ref_name }} + run: | + python - <<'PY' + import os + import pathlib + from packaging.utils import parse_sdist_filename, parse_wheel_filename + from packaging.version import InvalidVersion, Version + + + def fail(message): + print(f"::error::{message}") + raise SystemExit(1) + + + def only(pattern): + matches = sorted(pathlib.Path("dist").glob(pattern)) + if len(matches) != 1: + fail(f"expected exactly one {pattern} in dist/, found {[p.name for p in matches]}") + return matches[0] + + + built = { + parse_wheel_filename(only("*.whl").name)[1], + parse_sdist_filename(only("*.tar.gz").name)[1], + } + if len(built) != 1: + fail(f"sdist and wheel disagree on the version: {sorted(str(v) for v in built)}") + packaged = built.pop() + + tag = os.environ["TAG_NAME"] + try: + tagged = Version(tag[1:]) + except InvalidVersion: + fail(f"tag {tag} is not a PEP 440 version, so it cannot name a release") + if tagged != packaged: + fail(f"tag {tag} would publish version {packaged}; bump setup.py or retag") + print(f"tag {tag} matches the built version {packaged}") + PY + + - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + with: + name: dist + path: dist/ + + publish: + name: Publish to PyPI + needs: build + if: startsWith(github.ref, 'refs/tags/v') + runs-on: ubuntu-latest + # The environment gates the OIDC identity that PyPI trusts, and is where a + # manual approval can be required before anything reaches PyPI. + environment: + name: pypi + url: https://pypi.org/p/dion + permissions: + id-token: write + steps: + - uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0 + with: + name: dist + path: dist/ + + - uses: pypa/gh-action-pypi-publish@dc37677b2e1c63e2034f94d8a5b11f265b73ba33 # v1.14.2 diff --git a/.gitignore b/.gitignore index 34d7e8d..eaef29e 100644 --- a/.gitignore +++ b/.gitignore @@ -8,4 +8,6 @@ output/ .venv/ submit.ipynb aztool/ -dion.egg-info/ \ No newline at end of file +dion.egg-info/ +dist/ +build/ diff --git a/MANIFEST.in b/MANIFEST.in new file mode 100644 index 0000000..38640b4 --- /dev/null +++ b/MANIFEST.in @@ -0,0 +1,11 @@ +# setup.py reads the requirements files at build time to populate +# install_requires and extras_require. Without them in the sdist, +# `read_requirements` finds nothing, warns, and returns an empty list -- so +# building from the sdist yields a wheel that declares no dependencies at all. +# The glob covers requirements files added later, which an explicit list would +# not. See tests/test_packaging.py. +include requirements_*.txt +include LICENSE +include NOTICE.md +include README.md +include CHANGELOG.md diff --git a/RELEASING.md b/RELEASING.md new file mode 100644 index 0000000..f2e5c2a --- /dev/null +++ b/RELEASING.md @@ -0,0 +1,90 @@ +# Releasing Dion + +Dion publishes to [PyPI](https://pypi.org/p/dion) from +`.github/workflows/release.yml` when a `v*` tag is pushed. Publishing uses PyPI +[Trusted Publishing](https://docs.pypi.org/trusted-publishers/), so no API token +is stored in this repository. + +## One-time setup + +All three steps need a human with the right accounts; none can be done from a PR. + +**1. Enable GitHub Actions for workflows in this repository.** As of this +writing no workflow defined in `.github/` has ever run here — the only entries +under *Actions* are the org's managed ones (CodeQL default setup, Copilot +review, Dependabot), and `release.yml` did not trigger on the pull request that +added it. Reading the setting needs admin, so check *Settings → Actions → +General* and confirm that actions are allowed and that `actions/*` and +`pypa/gh-action-pypi-publish` are permitted by the allow-list, if one is in +force. Without this neither half of the workflow runs: no pre-tag validation, +and no publish. + +**2. Register the pending publisher on PyPI.** The `dion` name is unclaimed, so +the first upload creates the project. Log in to pypi.org with the account that +should own it, go to *Your projects → Publishing → Add a new pending publisher*, +and enter: + +| Field | Value | +| --- | --- | +| PyPI Project Name | `dion` | +| Owner | `microsoft` | +| Repository name | `dion` | +| Workflow name | `release.yml` | +| Environment name | `pypi` | + +A pending publisher is what lets the workflow create a project that does not +exist yet. Once the first release lands it becomes an ordinary trusted publisher. + +**3. Create the `pypi` environment.** In *Settings → Environments*, add an +environment named `pypi`. The name must match the workflow and the pending +publisher exactly. Adding required reviewers here puts a human approval in front +of every upload, which is worth doing on a public package. + +## Cutting a release + +1. Bump `version` in `setup.py`. Versions below `1.0` are pre-release: breaking + changes are allowed, but a released version number can never be reused. +2. Move the `[Unreleased]` entries in `CHANGELOG.md` under a new + `## [X.Y.Z] - YYYY-MM-DD` heading. +3. Open a PR with both changes and merge it. The `Release` workflow builds and + validates the distributions on that PR, so packaging breakage surfaces before + the tag exists. +4. Tag the merge commit and push: + + ```bash + git tag vX.Y.Z && git push origin vX.Y.Z + ``` + +5. The workflow rebuilds, verifies the tag matches the version baked into the + built distributions, and publishes. If the `pypi` environment requires + reviewers, approve the run. + +## Notes + +- **A version is permanent.** PyPI does not allow reuse of a version number or + of a distribution filename, even after deletion. A bad release is yanked and + superseded, never replaced. Test on TestPyPI first if a release is unusual: + configure a second pending publisher at test.pypi.org and run the workflow + against it, or upload once by hand with + `twine upload -r testpypi dist/*`. +- **`twine check` is not a metadata check.** It renders the long description and + little else. The fields PyPI actually validates on upload — `author_email` + above all — are covered by `tests/test_packaging.py`, which runs in CI. +- **The sdist has to carry `requirements_*.txt`.** `setup.py` reads them at build + time to populate `install_requires`; if `MANIFEST.in` stops shipping them, a + build from the sdist still succeeds but declares no dependencies at all. + `tests/test_packaging.py` asserts against the built artifacts to catch this. +- **`python -m build` is spelled without flags on purpose.** With no arguments it + builds the sdist from the source tree and then the wheel *from that sdist*, + which is the round trip that exposes the failure above. `python -m build + --sdist --wheel` builds both from the source tree and would leave the wheel + assertions passing over a broken sdist. +- **The workflow pins its actions to commit SHAs.** The publish job holds an + OIDC identity that PyPI trusts to upload as `dion`, so it must not run code + fetched from a mutable tag or branch in someone else's repository — + `pypa/gh-action-pypi-publish@release/v1` is a branch. `.github/dependabot.yml` + raises PRs to move the pins forward; the trailing `# vX.Y.Z` comment on each + is what it reads to know the current version. +- **A tag is enough to publish.** Anyone who can push a `v*` tag — or run the + workflow manually against one — reaches the upload step. Required reviewers on + the `pypi` environment are what stands between that and PyPI. diff --git a/pyproject.toml b/pyproject.toml new file mode 100644 index 0000000..36ea2cc --- /dev/null +++ b/pyproject.toml @@ -0,0 +1,6 @@ +# Package metadata lives in setup.py. This file exists so that builds go through +# PEP 517 (`python -m build`, `pip install .`) with a declared, isolated build +# environment, rather than the deprecated `python setup.py bdist_wheel` path. +[build-system] +requires = ["setuptools>=64", "wheel"] +build-backend = "setuptools.build_meta" diff --git a/setup.py b/setup.py index 7ff2618..cb91ec2 100644 --- a/setup.py +++ b/setup.py @@ -56,15 +56,34 @@ def read_requirements(path): # }, # Author information: author="Ahn, Kwangjun and Xu, Byron and Abreu, Natalie and Langford, John", # as listed in the paper - author_email="{kwangjunahn, byronxu}@microsoft.com", # left this form to prevent bots from harvesting emails + # PyPI validates this field and rejects an unparseable address, so it has to be a + # real mailbox. Bug reports belong on the issue tracker (see project_urls) rather + # than in a maintainer's inbox. + author_email="jcl@microsoft.com", # Description of the package: description="Dion: Distributed Orthonormal Updates", long_description=readme_contents, long_description_content_type="text/markdown", + project_urls={ + "Homepage": "https://github.com/microsoft/dion", + "Source": "https://github.com/microsoft/dion", + "Issues": "https://github.com/microsoft/dion/issues", + "Changelog": "https://github.com/microsoft/dion/blob/main/CHANGELOG.md", + }, # Plugins entry point classifiers=[ + "Development Status :: 4 - Beta", + "Intended Audience :: Science/Research", + "License :: OSI Approved :: MIT License", + "Operating System :: OS Independent", "Programming Language :: Python", "Programming Language :: Python :: 3", + "Programming Language :: Python :: 3.9", + "Programming Language :: Python :: 3.10", + "Programming Language :: Python :: 3.11", + "Programming Language :: Python :: 3.12", + "Programming Language :: Python :: 3.13", + "Topic :: Scientific/Engineering :: Artificial Intelligence", ], python_requires=">=3.9", license="MIT", diff --git a/tests/test_packaging.py b/tests/test_packaging.py new file mode 100644 index 0000000..bd04499 --- /dev/null +++ b/tests/test_packaging.py @@ -0,0 +1,182 @@ +"""Tests for the packaging metadata that PyPI validates at upload time. + +These guard failure modes that every local check passes: + +- PyPI rejects an unparseable ``author_email``, but ``twine check`` only + renders the long description and never looks at the field. The address + shipped here for the package's first year (``{user1, user2}@microsoft.com``, + obfuscated against harvesters) builds and passes ``twine check`` cleanly and + fails only against the upload API. +- ``setup.py`` reads ``requirements_*.txt`` at build time. If those files are + absent from the sdist, ``read_requirements`` warns and returns ``[]``, so + building from the sdist succeeds and produces a wheel declaring *no* + dependencies. ``MANIFEST.in`` is what keeps them in. +- ``find_packages`` only picks up directories that have an ``__init__.py``, so + a new subpackage without one is dropped from the distribution silently. + +Metadata is read out of ``setup.py`` with ``ast`` so the tests need neither a +build step nor torch. The artifact tests run only when ``DION_DIST_DIR`` points +at a built ``dist/`` directory; CI sets it after ``python -m build``. That bare +``python -m build`` builds the wheel *from the sdist*, so the wheel assertions +below also cover what the sdist carries. +""" + +import ast +import fnmatch +import os +import tarfile +import zipfile +from email.utils import parseaddr +from pathlib import Path + +import pytest +from packaging.specifiers import SpecifierSet +from packaging.version import Version + +REPO_ROOT = Path(__file__).resolve().parent.parent +SETUP_PY = REPO_ROOT / "setup.py" +MANIFEST_IN = REPO_ROOT / "MANIFEST.in" +PACKAGE_DIR = REPO_ROOT / "dion" +REQUIREMENTS_FILES = sorted(p.name for p in REPO_ROOT.glob("requirements_*.txt")) + + +def _setup_call(): + tree = ast.parse(SETUP_PY.read_text()) + for node in ast.walk(tree): + if isinstance(node, ast.Call) and getattr(node.func, "id", None) == "setup": + return {kw.arg: kw.value for kw in node.keywords} + raise AssertionError("no setup() call found in setup.py") + + +def _module_assignment(name): + tree = ast.parse(SETUP_PY.read_text()) + for node in tree.body: + if isinstance(node, ast.Assign): + for target in node.targets: + if isinstance(target, ast.Name) and target.id == name: + return ast.literal_eval(node.value) + raise AssertionError(f"no module-level assignment to {name} in setup.py") + + +def _keyword(name): + node = _setup_call().get(name) + if node is None: + raise AssertionError(f"setup() has no {name} keyword") + return ast.literal_eval(node) + + +def _manifest_include_patterns(): + patterns = [] + for line in MANIFEST_IN.read_text().splitlines(): + line = line.strip() + if line.startswith("include "): + patterns.extend(line.split()[1:]) + return patterns + + +def test_author_email_is_a_deliverable_address(): + """PyPI's upload API rejects an address that does not parse.""" + email = _keyword("author_email") + name, addr = parseaddr(email) + assert addr, f"author_email {email!r} does not parse as an email address" + assert "@" in addr and not addr.startswith("@") and not addr.endswith("@") + assert "," not in addr and "{" not in addr and "}" not in addr + + +def test_version_is_pep440(): + """A tag-driven release publishes this string; PyPI requires PEP 440.""" + Version(_module_assignment("version")) + + +def test_project_urls_point_at_the_repository(): + """Without these the PyPI page has no link back to the source.""" + urls = _keyword("project_urls") + assert "Issues" in urls, "no issue tracker link; bug reports land in a maintainer inbox" + for label, url in urls.items(): + assert url.startswith("https://github.com/microsoft/dion"), ( + f"project_urls[{label!r}] does not point at the repository: {url}" + ) + + +def test_classifiers_declare_the_license(): + """setup.py's license="MIT" is free text; the classifier is what PyPI facets on.""" + classifiers = _keyword("classifiers") + assert "License :: OSI Approved :: MIT License" in classifiers + + +def test_python_classifiers_agree_with_python_requires(): + """A classifier PyPI advertises but python_requires excludes is a false claim.""" + supported = SpecifierSet(_keyword("python_requires")) + declared = [ + c.rsplit(" :: ", 1)[1] + for c in _keyword("classifiers") + if c.startswith("Programming Language :: Python :: ") + ] + versions = [v for v in declared if "." in v] + assert versions, "no Programming Language :: Python :: X.Y classifiers to check" + excluded = [v for v in versions if not supported.contains(v)] + assert not excluded, ( + f"classifiers advertise Python {excluded} but python_requires " + f"{str(supported)!r} rejects them" + ) + + +def test_repo_has_requirements_files(): + """The manifest and sdist tests below are parametrized on this glob.""" + assert REQUIREMENTS_FILES, "no requirements_*.txt in the repo root; the tests below are vacuous" + + +@pytest.mark.parametrize("requirements_file", REQUIREMENTS_FILES) +def test_manifest_ships_every_requirements_file(requirements_file): + """Any requirements file setup.py may read has to survive into the sdist.""" + patterns = _manifest_include_patterns() + assert any(fnmatch.fnmatch(requirements_file, pattern) for pattern in patterns), ( + f"{requirements_file} matches no include line in MANIFEST.in, so an sdist build " + f"would silently drop the dependencies it declares" + ) + + +def _dist_dir(): + dist = os.environ.get("DION_DIST_DIR") + if not dist: + pytest.skip("DION_DIST_DIR not set; run `python -m build` first") + return Path(dist) + + +def _one(pattern): + matches = sorted(_dist_dir().glob(pattern)) + assert len(matches) == 1, f"expected exactly one {pattern} in dist/, found {matches}" + return matches[0] + + +def test_sdist_contains_the_requirements_files(): + with tarfile.open(_one("*.tar.gz")) as tar: + names = {Path(n).name for n in tar.getnames()} + missing = [name for name in REQUIREMENTS_FILES if name not in names] + assert not missing, f"sdist is missing {missing}; a build from it would declare no deps" + + +def test_wheel_declares_its_runtime_dependencies(): + with zipfile.ZipFile(_one("*.whl")) as wheel: + metadata_name = next(n for n in wheel.namelist() if n.endswith(".dist-info/METADATA")) + metadata = wheel.read(metadata_name).decode() + requires = [ + line.split(":", 1)[1].strip() + for line in metadata.splitlines() + if line.startswith("Requires-Dist:") and "extra ==" not in line + ] + assert any(r.startswith("torch") for r in requires), f"wheel declares no torch: {requires}" + assert any(r.startswith("numpy") for r in requires), f"wheel declares no numpy: {requires}" + + +def test_wheel_ships_every_package_module(): + """find_packages() drops a subdirectory that has no __init__.py, without complaint.""" + source = {p.relative_to(REPO_ROOT).as_posix() for p in PACKAGE_DIR.rglob("*.py")} + assert source, f"no modules found under {PACKAGE_DIR}" + with zipfile.ZipFile(_one("*.whl")) as wheel: + shipped = set(wheel.namelist()) + missing = sorted(source - shipped) + assert not missing, ( + f'the wheel is missing {missing}; find_packages(include=["dion", "dion.*"]) ' + f"only picks up directories with an __init__.py" + )