Skip to content

Allow inputting a base hash in Regression workflow#15082

Merged
Mytherin merged 3 commits intoduckdb:mainfrom
lnkuiper:regression_input
Dec 2, 2024
Merged

Allow inputting a base hash in Regression workflow#15082
Mytherin merged 3 commits intoduckdb:mainfrom
lnkuiper:regression_input

Conversation

@lnkuiper
Copy link
Copy Markdown
Collaborator

@lnkuiper lnkuiper commented Dec 2, 2024

Since #14973, our nightly no longer runs regressions against itself (always succeeding) but against the last successful nightly regression, now sometimes failing. This has caught a CSV regression.

If we don't fix this regression, subsequent workflow runs will fail indefinitely. Sometimes, however, we want to accept regressions, for example, so the CSV reader can parse more timestamp types (at the cost of taking more time - at least, I think that's what's happening here). In such cases, we need to re-run the regression workflow against itself so that it succeeds.

This PR adds an input parameter to run the regression test against a specific DuckDB version. This would also allow us to run the current main against v.1.13.

@lnkuiper lnkuiper requested a review from carlopi December 2, 2024 09:14
@duckdb-draftbot duckdb-draftbot marked this pull request as draft December 2, 2024 09:18
Copy link
Copy Markdown
Contributor

@carlopi carlopi left a comment

Choose a reason for hiding this comment

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

Thanks!!
(needs to be undrafted)

Note that currently this is triggered only on duckdb/duckdb main, so CI is not really relevant once the YAML parses. (I think it would be cool to refactor this, at some later time, to be more flexible)

After this is merged, Regression should be triggered for example like:

gh workflow run Regression --repo duckdb/duckdb --raw-field base_hash=e141994cf832e2362952d432e9d38357d48e5919

( this is on tonight nightly, but any value should do there)

That should make so that current main is tested vs latest nightly. That should succeed and make so that on tonight run the latest successful one (that is current main) is used as a reference.

@lnkuiper lnkuiper marked this pull request as ready for review December 2, 2024 09:53
@carlopi
Copy link
Copy Markdown
Contributor

carlopi commented Dec 2, 2024

Alternative would be checking against main of 24 hour ago. I think that is also OK, but I see more chances of failure go unnoticed (we don't have yet a system in place to track failure reliably, and I think this can just be moved further).

Pro of checking last successful:

  • requires active action (either fixing the regression or resetting baseline), so regression are harder to ignore

Pro of using 24 hours ago:

  • maintenance free

Now that I wrote it down "maintenance free" seems hard to pass on.
@Mytherin?

Either way I would keep the logic for adding a custom baseline, since it can be handy to be able to trigger say "main vs 1.1.3" and see what happened.

@lnkuiper
Copy link
Copy Markdown
Collaborator Author

lnkuiper commented Dec 2, 2024

@carlopi Should be "Maintenance free with a higher chance of letting regressions slip through".

Not saying it's a bad idea, just saying that it's not a win/win - it's a trade-off.

@Mytherin
Copy link
Copy Markdown
Collaborator

Mytherin commented Dec 2, 2024

Since we're running regression tests in between released versions and in the weekly regression runner anyway, I think we should be able to catch all regressions regardless between major releases, so I would say adding an extra layer where we can see this is likely not worth it

@duckdb-draftbot duckdb-draftbot marked this pull request as draft December 2, 2024 12:17
@lnkuiper lnkuiper marked this pull request as ready for review December 2, 2024 12:17
@Mytherin Mytherin merged commit 632335b into duckdb:main Dec 2, 2024
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request Dec 27, 2024
Top-N: Perform global boundary checking before doing sort-key conversion (duckdb/duckdb#15087)
Allow inputting a base hash in Regression workflow (duckdb/duckdb#15082)
Avoid building for Python 3.7 on Windows (duckdb/duckdb#15085)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request Dec 27, 2024
Top-N: Perform global boundary checking before doing sort-key conversion (duckdb/duckdb#15087)
Allow inputting a base hash in Regression workflow (duckdb/duckdb#15082)
Avoid building for Python 3.7 on Windows (duckdb/duckdb#15085)
krlmlr added a commit to duckdb/duckdb-r that referenced this pull request Dec 27, 2024
Top-N: Perform global boundary checking before doing sort-key conversion (duckdb/duckdb#15087)
Allow inputting a base hash in Regression workflow (duckdb/duckdb#15082)
Avoid building for Python 3.7 on Windows (duckdb/duckdb#15085)
github-actions bot pushed a commit to duckdb/duckdb-r that referenced this pull request Dec 28, 2024
Top-N: Perform global boundary checking before doing sort-key conversion (duckdb/duckdb#15087)
Allow inputting a base hash in Regression workflow (duckdb/duckdb#15082)
Avoid building for Python 3.7 on Windows (duckdb/duckdb#15085)
github-actions bot added a commit to duckdb/duckdb-r that referenced this pull request Dec 28, 2024
Top-N: Perform global boundary checking before doing sort-key conversion (duckdb/duckdb#15087)
Allow inputting a base hash in Regression workflow (duckdb/duckdb#15082)
Avoid building for Python 3.7 on Windows (duckdb/duckdb#15085)

Co-authored-by: krlmlr <krlmlr@users.noreply.github.com>
@lnkuiper lnkuiper deleted the regression_input branch April 14, 2025 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants