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

Automerge rebuilds by default #1115

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

CJ-Wright
Copy link
Member

No description provided.

@CJ-Wright CJ-Wright requested a review from a team as a code owner July 25, 2020 20:00
Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

Not a fan of automerge by default

@beckermr
Copy link
Member

We discussed that the last core meeting and everyone was in agreement. You may not have been there?

@isuruf
Copy link
Member

isuruf commented Jul 25, 2020

Please provide tools for maintainers to opt out of this. I can't opt out for 200 feedstocks manually.

@beckermr
Copy link
Member

We could only provide a tool for feedstocks upon which you are the sole maintainer. Otherwise, the decision needs to be reviewed via pr.

@isuruf
Copy link
Member

isuruf commented Jul 25, 2020

What? So, you don't want to ask permission from other maintainers turn this on, but you want me to ask permission from other maintainers turn this off?

@beckermr
Copy link
Member

This was effectively the policy core agreed to in the meeting IMHO.

If you'd like to discuss more I think it's fine to open an issue on the conda forge GitHub site.

The logic in the meeting was that core goes through and merges rebuild PRs anyways, so the practical effects of this change were minimal. Hence the asymmetry in the permissions request.

@isuruf
Copy link
Member

isuruf commented Jul 25, 2020

The logic in the meeting was that core goes through and merges rebuild PRs anyways, so the practical effects of this change were minimal.

It makes a huge difference. Core doesn't merge right away giving the maintainers time to review. This way, there's almost no time for the maintainers to review a PR. Also, only a few of core has access to regro/cf-scripts and even fewer have admin permissions.

@beckermr
Copy link
Member

We should be discussing this on an issue in conda-forge GitHub io. These objections were not brought up at the meeting and so we need to have everyone be able to see them in the regular spot.

I'd be fine if the bot comes though and changes the pr to automerge after a few days personally.

@beckermr
Copy link
Member

Woops we are here! I am using the GitHub app and getting lost. :/

@conda-forge/core this pr announces the policy change in rebuild prs and automerge discussed at the last meeting. @isuruf has raised an objection that was not raised in the meeting. Bumping you all for input and comments.

@synapticarbors
Copy link
Member

I apologize since I can almost never make the meeting due to work constraints. I've always felt a bit hesitant about automerge, just because our simple import tests can pass, but they can miss changes in version restrictions of dependencies, new dependencies, new submodules, etc. I tend to go to the upstream source and (for python packages), check the setup.py, changelog, etc before I merge. I know from experience that all checks can be green, but changes needed to be made to the meta.yaml.

I realize this particular issue doesn't cause all PRs to automerge. I'm not sure where the right balance is between speeding migrations and risking not doing careful reviews (and I say this as someone who is on over 100 teams).

@ocefpaf
Copy link
Member

ocefpaf commented Jul 25, 2020

From the meeting notes:

rebuild migration automerges default

Currently either automerge is (org-wide?) on or off, but it would be good to allow ppl to choose to automerge only for rebuilds and not version updates

These automerges may be safer than version automerges, since the deps don’t change and the build is more likely to fail if the package would be broken.

regro/cf-scripts#1063

Overall response is positive, we need to document/announce this change

From what I recall from the meeting the idea for the default behavior on this kind of automerge is opt-in and not out.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 25, 2020

@isuruf has raised an objection that was not raised in the meeting. Bumping you all for input and comments.

I recall him being very vocal about the opt/in-out policies. It was raised in the meeting that he and others do not want to make changes in hundreds of feedstocks.

While I'm pro this change for a future Python migration we have to respect what the other maintainers think and analyze case-by-case. They probably have a reason to not want this for boost.

@beckermr
Copy link
Member

The discussion on opt-in/out was on the pinning epochs draft cfep. This item did not have that same discussion.

That however doesn't mean this issue cannot be reraised as nothing there was final.

@CJ-Wright
Copy link
Member Author

The logic was mostly that automerges of rebuilds were safer than version bumps for a few reasons:

  1. They don't introduce new dependencies that are not put into the meta.yaml
  2. Most rebuilds that do not work properly error in the CI during the building process, for example a linking error.
  3. I expect the majority of rebuild errors that are not caught by the CI will not be caught at all until it produces a problem. I don't expect these errors to be found by looking at the setup.py (or other third party installation information) as is true for the version updates.
  4. Automerge is also controlled on a per migration basis, as an example the arch rebuild has no automerges on it.

This approach has had great success with R 4.0 enabling that rebuild to be done in a week.

@ocefpaf
Copy link
Member

ocefpaf commented Jul 26, 2020

This approach has had great success with R 4.0 enabling that rebuild to be done in a week.

We cannot compare R packages with Python or the boost migration. CRAN has rigid rules for package submission that made such actino safe and easy. All package must pass builds before getting into CRAN, their dependency check fail early and has a very good metadata structure.

@CJ-Wright
Copy link
Member Author

their dependency check fail early

My point is that such a dependency check is not needed for this particular class of PRs.

I think the more important point here is: how often does the CI provide a false positive for rebuilds like boost? My understanding is that these kinds of rebuilds are quite brittle and are likely to fail the CI when issues arise. Additionally in those cases where the CI does provide a false positive it is unlikely that the maintainers will catch it.

@isuruf
Copy link
Member

isuruf commented Jul 26, 2020

Additionally in those cases where the CI does provide a false positive it is unlikely that the maintainers will catch it.

This doesn't take into account that maintainers know more about the packages they maintain. Sometimes the upstream developers are the maintainers of the package. Sometimes, the maintainers closely follow the development of the upstream package.

I agree with @ocefpaf that R was special. I made the initial proposal for R 4.0.0 migration to be done by the automerge bot because they have very little dependencies outside of R packages and they are all maintained by a handful maintainers who can't possibly give the attention to the feedstock.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

6 participants