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

Fix broken benchmark tests #1000

Merged
merged 6 commits into from
Jul 18, 2024
Merged

Fix broken benchmark tests #1000

merged 6 commits into from
Jul 18, 2024

Conversation

haider-rs
Copy link
Contributor

Description

This PR fixes outdated/broken benchmark tests.

Fixes # (issue)
#997

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Code review prechecks:

  • Code follows the style guidelines of this project
  • Code has been self-reviewed
  • Inline comments have been added for each method
  • I have made corresponding changes to the documentation
  • Code changes introduces no new problems or warnings
  • Test cases have been added
  • Dependent changes have been merged and published in downstream modules

@haider-rs haider-rs force-pushed the fix-broken-benches branch from 1fd00e1 to 89cf8ef Compare July 10, 2024 19:36
@haider-rs haider-rs self-assigned this Jul 11, 2024
@haider-rs haider-rs force-pushed the fix-broken-benches branch from 385c5bf to 7ebd55d Compare July 11, 2024 16:07
@haider-rs haider-rs marked this pull request as ready for review July 11, 2024 16:41
@4meta5
Copy link
Contributor

4meta5 commented Jul 11, 2024

can we add ci-benchmark and push an empty commit (ie into development) to trigger generating new weights based on these changes as well?

this would also confirm that running the benchmarks to generate weights works when it is run through the CI (I know you verified it runs manually but could be different)

Copy link
Contributor

@4meta5 4meta5 left a comment

Choose a reason for hiding this comment

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

other than rerunning weights, looks good to me

@haider-rs haider-rs added the !ci-benchmark Benchmark and commit new weights label Jul 11, 2024
Copy link
Collaborator

@dvc94ch dvc94ch left a comment

Choose a reason for hiding this comment

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

LGMT, thanks!

@dvc94ch
Copy link
Collaborator

dvc94ch commented Jul 12, 2024

rerunning. the failure was due to cancelling the benchmarking after almost 6h. how long does benchmarking usually take? 6h seems like a lot... cc @penumbra23

@haider-rs haider-rs linked an issue Jul 12, 2024 that may be closed by this pull request
Copy link
Collaborator

@FlorianFranzen FlorianFranzen left a comment

Choose a reason for hiding this comment

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

Looks reasonable.

@FlorianFranzen
Copy link
Collaborator

Not sure what the hold-up is here. Will merge it for now as this block my further work.

@dvc94ch runtime-benchmarks will need to be addressed separately.

@FlorianFranzen FlorianFranzen merged commit 55352c3 into development Jul 18, 2024
10 of 11 checks passed
@FlorianFranzen FlorianFranzen deleted the fix-broken-benches branch July 18, 2024 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
!ci-benchmark Benchmark and commit new weights
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Broken benchmark test
4 participants