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

Move hash collision test to run only when merging to main #13973

Merged
merged 1 commit into from
Jan 2, 2025

Conversation

Omega359
Copy link
Contributor

@Omega359 Omega359 commented Jan 1, 2025

Which issue does this PR close?

part of #13845

Rationale for this change

The hash collisions job is expensive (just as expensive as the cargo docs tests which are being worked on elsewhere) and rarely fail. It would be better to just run the hash collision job on merge to main instead of on every PR checkin.

What changes are included in this PR?

github actions updates.

Are these changes tested?

Well, kinda. I don't have a merge to main to actually test that portion however from reading the github documentation it should work.

Are there any user-facing changes?

No.

@github-actions github-actions bot added the development-process Related to development process of DataFusion label Jan 1, 2025
@Omega359 Omega359 marked this pull request as ready for review January 1, 2025 20:50
Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Thanks @Omega359 -- let's merge this to main and we can iterate / fix it as follow on PRs if needed.

Comment on lines +28 to +32
pull_request:
branches:
- main
types:
- closed
Copy link
Contributor

Choose a reason for hiding this comment

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

The way we do it in arrow is

on:
  # always trigger
  push:
    branches:
      - main

https://github.com/apache/arrow-rs/blob/4a0bdde24f48d0fc1222d936493f798d9ea4789d/.github/workflows/arrow.yml#L25-L29

I'll make a small follow on PR to touch it up if necessary

@alamb alamb merged commit 264f4c5 into apache:main Jan 2, 2025
25 checks passed
@alamb
Copy link
Contributor

alamb commented Jan 2, 2025

Here is what appears to be the latest workflow: https://github.com/apache/datafusion/actions/runs/12587068377

It does not appear to be running the hash collisions test 🤔

Interestingly there is a separate action that seems to have run on your PR that ran the test:
https://github.com/apache/datafusion/actions/runs/12587068386

I think it would be better if the checks run on the commit on main so the test results end up attached to the commit.

I will make a new PR

@Omega359
Copy link
Contributor Author

Omega359 commented Jan 2, 2025

It wouldn't run in rust workflow as it's been removed from there. https://github.com/apache/datafusion/actions/runs/12587068386 is where it ran (new workflow).

@alamb
Copy link
Contributor

alamb commented Jan 2, 2025

It wouldn't run in rust workflow as it's been removed from there. https://github.com/apache/datafusion/actions/runs/12587068386 is where it ran (new workflow).

Yeah, what I am seeing from https://github.com/apache/datafusion/actions

is that the job ran on your brnach

Screenshot 2025-01-02 at 1 58 05 PM

So I don't think it gets reported attached to the commit on main:

Screenshot 2025-01-02 at 1 59 40 PM

@Omega359
Copy link
Contributor Author

Omega359 commented Jan 2, 2025

Hmm, yes, that is not what we desire here. We want a blocking check on the merge so on failure merge fails. Perhaps it should run on approval?

@alamb
Copy link
Contributor

alamb commented Jan 2, 2025

We want a blocking check on the merge so on failure merge fails. Perhaps it should run on approval?

That is a great idea

@alamb
Copy link
Contributor

alamb commented Jan 2, 2025

We want a blocking check on the merge so on failure merge fails. Perhaps it should run on approval?

That is a great idea

I thought about this some more. I think the key property is that we don't use a version of DataFusion where hash collisions fails. If we run the new job on approval I worry we will maybe not wait for it to complete prior to merge

Thus I would like to proceed with this PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
development-process Related to development process of DataFusion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants