sympy / sympy_benchmarks

Some benchmarks of SymPy
14 stars 32 forks source link

Update GitHub Actions workflow to run PR benchmark changes in CI #92

Closed brocksam closed 1 year ago

brocksam commented 1 year ago

This PR addresses the issue identified in PR #91 whereby benchmarks changed in a PR are not run in CI for that PR.

brocksam commented 1 year ago

I'm clearly doing something stupid with Git here but can't work out what. I'm doing git fetch upstream 1.12 with upstream being the Sympy repo, so we should have access to the 1.12 branch. asv is also now configured to target "upstream/1.12", but the call to asv run in the CI job is erroring with Unknown branch upstream/1.12 in configuration. Pretty stumped here, any input/troubleshooting ideas from others (@oscarbenjamin) would be greatly appreciated.

oscarbenjamin commented 1 year ago

The strange thing is that the current workflow uses the checkout action to checkout a different repo: https://github.com/sympy/sympy_benchmarks/blob/5563b6dd9266d7d9bc87f2c306cc68d7d566a8d4/.github/workflows/run-benchmarks.yml#L13-L17 That effectively does git clone sympy && cd sympy. The change in this PR is to then use the checkout action again with

      - name: Checkout SymPy Benchmarks
        uses: actions/checkout@v3
        with:
          sparse-checkout: |
            asv.conf.actions.json
            benchmarks

I'm not really sure what the effect of this is but it seems like it is trying to checkout one repo while already inside another.

I think that the problem here is using the checkout action to checkout the sympy repo in the first place. That should just checkout the PR branch of the sympy_benchmarks repo as is standard for CI that runs on a PR. Then there can be a separate step to checkout sympy which can just use git clone rather than the checkout action.

brocksam commented 1 year ago

Thanks for the insight, @oscarbenjamin. These changes have certainly cleaned up what's going on and make a lot more sense. asv is actually being used in its intended way.

oscarbenjamin commented 1 year ago

Thanks for the fix @brocksam !

I guess the only way to know it has worked is to merge it.

oscarbenjamin commented 1 year ago

We can check gh-89 to see if it is fixed...