Skip to content

[maint] Test wheels to check that they work fine#53

Merged
charlesbmi merged 2 commits intomainfrom
maint/smoke-test-wheels
Dec 9, 2025
Merged

[maint] Test wheels to check that they work fine#53
charlesbmi merged 2 commits intomainfrom
maint/smoke-test-wheels

Conversation

@charlesbmi
Copy link
Copy Markdown
Collaborator

@charlesbmi charlesbmi commented Dec 9, 2025

#51

Introduction

Want to avoid publishing wheels that don't install well

Changes

  1. test wheels on build
  2. test wheels after they are published to PyPI
  3. requires workaround to depend on numpy, due to implicit dependency on numpy patrick-kidger/jaxtyping#360

Behavior

Should run on CI

Review checklist

  • All existing tests and checks pass
  • Unit tests covering the new feature or bugfix have been added
  • The documentation has been updated if necessary

@qodo-code-review
Copy link
Copy Markdown

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

51 - Partially compliant

Compliant requirements:

  • Ensure CI builds target Python 3.13 and succeed.
  • Verify the published wheel for Python 3.13 is functional (installs and imports).

Non-compliant requirements:

  • Provide a pre-compiled wheel for Python 3.13 along with other supported versions.

Requires further human verification:

  • Confirm that the CI artifacts actually include a Python 3.13 wheel for all target platforms beyond Linux and that it is uploaded to the GitHub Release/PyPI.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Possible Package Name Mismatch

The install step uses a filename pattern mach_beamform-*.whl while the PyPI smoke test references mach-beamform as the package name. Confirm the artifact naming and distribution name align to avoid test/install failures.

- name: Install wheel
  run: pip install wheelhouse/mach_beamform-*.whl

- name: Test import mach
  run: python -c 'import mach; print(mach.__version__)'
Matrix Coverage Limitation

Tests only run on ubuntu-22.04; if wheels are built for multiple platforms, consider adding at least one macOS and Windows smoke import to catch platform-specific issues.

test_wheels:
  name: Test wheel import
  needs: [build_wheels]
  runs-on: ubuntu-22.04
  strategy:
    matrix:
      python-version: ["3.9", "3.10", "3.11", "3.12", "3.13"]

  steps:
  - name: Set up Python
    uses: actions/setup-python@v5
    with:
      python-version: ${{ matrix.python-version }}

  - name: Download wheels
    uses: actions/download-artifact@v4
    with:
      name: dist-linux-wheels
      path: wheelhouse

  - name: Install wheel
    run: pip install wheelhouse/mach_beamform-*.whl

  - name: Test import mach
    run: python -c 'import mach; print(mach.__version__)'

upload_all:
  name: Upload if release
  needs: [test_wheels]
  runs-on: ubuntu-22.04

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review bot commented Dec 9, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
High-level
Post-release test does not prevent bad releases

The smoke_test job runs after publishing to PyPI, which means a failing test
indicates a broken package is already public. It is recommended to adopt a safer
strategy by publishing to TestPyPI first, running tests, and then publishing to
the official PyPI registry upon success.

Examples:

.github/workflows/wheels.yml [114-130]
  smoke_test:
    name: check PyPI release
    needs: [upload_all]
    runs-on: ubuntu-22.04
    strategy:
      matrix:
        python-version: ["3.9", "3.10", "3.11", "3.12", "3.13"]

    steps:
    - name: Install uv

 ... (clipped 7 lines)

Solution Walkthrough:

Before:

jobs:
  test_wheels:
    needs: [build_wheels]
    # ... tests local wheel files ...

  upload_all:
    needs: [test_wheels]
    if: is_release
    steps:
      - name: Publish to PyPI
        run: uv publish

  smoke_test:
    needs: [upload_all]
    # ... tests by installing from the live PyPI registry ...

After:

jobs:
  # ... test_wheels job ...

  publish_to_testpypi:
    needs: [test_wheels]
    if: is_release
    steps:
      - name: Publish to TestPyPI
        # ... configuration to publish to test.pypi.org ...

  smoke_test_from_testpypi:
    needs: [publish_to_testpypi]
    # ... tests by installing from TestPyPI ...

  publish_to_pypi:
    needs: [smoke_test_from_testpypi]
    if: is_release
    steps:
      - name: Publish to PyPI
        run: uv publish
Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a significant flaw in the release process where the smoke_test job runs after publishing, meaning a broken package could be released, and proposes a much safer industry-standard deployment strategy.

High
Possible issue
Fix wheel installation by selecting version
Suggestion Impact:The pip install command was changed to compute the matrix Python version without dots and install the matching cp-tagged wheel, preventing incompatible wheel installs.

code diff:

+      run: |
+        PYTHON_VERSION_NO_DOT=$(echo "${{ matrix.python-version }}" | tr -d '.')
+        pip install wheelhouse/mach_beamform-*-cp${PYTHON_VERSION_NO_DOT}-*.whl

Modify the pip install command to select the correct wheel file corresponding to
the Python version in the test matrix, preventing installation failures from
incompatible wheels.

.github/workflows/wheels.yml [74-75]

 - name: Install wheel
-  run: pip install wheelhouse/mach_beamform-*.whl
+  run: |
+    PY_VERSION_NODOT=$(echo "${{ matrix.python-version }}" | tr -d '.')
+    pip install wheelhouse/mach_beamform-*-cp${PY_VERSION_NODOT}-*.whl

[Suggestion processed]

Suggestion importance[1-10]: 9

__

Why: The suggestion correctly identifies a critical bug in the test_wheels job where using a broad glob * would cause pip to fail by trying to install wheels for all Python versions, and the proposed fix correctly filters for the specific wheel.

High
  • Update

@charlesbmi charlesbmi force-pushed the maint/smoke-test-wheels branch from 627ead9 to d37174f Compare December 9, 2025 07:06
@charlesbmi charlesbmi merged commit cf1ce78 into main Dec 9, 2025
9 checks passed
@charlesbmi charlesbmi deleted the maint/smoke-test-wheels branch December 9, 2025 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant