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

Add performance benchmarking scripts and run in CI #1978

Closed
wants to merge 49 commits into from

Conversation

EwoutH
Copy link
Member

@EwoutH EwoutH commented Jan 19, 2024

Let's see how many attempts this is going to take

General usage (once this is merged to main):

  1. Go to the main branch.
  2. Run global_benchmarks.py
  3. Go to your development branch
  4. Run global_benchmarks.py again
  5. Run compare_timings.py

It should output a table like this:

Model Size Init time [95% CI] Run time [95% CI]
SchellingModel small 🔵 +5.7% [+1.7%, +10.8%] 🔵 +5.8% [+1.1%, +12.0%]
SchellingModel large 🔵 -1.0% [-2.1%, -0.2%] 🔵 -0.5% [-2.1%, +1.2%]
WolfSheep small 🔵 +0.0% [-0.6%, +0.6%] 🔵 -0.0% [-0.5%, +0.4%]
WolfSheep large 🔴 +211.2% [+172.4%, +255.0%] 🔵 +4.6% [-8.0%, +22.4%]
BoidFlockers small 🔵 +0.1% [-9.7%, +11.6%] 🔵 +3.1% [-0.7%, +7.4%]
BoidFlockers large 🔵 -11.9% [-25.2%, -1.8%] 🟢 -18.2% [-31.6%, -4.7%]

Positive indicates increate in runtime, negative a decrease.

Few less replications, some more seeds. Every benchmark now takes between 10 and 20 seconds (on my machine).
That allows switching branches without benchmarks results disappearing
Prints some stuff when running and saves a pickle file after running.
- The bootstrap_speedup_confidence_interval function calculates the mean speedup and its confidence interval using bootstrapping, which is more suitable for paired data.
- The mean speedup and confidence interval are calculated for both initialization and run times.
- Positive values indicate an increase in time (longer duration), and negative values indicate a decrease (shorter duration).
- The results are displayed in a DataFrame with the percentage changes and their confidence intervals.
@EwoutH
Copy link
Member Author

EwoutH commented Jan 19, 2024

/rerun-benchmarks

@EwoutH
Copy link
Member Author

EwoutH commented Jan 19, 2024

/rerun-benchmarks

Edit: https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#issue_comment

Note: This event will only trigger a workflow run if the workflow file is on the default branch.

@EwoutH EwoutH marked this pull request as ready for review January 19, 2024 17:54
@EwoutH EwoutH marked this pull request as draft January 19, 2024 17:56
@EwoutH EwoutH marked this pull request as ready for review January 19, 2024 17:56
@EwoutH EwoutH marked this pull request as draft January 19, 2024 17:58
@EwoutH EwoutH marked this pull request as ready for review January 19, 2024 17:58
@EwoutH EwoutH marked this pull request as draft January 19, 2024 18:01
@EwoutH EwoutH marked this pull request as ready for review January 19, 2024 18:01
@EwoutH EwoutH marked this pull request as draft January 19, 2024 18:05
@EwoutH EwoutH marked this pull request as ready for review January 19, 2024 18:05
@EwoutH
Copy link
Member Author

EwoutH commented Jan 19, 2024

(I'm truly sorry if someone set email notifications for "ready for review")

@Corvince
Copy link
Contributor

I think there is a vscode extension that lets you run GitHub actions locally, maybe give it a try?

@Corvince
Copy link
Contributor

(I'm truly sorry if someone set email notifications for "ready for review")

I don't, but why are you doing this? To trigger GA actions?

@EwoutH
Copy link
Member Author

EwoutH commented Jan 19, 2024

From my reading GITHUB_TOKEN that you seem to use should have that permission. Are you sure that's the problem?

Unfortunately, I'm 90% sure. The error:

Error: Unhandled error: HttpError: Resource not accessible by integration

implies
The error along with a 403 status code indicates a permissions issue. It most likely means that the GitHub token being used does not have the necessary permissions to create a comment on the issue or pull request.

@EwoutH
Copy link
Member Author

EwoutH commented Jan 19, 2024

Okay, there are a few minor things, like Black and ruff failing because the added benchmark models don't adhere to their standards, but the benchmark scripts are further ready. General usage (once this is merged to main):

  1. Go to the main branch.
  2. Run global_benchmarks.py
  3. Go to your development branch
  4. Run global_benchmarks.py again
  5. Run compare_timings.py

It should output a table like this:

Model Size Init time [95% CI] Run time [95% CI]
SchellingModel small 🔵 +5.7% [+1.7%, +10.8%] 🔵 +5.8% [+1.1%, +12.0%]
SchellingModel large 🔵 -1.0% [-2.1%, -0.2%] 🔵 -0.5% [-2.1%, +1.2%]
WolfSheep small 🔵 +0.0% [-0.6%, +0.6%] 🔵 -0.0% [-0.5%, +0.4%]
WolfSheep large 🔴 +211.2% [+172.4%, +255.0%] 🔵 +4.6% [-8.0%, +22.4%]
BoidFlockers small 🔵 +0.1% [-9.7%, +11.6%] 🔵 +3.1% [-0.7%, +7.4%]
BoidFlockers large 🔵 -11.9% [-25.2%, -1.8%] 🟢 -18.2% [-31.6%, -4.7%]

Positive indicates increate in runtime, negative a decrease. I will add fancy emojis tomorrow to make it really clear if it's good, bad or insignificant.

@Corvince
Copy link
Contributor

I think you can set the permission inside the workflow file similar to this

https://github.com/projectmesa/mesa/blob/main/.github%2Fworkflows%2Frelease.yml#L18-L20

@EwoutH
Copy link
Member Author

EwoutH commented Jan 19, 2024

According to ChatGPT it's something different:

Adding the id-token: write permission in your GitHub Actions workflow will not resolve the issue you're experiencing with the "Resource not accessible by integration" error. The id-token: write permission is used to request an OIDC token for use with other services that understand OpenID Connect (OIDC) tokens, and it is unrelated to the permissions needed to comment on issues or pull requests in GitHub.

@Corvince
Copy link
Contributor

That was just meant as an example. I would guess you need pull-requests: write

@Corvince
Copy link
Contributor

You never knows what helps
@EwoutH
Copy link
Member Author

EwoutH commented Jan 19, 2024

@Corvince Thanks for your insights. They're really useful.

I think I zoomed further in to the issue: https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

Check the column "Maximum access for pull requests from public forked repositories". Everything is read, even if you define it explicitly.

I'm pulling this PR in from my own fork EwoutH/mesa. Many contributors will do that, so this workflow has to work from public forks. But I understand very clearly why it's disabled by default, otherwise anyone can open any PR and do anything.

So the solution stays the same as suggested here, we need to make a new very-specific, but permissive token with "pull-requests: write" access that only runs on this workflow and only is allowed to make pull request comments.

@EwoutH EwoutH marked this pull request as ready for review January 19, 2024 21:15
@EwoutH EwoutH marked this pull request as draft January 19, 2024 21:21
@EwoutH
Copy link
Member Author

EwoutH commented Jan 19, 2024

Weird stuff. Can't figure it out without access to tokens and secrets unfortunately. Will wait until I have the permissions.

@EwoutH EwoutH changed the title Add performance benchmarking scripts Add performance benchmarking scripts and run in CI Jan 19, 2024
@quaquel
Copy link
Member

quaquel commented Jan 20, 2024

@EwoutH Do you mind if I commit to this branch with various minor updates?

  • fix for < 3.12
  • insertion of mesa into sys.path
  • ruff related fixes

@EwoutH
Copy link
Member Author

EwoutH commented Jan 20, 2024

Go ahead, and thanks for asking.

With #1979 merged I just wanted to rebase it on that PR, shall I do that first?

@EwoutH
Copy link
Member Author

EwoutH commented Jan 20, 2024

@quaquel with #1979 merged it actually easier to just open a new branch (and PR) from that. On this branch everything is outdated except the CI stuff.

@EwoutH
Copy link
Member Author

EwoutH commented Jan 20, 2024

I'm going to do a bit of weekend, I will take a looks at the CI tomorrow or Monday again.

(@jackiekazil or @tpike3 I would greatly appreciate if I have the permissions by then)

@quaquel
Copy link
Member

quaquel commented Jan 20, 2024

I see a major performance difference between this branch and master (factor 5 or so). I want to dig in and see which commit explains this difference. Will keep you posted.

@EwoutH
Copy link
Member Author

EwoutH commented Jan 20, 2024

In runtime? Quite sure it’s this one bc61345, for this branch I set the replications to 1 to iterate faster.

@jackiekazil
Copy link
Member

Should this one be closed in lieu of the other PR?

@EwoutH
Copy link
Member Author

EwoutH commented Jan 21, 2024

Closing this, #1979 and #1983 have replaced this PR.

@EwoutH EwoutH closed this Jan 21, 2024
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.

5 participants