Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 10 commits into from
Jul 6, 2023

Conversation

brocksam
Copy link
Contributor

@brocksam brocksam commented Jul 6, 2023

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

@brocksam
Copy link
Contributor Author

brocksam commented Jul 6, 2023

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
Copy link
Contributor

The strange thing is that the current workflow uses the checkout action to checkout a different repo:

- name: Checkout SymPy
uses: actions/checkout@v3
with:
repository: sympy/sympy
fetch-depth: 0

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
Copy link
Contributor Author

brocksam commented Jul 6, 2023

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
Copy link
Contributor

Thanks for the fix @brocksam !

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

@oscarbenjamin oscarbenjamin merged commit 49dea3d into sympy:master Jul 6, 2023
1 check passed
@oscarbenjamin
Copy link
Contributor

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

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