8000 RFC enable github's pull request merge queue? · Issue #25583 · scikit-learn/scikit-learn · GitHub
[go: up one dir, main page]
Skip to content

RFC enable github's pull request merge queue? #25583

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.

Sign up for GitHub

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

Jump to bottom
Closed
ogrisel opened this issue Feb 10, 2023 · 8 comments
Closed

RFC enable github's pull request merge queue? #25583

ogrisel opened this issue Feb 10, 2023 · 8 comments
Labels
RFC

Comments

@ogrisel
Copy link
Member
ogrisel commented Feb 10, 2023

https://github.blog/changelog/2023-02-08-pull-request-merge-queue-public-beta/

It seems like a nice usability improvement.
@ogrisel ogrisel added the RFC label Feb 10, 2023
@ogrisel ogrisel changed the title RFC enable github merge queues? RFC enable github's pull request merge queue? Feb 10, 2023
@lesteve
Copy link
Member
lesteve commented Feb 10, 2023

Just curious, I quickly read the article and I am not to sure this would be a game changer, but maybe I missed something ...

Before merge queue, developers were often required to update their pull request branches prior to merging to ensure their changes wouldn't break the main branch when merged.

This rarely happens in scikit-learn right? I feel PRs don't step on each other toes in a vast majority of cases but again maybe I am missing something.

@ogrisel
Copy link
Member Author
ogrisel commented Feb 10, 2023

I think it reduces the likelihood of wasting CI when using conditional delayed merges.

@betatim
Copy link
Member
betatim commented Feb 10, 2023 •
edited
Loading

As I understand it the problem that a merge queue solves is that PRs are "constantly" being merged into main and you have to rebase on main/merge main into before being able/allowed to merge your PR. In case the combination of the new changes on main and your branch lead to a CI failure.

However I don't think the merging of PRs is done like this* in scikit-learn. So maybe the benefit is small?

What does seem to happen regularly is that your "what's new" changes conflict with main if your PR doesn't get merged soon. But merge queues don't help with that.

Overall, I feel like there is something I am missing regarding this new feature. It seems like a solution to a problem not a lot of people have?? (Or that I misunderstand something)

  • If the CI was run three days ago for a PR and is green, then people merge a PR. Even if there have been changes to main in the mean time.

@thomasjpfan
Copy link
Member

From memory, the issue that merge queue solves for happens around ~2 times a year in the past 3 years. For example, when a new test was added to main that would have failed in a PR and the PR was not updated with main recently. If we merge the PR, then it will cause main to fail.

In any case, I think it's okay to turn merge queues on and see if we like it. We can always turn it off if it's too much of a hassle.

@ogrisel
Copy link
Member Author
ogrisel commented Feb 10, 2023

I enabled it for main. Let's see how it goes.

@ogrisel ogrisel closed this as completed Feb 10, 2023
@ogrisel
Copy link
Member Author
ogrisel commented Feb 13, 2023

I had a problem when trying to use it today in #25585. Not sure what's going one because everything was green on this PR but apparently there was a 4 min timeout (see details in the linked PR).

@thomasjpfan
Copy link
Member

I saw the same issue with the merge queue in #25613.

@thomasjpfan
Copy link
Member

I saw the same issue with the merge queue in #25633. I increase the wait time to 90 minutes because Azure Pipeline took > 60 minutes to complete. Then I noticed that the "Check Changelog" GitHub Action never actually runs on the merge queue and waits forever. I suspect this is a bug with the merge queue feature.

Since the merge queue never actually merges even when all the tests pass, I am going to turn the merge queue off for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
RFC
Projects
None yet
Milestone
No milestone
36ED
Development

No branches or pull requests
4 participants
@ogrisel @betatim @lesteve @thomasjpfan
0