-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Comments
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. |
Sure. #5504 |
I see what you are saying now: #15500 (comment) I now agree it would be good to test these somehow |
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. |
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. |
take |
We could do that too however not all benchmarks are sql queries. |
I don't think we need to run the benchmark on CI, at least it should be optional and disable by default.
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 |
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. |
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.
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. |
This is what I said so I agree with this
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. |
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
The text was updated successfully, but these errors were encountered: