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

[CI] Diff against the right remote + branch in Regressions.yml - Regression Test new micro benchmark #12106

Merged
merged 7 commits into from
May 24, 2024

Conversation

Tishj
Copy link
Contributor

@Tishj Tishj commented May 17, 2024

This PR aims to fix CI of PRs made to feature:

Run git diff --name-only origin/main | (grep "benchmark/micro/*" || true) > .github/regression/updated_micro_benchmarks.csv
Detected the following modified benchmarks. Running a regression test
benchmark/micro/csv/ignore_errors.benchmark
benchmark/micro/csv/multiple_small_files.benchmark
benchmark/micro/csv/time_type.benchmark
benchmark/micro/result_collection/batched_stream_query.benchmark
benchmark/micro/csv/ignore_errors.benchmark
Old timing: 2.205773
New timing: Failed to run benchmark benchmark/micro/csv/ignore_errors.benchmark

benchmark/micro/csv/multiple_small_files.benchmark
Old timing: 0.244721
New timing: Failed to run benchmark benchmark/micro/csv/multiple_small_files.benchmark

benchmark/micro/csv/time_type.benchmark
Old timing: 0.921153
New timing: Failed to run benchmark benchmark/micro/csv/time_type.benchmark

benchmark/micro/result_collection/batched_stream_query.benchmark
Old timing: 7.897132
New timing: Failed to run benchmark benchmark/micro/result_collection/batched_stream_query.benchmark

By only running this job if the event is a PR, and diffing against the target branch instead of hardcoding this to main

@Tishj Tishj requested a review from Tmonster May 17, 2024 11:06
@Tmonster
Copy link
Contributor

Why are we doing only on pull request? What was wrong with always()? The other regression tests also run always

@Tishj
Copy link
Contributor Author

Tishj commented May 17, 2024

As said before, there is no way to know what branch this has to diff against for a push
if you mark the PR as ready for review it will trigger this step

Copy link
Contributor

@Tmonster Tmonster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

true, forgot about that detail. LGTM!

@carlopi
Copy link
Contributor

carlopi commented May 17, 2024

I just realized looking at this: why do we have the push block at all? (I know why we used to have it, but I think it can be probably just removed everywhere)

@Mytherin Mytherin merged commit 1678c7f into duckdb:main May 24, 2024
4 checks passed
@Mytherin
Copy link
Collaborator

After merging and getting a merge conflict when propagating this to feature - I wonder if there is any added functionality here over what was already done in #11762

@Tmonster
Copy link
Contributor

I'll check and submit a PR to fix main and or master

@Mytherin
Copy link
Collaborator

I've left feature with only #11762 for now, feel free to open a new PR to feature with whichever changes are relevant there.

github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Jun 5, 2024
Merge pull request duckdb/duckdb#12238 from hawkfish/asof-pushdown
Merge pull request duckdb/duckdb#12236 from carlopi/macosmin12
Merge pull request duckdb/duckdb#12229 from carlopi/cleanup_workflows
Merge pull request duckdb/duckdb#12224 from Tishj/python_disable_replacement_scan
Merge pull request duckdb/duckdb#12106 from Tishj/generic_new_micro_benchmark_check
Merge pull request duckdb/duckdb#12208 from Tishj/python_fix_replacement_scan_hiding
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants