Adding performance benchmarks#548
Conversation
|
✅ 66/66 passed, 5m56s total Running from perf_test #92 |
|
✅ 295/295 passed, 5 flaky, 1 skipped, 2h45m42s total Flaky tests:
Running from acceptance #2194 |
|
✅ 66/66 passed, 6m15s total Running from perf_test #4 |
|
@cornzyblack @ghanse i moved the previous PR here so that we can run the tests directly. It is complete, please review. |
There was a problem hiding this comment.
Pull Request Overview
This PR implements comprehensive performance benchmarks for the DQX library to monitor performance regression over time and ensure the library maintains optimal performance as it evolves.
- Adds pytest-benchmark based performance tests covering all check functions including row, dataset, and foreach column rules
- Implements automated performance testing pipeline with GitHub Actions that runs on PRs and nightly builds
- Creates automated reporting system that generates markdown documentation from benchmark results
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/perf/test_apply_checks.py | Comprehensive benchmark tests covering all check functions with 100 million row datasets |
| tests/perf/generate_md_report.py | Script to generate markdown reports from baseline benchmark results |
| tests/perf/conftest.py | Test configuration and fixtures for performance tests |
| tests/perf/.benchmarks/baseline.json | Baseline performance metrics for comparison |
| .github/workflows/performance.yml | GitHub Actions workflow for PR performance testing |
| .github/workflows/nightly.yml | Added nightly performance testing job |
| pyproject.toml | Added performance testing dependencies and hatch script |
| docs/dqx/docs/reference/benchmarks.mdx | Auto-generated benchmark documentation |
| docs/dqx/docs/dev/contributing.mdx | Updated contributing guidelines with performance testing info |
| Makefile | Added performance testing target |
| .github/pull_request_template.md | Added performance testing checkbox |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
Hi @mwojtyczka, all checks in check_funcs have been covered. I can see you added some tests with |
That would be great. We should cover all check funcs with foreach. Check if you can commit to the branch. |
Alright will add them, so I can't commit and push to this branch directly, but I can open a PR to this branch and maybe you can trigger the performance from your end? |
You can fork it and create PR to merge it to this branch. Alternatively, you could just drop the code here, but it would be great if your contribution is visible. I plan to merge on Monday, if you can complete by Monday go ahead, otherwise, I will add the missing tests. |
Sure, will add it to this branch (PR) before Monday. |
|
Hi @mwojtyczka I have made the changes in this PR #556 @ghanse I added the custom ways to generate dataset types in conftest for this comment |
## Changes - Added foreach Performance tests for row checks - Added a custom way to generate dataset types in conftest ### Linked issues <!-- DOC: Link issue with a keyword: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved. See https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword --> Resolves #.. ### Tests <!-- How is this tested? Please see the checklist below and also describe any other relevant tests --> - [x] manually tested - [ ] added unit tests - [ ] added integration tests - [ ] added end-to-end tests - [x] added performance tests --------- Co-authored-by: Marcin Wojtyczka <marcin.wojtyczka@databricks.com>
ghanse
left a comment
There was a problem hiding this comment.
Overall LGTM. Left a minor comment on GPG signing.
| Baseline and benchmark reports are committed automatically by the performance github workflow. | ||
| However, these commits are not GPG-signed because your private key is not available to GitHub. | ||
| Before merging a PR that updates the baseline or reports, you must manually sign and re-apply the commits using your local GPG key, as follows: | ||
| ```shell | ||
| git reset --soft $(git merge-base origin/main HEAD) # point to the common ancestor of main and your branch | ||
| #git reset --soft HEAD~<how-many-commit-go-back> # alternatively, specify how many commits to go back | ||
| git commit -S --reuse-message=ORIG_HEAD | ||
| git push --force-with-lease origin HEAD | ||
| ``` |
There was a problem hiding this comment.
Is there any way to avoid this? Could we add a GPG key in the repo secrets and sign with that key when the benchmarks are committed?
There was a problem hiding this comment.
I haven't found a way because the commit is on behalf of the user that triggers the workflow. I don't want to use a specific user.
Changes
docs/dqx/docs/reference/benchmarks.mdx) using the baseline (tests/perf/.benchmarks/baseline.json), whenever the baseline changes.tests/perf/.benchmarks/baseline.json. If the baseline file is removed or any test is removed or added, the baseline is updated automatically and commited to git. Note that the commits are not signed so user must re-add the commit if baseline is updated. Instructions in the contributing guide are provided.Tests specification:
Example output for perf tests:
This PR is continuation of #525
Linked issues
Resolves #56
Tests