You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "Omega359 (via GitHub)" <gi...@apache.org> on 2025/03/31 19:46:11 UTC
[I] Run all benchmarks on merge to main branch [datafusion]
Omega359 opened a new issue, #15511:
URL: https://github.com/apache/datafusion/issues/15511
### 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_
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "Shreyaskr1409 (via GitHub)" <gi...@apache.org>.
Shreyaskr1409 commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2769198630
take
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2769848896
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2767313655
Sure. https://github.com/apache/datafusion/issues/5504
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "Shreyaskr1409 (via GitHub)" <gi...@apache.org>.
Shreyaskr1409 commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2769170980
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2767310181
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2769754304
> > I don't think we need to actually "benchmark" the code for each merge.
>
> The issue [#5504](https://github.com/apache/datafusion/issues/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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "Shreyaskr1409 (via GitHub)" <gi...@apache.org>.
Shreyaskr1409 commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2769119055
> I don't think we need to actually "benchmark" the code for each merge
How about we set a tag/label for performance related PRs and run benchmark tests for those specific PRs only? I could look into it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "vito1317 (via GitHub)" <gi...@apache.org>.
vito1317 commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-4967207827
take
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2767683793
I see what you are saying now: https://github.com/apache/datafusion/pull/15500#issuecomment-2767216080
I now agree it would be good to test these somehow
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "vito1317 (via GitHub)" <gi...@apache.org>.
vito1317 commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-4967196046
take
Proposal for a first increment, keeping the CI-cost concerns discussed above in mind:
- a `benchmarks-smoke` job in the extended tests workflow, gated to merges to `main` / release branches and manual dispatch (not PRs)
- `cargo check` all benchmark bins and benches first — this alone catches the most common "benchmark silently broken" failures cheaply
- then run a representative subset end-to-end on small data, one query each, as a "does it still run" check (explicitly not a performance measurement): `tpch`, `tpch_mem`, `sort_tpch`, `external_aggr` on TPC-H SF1 (generated with tpchgen-cli), plus `cancellation` (generates its own data)
- includes a small `bench.sh` fix: the TPC-H answers copy used `docker run -it`, which fails without a TTY in CI
Benchmarks needing multi-GB downloads (clickbench, h2o, imdb, tpcds) are left out of the first pass; coverage can be extended once the approach looks right. PR incoming.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "Omega359 (via GitHub)" <gi...@apache.org>.
Omega359 commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2769284420
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2777328776
> 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
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2767724669
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [I] Run all benchmarks on merge to main branch [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on issue #15511:
URL: https://github.com/apache/datafusion/issues/15511#issuecomment-2774860687
> 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org