Repository navigation
test: run microbenchmarks in extended CI - #25836
Conversation
github-actions
Bot
added
development-process
|
@aetosdios27 |
1b9eb64 to
9a57a65
Compare
|
@kosiew Conflicts are resolved: rebased onto current |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for adding microbenchmark execution to extended CI. The approach looks good and addresses runtime failures that compilation alone cannot catch.
The boundary is appropriate: benchmark correctness is checked through Criterion test mode, separately from performance measurement. The benchmark fixes update stale invocation contracts at setup rather than masking failures, without changing production SQL or public APIs. The modest additional CI complexity is justified by the centralized command and explicit fixture selection.
I'm happy to approve. I left two optional suggestions to clarify the Spark slice benchmark labels and strengthen the repeat overflow smoke check. Neither is blocking.
| <Arc<Field>>::from(Field::new(format!("arg_{idx}"), arg.data_type(), true)) | ||
| }) | ||
| .collect::<Vec<_>>(); | ||
| // Apply planner coercion (including ListView to List) before timing the UDF. |
There was a problem hiding this comment.
Could we label the ListView cases as ListView -> List (coercion excluded)? Coercion happens before timing, so these measure execution on List rather than native ListView or conversion cost. Including both types would distinguish them from existing List cases.
| let args_cloned = args.clone(); | ||
| black_box(invoke_repeat_with_args(args_cloned, repeat_times)) | ||
| black_box( | ||
| invoke_repeat_with_args(args_cloned).expect_err( |
There was a problem hiding this comment.
Optional: could we check once outside the timed loop that the overflow error contains string size overflow on repeat? expect_err also accepts unrelated errors. The production regression test already checks this diagnostic, so this is only a smoke-check improvement.
Execute Criterion benchmarks in test mode with debug checks and a small planner fixture. Repair stale benchmark argument and return types exposed by the new check. Closes apache#24868
9a57a65 to
103a62f
Compare
|
@kosiew Addressed both suggestions: the slice cases now say |
|
Thank you for opening this pull request! Reviewer note: cargo-semver-checks reported the current version number is not SemVer-compatible with the changes in this pull request (compared against the base branch). Details |
kosiew
left a comment
kosiew
left a comment
There was a problem hiding this comment.
Thanks for adding microbenchmark execution to extended CI and addressing the follow-up feedback. The approach is appropriate, and both optional review requests have been implemented and independently smoke-tested.
Keeping benchmark correctness checks separate from performance measurement is the right architectural boundary. It catches runtime failures and stale invocation assumptions without introducing performance measurements into CI or changing production behavior.
The follow-up changes correctly clarify that Spark ListView coercion is excluded from timing and validate the expected repeat overflow error outside the timed loop. I found no new defects or remaining actionable concerns, so I'm approving. CI checks are still pending and should complete before merging.
|
🚀 |
|
This job takes 10 minutes of runner time to run. Do we really need to add that much extra CI time? Here is an example: |
Whoops! Shall I raise a PR to amend this to manual execution only?
|
Thank you @kosiew 🙏 That would be my preference -- or maybe just remove the CI check entirely (leave the xtask target) and add some agent instructions 🤔 |
|
I think if we don't have enough CI resources, it'd be better to remove those tests from CI. Flaky microbenchmarks are not ideal, but acceptable. Unfortunately, DataFusion tends to accumulate too many microbenchmarks during development, the full suite is expected to become slow to run. 🤦🏼 IMO the best way is to add a once-per-day CI tier, and run all microbenchs, and even sql benchmarks once to ensure they're working. |
## Which issue does this PR close? - Follow up to apache#25836 [comment](apache#25836 (comment)) ## Rationale for this change The required `microbenchmark tests (amd64)` job consumes approximately 10 runner minutes per merge-queue run. This recurring cost is not justified under the current maintainer resource constraints. This PR moves full microbenchmark health checks to local-only execution, eliminating their automatic CI cost and merge requirement. Runtime-only failures may remain undetected until a contributor explicitly runs the benchmarks. ## What changes are included in this PR? - Remove the microbenchmark job from `.github/workflows/extended.yml`, including automatic execution on merge groups and release pushes. - Depends on apache#26811, which removes `microbenchmark tests (amd64)` from the required checks in `.asf.yaml`. That must merge first: branch protection is read from `main`, so this PR stays blocked on the removed check until then. - Retain the existing local `cargo xtask ci step test microbenchmarks` command and its execution semantics without introducing a dedicated hosted workflow. - Update the contributor guide with local execution instructions, expectations for benchmark-related PRs, and the accepted risk of delayed runtime-failure detection. ## What is the testing strategy for this PR? No new automated tests are included. This change modifies CI configuration and documentation without changing the benchmark implementation. Existing Clippy checks retain benchmark compilation coverage, while the full benchmark health-check command remains available locally. The patch does not establish that the required-check validator, local benchmark command, documentation formatting checks, or affected workflow-policy tests have been run. Merge-queue behavior must also be verified after merge. ## Are there any user-facing changes? No changes to DataFusion's public APIs or query behavior. For contributors and maintainers, full microbenchmark health checks are no longer automatically executed or required for merging. They must be run locally using: ```shell cargo xtask ci step test microbenchmarks ``` No dedicated GitHub Actions workflow is provided. The contributor documentation has been updated accordingly. ## LLM-generated code disclosure This PR includes LLM-generated code and comments. All LLM-generated content has been manually reviewed.
Which issue does this PR close?
Closes #24868.
Rationale for this change
Microbenchmarks can compile successfully but panic when executed. Running their cases in CI catches stale benchmark fixtures and invocation assumptions without collecting performance measurements.
What changes are included in this PR?
cargo xtask ci step test microbenchmarksand a required extended CI job. Following the existing workflow convention, it runs in the merge queue, on release-branch pushes, and manually; it skips PR events.--no-fail-fast, including the physical-plan benchmarks gated bytest_utils. Theci-benchprofile retains workspace debug assertions and overflow checks with light optimization to keep larger cases practical.CLICKBENCH_DATA_PATH. Ordinary performance runs retain their full-dataset default. The separate SQL benchmark harness, which requires generated datasets, remains covered by its existing jobs.What is the testing strategy for this PR?
cargo xtask ci step test microbenchmarksrun passed all 131 benchmark targets on upstream99193cbd0. After rebasing onto6c1727fcd, the Spark slice and repeat benchmark smoke tests passed again, including the new labels and overflow diagnostic assertion.cargo fmt --allandcargo clippy --all-targets --all-features -- -D warningspass with the pinned Rust 1.99 toolchain on the final rebase../ci/scripts/security_audit.shpasses after patchingrustls-webpkito 0.103.16 andwebpki-root-certsto 1.0.10.sort_with_mem_limit_1_runnerRSS assertion: the core integration target had 1,463 passing tests, one failure, and one ignored test. Its RSS monitor reported a zero-byte baseline and a 192.2 MB peak against a 190.7 MB limit. The unchanged test passed in isolation on a clean upstream build, but the full run still fails locally. No tests were skipped and no memory-test code or limits were changed.Are there any user-facing changes?
No query behavior or public API changes. Benchmark contributors gain a local smoke-test command and automated execution coverage.