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

Run all benchmarks on merge to main branch #15511

Open
Omega359 opened this issue Mar 31, 2025 · 11 comments
Open

Run all benchmarks on merge to main branch #15511

Omega359 opened this issue Mar 31, 2025 · 11 comments
Assignees
Labels
enhancement New feature or request

Comments

@Omega359
Copy link
Contributor

Is your feature request related to a problem or challenge?

There has been a number of issues where benchmarks stopped working and no one noticed until someone happened to try and run them. This should be resolved and I suggest we do it in the github extended tests where we run all the benchmarks just to verify they actually run whenever a PR is merged into main.

Describe the solution you'd like

Add running of benchmarks in the datafusion extended tests github job.

Describe alternatives you've considered

No response

Additional context

No response

@alamb
Copy link
Contributor

alamb commented Mar 31, 2025

I would persoanlly suggest if we are going to run the benchmarks we should also be actively tracking them / making sure they are measuring something useful.

@Omega359
Copy link
Contributor Author

Sure. #5504

@alamb
Copy link
Contributor

alamb commented Apr 1, 2025

I see what you are saying now: #15500 (comment)

I now agree it would be good to test these somehow

@jayzhan211
Copy link
Contributor

There has been a number of issues where benchmarks stopped working and no one noticed until someone happened to try and run them

Instead of running the benchmark, how about adding those benchmark query to tests, I don't think we need to actually "benchmark" the code for each merge.

@Shreyaskr1409
Copy link
Contributor

Shreyaskr1409 commented Apr 1, 2025

I don't think we need to actually "benchmark" the code for each merge.

The issue #5504 would require all benchmarks to run after each merge. I think we could just add benchmarks directly for now. What do you think?

I am willing to work on* this.

@Shreyaskr1409
Copy link
Contributor

take

@Omega359
Copy link
Contributor Author

Omega359 commented Apr 1, 2025

There has been a number of issues where benchmarks stopped working and no one noticed until someone happened to try and run them

Instead of running the benchmark, how about adding those benchmark query to tests, I don't think we need to actually "benchmark" the code for each merge.

We could do that too however not all benchmarks are sql queries.

@jayzhan211
Copy link
Contributor

I don't think we need to actually "benchmark" the code for each merge.

The issue #5504 would require all benchmarks to run after each merge. I think we could just add benchmarks directly for now. What do you think?

I am willing to work on* this.

I don't think we need to run the benchmark on CI, at least it should be optional and disable by default.

We could do that too however not all benchmarks are sql queries.

I agree that maintaining it is challenging. Adding it to the extended test suite and running it on every merge isn’t a viable solution, as it’s costly and often unnecessary. I don’t think keeping the benchmark functional is essential—it’s more like a script that we can modify as needed, depending on what we want to measure each time

@Omega359
Copy link
Contributor Author

Omega359 commented Apr 1, 2025

I disagree. If it's in the code base it should work or it should be marked as 'experimental' imho. No one wants to have to go through and fix a benchmark anytime they happen to want to check something.

@berkaysynnada
Copy link
Contributor

Instead of running the benchmark, how about adding those benchmark query to tests, I don't think we need to actually "benchmark" the code for each merge.

Keeping all benchmark coverage with their replicas is challenging (as @Omega359 said not all benchmarks are sql queries) and very prone to go out of sync.

Adding it to the extended test suite and running it on every merge isn’t a viable solution, as it’s costly and often unnecessary

You are also right here about the cost, but what if we can have 2 modes for benchmarks, one for the actual benchmarking purpose, and one with just to validate. If it is in validation mode, it will work with very low loads -- e.g. min sampling count, min batch sizes etc.

cargo test (or extended tests) would run the validation mode, and normal benchmarking goes with the standard mode.

@jayzhan211
Copy link
Contributor

jayzhan211 commented Apr 4, 2025

You are also right here about the cost, but what if we can have 2 modes for benchmarks, one for the actual benchmarking purpose, and one with just to validate. If it is in validation mode, it will work with very low loads -- e.g. min sampling count, min batch sizes etc.

This is what I said so I agree with this

how about adding those benchmark query to tests

We mirror the benchmark code as the test, so we run the test and make sure it works, but we don't actually run the benchmark.

If it is sql query, we can add it in sqllogictest crate, if it is not we add it to rust test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants