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.

Sign up for GitHub

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

Jump to bottom

Add CI workflow for performance benchmarks #1983

Merged
merged 2 commits into from
Jan 21, 2024
Merged

Add CI workflow for performance benchmarks #1983

merged 2 commits into from
Jan 21, 2024

Conversation

EwoutH
Copy link
Member

@EwoutH EwoutH commented Jan 20, 2024

This PR introduces a new GitHub Actions workflow for performance benchmarking. The workflow is triggered on pull_request_target events and can be triggered with a "/rerun-benchmarks" comment in the PR. It should be compatible with PRs from both forks and the main repository. It includes steps for setting up the environment, checking out the PR and main branches, installing dependencies, and running benchmarks on both branches. The results are then compared, encoded, and posted as a comment on the PR.

It's largely based and supersedes #1978 and is reasonably well tested on my own fork in EwoutH#22. It shouldn't need any additional permissions.

The results is a comment on a PR like this:

Screenshot_349

This PR will start working when it's merged onto the main branch.

@EwoutH EwoutH added the ci Release notes label label Jan 20, 2024
@EwoutH EwoutH requested a review from Corvince January 20, 2024 21:38
@EwoutH
Add CI workflow for performance benchmarks
8b1299d 
This commit introduces a new GitHub Actions workflow for performance benchmarking. The workflow is triggered on `pull_request_target` events and can be triggered with a "/rerun-benchmarks" comment in the PR. It should be compatible with PRs from both forks and the main repository. It includes steps for setting up the environment, checking out the PR and main branches, installing dependencies, and running benchmarks on both branches. The results are then compared, encoded, and posted as a comment on the PR.
@rht
Copy link
Contributor

rht commented Jan 20, 2024

I might have missed some detail, but how did you fix the permission problem, if you could summarize? This is for my learning purpose.

@jackiekazil
Copy link
Member

Catching up - this looks like a great addition. Since I have been tracking less -- i am going to defer to others for the review and merge. Let me know if you need anything.

Corvince
Corvince reviewed Jan 21, 2024
View reviewed changes
Copy link
Contributor

@Corvince Corvince left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So happy you got it working! Added two comments

.github/workflows/benchmarks.yml Outdated Show resolved Hide resolved
.github/workflows/benchmarks.yml Show resolved Hide resolved
@EwoutH
Copy link
Member Author

EwoutH commented Jan 21, 2024

I might have missed some detail, but how did you fix the permission problem, if you could summarize? This is for my learning purpose.

Using the pull_request_target. That does have write permissions, but only initiated from the main branch of a repo. So it only takes effect once it's merged into main, protecting it from attacks on CI infrastructure.

@EwoutH EwoutH merged commit c0de4a1 into projectmesa:main Jan 21, 2024
13 checks passed
@EwoutH
Copy link
Member Author

EwoutH commented Jan 21, 2024 •
edited
Loading

So let's see if this works: /rerun-benchmarks

@github-actions GitHub Actions
Copy link

Performance benchmarks:

Model Size Init time [95% CI] Run time [95% CI]
SchellingModel small 🔵 -1.2% [-1.6%, -0.7%] 🔵 +0.7% [+0.1%, +1.3%]
SchellingModel large 🔵 -8.7% [-21.2%, -2.3%] 🔵 -0.4% [-1.6%, +0.8%]
WolfSheep small 🔵 -1.1% [-1.5%, -0.6%] 🔵 +0.4% [+0.3%, +0.5%]
WolfSheep large 🔵 +12.3% [-0.7%, +30.7%] 🔵 -2.3% [-3.9%, -0.6%]
BoidFlockers small 🟢 -3.4% [-3.9%, -3.0%] 🔵 -0.5% [-1.1%, +0.1%]
BoidFlockers large 🔵 -3.0% [-3.5%, -2.4%] 🔵 -1.0% [-1.5%, -0.5%]

@EwoutH
Copy link
Member Author

EwoutH commented Jan 21, 2024

It works! Still a bit noisy, but enough to get a general picture.

For future improvements we could:

  • Ensure seeds are correctly working
  • Run on a own, custom runner without hyperthreading, clock speed turbo boosting and conflicting tasks
  • Optimize configurations, replications and statistical methods
  • Add models that test relevant components

@EwoutH EwoutH mentioned this pull request Jan 21, 2024
Closed
quaquel added a commit to quaquel/mesa that referenced this pull request Jan 21, 2024
@quaquel
Squashed commit of the following:
9485087 
commit c0de4a1
Author: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>
Date:   Sun Jan 21 14:43:08 2024 +0100

    Add CI workflow for performance benchmarks (projectmesa#1983)

    This commit introduces a new GitHub Actions workflow for performance benchmarking. The workflow is triggered on `pull_request_target` events and can be triggered with a "/rerun-benchmarks" comment in the PR. It should be compatible with PRs from both forks and the main repository. It includes steps for setting up the environment, checking out the PR and main branches, installing dependencies, and running benchmarks on both branches. The results are then compared, encoded, and posted as a comment on the PR.
rht pushed a commit that referenced this pull request Jan 21, 2024
@quaquel @pre-commit-ci
Minor edits to benchmarking code (#1985)
4d0eeec 
* Make RandomActivationByType.agents_by_type backward compatible

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* updated warning

* Update .gitignore

* Update global_benchmark.py

* pep8 rename

* Update schelling.py

* Update schelling.py

* Update schelling.py

* Update schelling.py

* Squashed commit of the following:

commit c0de4a1
Author: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>
Date:   Sun Jan 21 14:43:08 2024 +0100

    Add CI workflow for performance benchmarks (#1983)

    This commit introduces a new GitHub Actions workflow for performance benchmarking. The workflow is triggered on `pull_request_target` events and can be triggered with a "/rerun-benchmarks" comment in the PR. It should be compatible with PRs from both forks and the main repository. It includes steps for setting up the environment, checking out the PR and main branches, installing dependencies, and running benchmarks on both branches. The results are then compared, encoded, and posted as a comment on the PR.

* Revert "Squashed commit of the following:"

This reverts commit 9485087.

* Update .gitignore

* further updates

* Update benchmarks/WolfSheep/__init__.py

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
@EwoutH
Copy link
Member Author

EwoutH commented Jan 24, 2024

Note to self: It looks like the comment-triggerd workflows can't find the PR branch correctly. Needs a fix.

This was referenced Jan 24, 2024
Merged
Merged
rht pushed a commit that referenced this pull request Jan 27, 2024
@quaquel @pre-commit-ci
perf: Speed up Agentset.shuffle (#2010)
449d230 
* Make RandomActivationByType.agents_by_type backward compatible

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* updated warning

* Update .gitignore

* Update global_benchmark.py

* pep8 rename

* Update schelling.py

* Update schelling.py

* Update schelling.py

* Update schelling.py

* Squashed commit of the following:

commit c0de4a1
Author: Ewout ter Hoeven <E.M.terHoeven@student.tudelft.nl>
Date:   Sun Jan 21 14:43:08 2024 +0100

    Add CI workflow for performance benchmarks (#1983)

    This commit introduces a new GitHub Actions workflow for performance benchmarking. The workflow is triggered on `pull_request_target` events and can be triggered with a "/rerun-benchmarks" comment in the PR. It should be compatible with PRs from both forks and the main repository. It includes steps for setting up the environment, checking out the PR and main branches, installing dependencies, and running benchmarks on both branches. The results are then compared, encoded, and posted as a comment on the PR.

* Revert "Squashed commit of the following:"

This reverts commit 9485087.

* Update .gitignore

* further updates

* Update benchmarks/WolfSheep/__init__.py

* shuffle update

* switch to walrus operator in do

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

* revert inplace to False

* remove old code

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tpike3 tpike3 tpike3 left review comments

@Corvince Corvince Corvince left review comments

@rht rht rht approved these changes

Assignees
No one assigned
Labels
ci Release notes label
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@EwoutH @rht @jackiekazil @Corvince @tpike3