Skip to content

CI Removes check milestone workflow #19843

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

Merged
merged 1 commit into from
Apr 8, 2021

Conversation

thomasjpfan
Copy link
Member

Reference Issues/PRs

Reverts #19833

What does this implement/fix? Explain your changes.

There is no way to trigger a pull_request target with a milestoned event: https://github.community/t/feature-request-add-milestone-changes-as-activity-type-to-pull-request/16778/4. This means a maintainer would need to add a milestone and add a label to trigger the milestone check. If there is no appropriate label, to add we need to add a label and then remove it to trigger the milestone check.

Given that this check is for milestones and we can not trigger it with a change in milestones, I think it is better to remove.

Any other comments?

  1. I have the same concerns as @NicolasHug: CI Add a check for milestones. #19833 (comment) when it comes to contributor experience. The milestone check can only be resolves by the triage team or a maintainer.
  2. For some PRs, I am uncomfortable with setting the 1.0 milestone if I am the first reviewer. If I was the second reviewer, I would be a little more comfortable setting the milestone.
  3. As stated above, we are not able to trigger on milestone changes. This is not intuitive for maintainers.
  4. Some PRs do not have a milestones because they are WIP and we do not know when they will land. For example, when we are waiting on a contributor to update or include a benchmark.
  5. Imagine a contributor opens a PR and sees the check and ask a maintainer to milestone it. But due to reason 4, we do not know when the PR will land. This may lead to a contributor being demotivated. If we do milestone it and it is not ready for release, again the contributor may be demotivated.

I think it is good to put items on the 1.0 checklist, but forcing all PRs to have a milestone does not make project management easier.

CC @cmarmo @glemaitre @ogrisel

Copy link
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Thanks @thomasjpfan . I agree with all of the above. Similar points were noted by Joel and I in the original issue #19596

@glemaitre glemaitre merged commit 4b53fc3 into scikit-learn:main Apr 8, 2021
@cmarmo
Copy link
Contributor

cmarmo commented Apr 8, 2021

There is no way to trigger a pull_request target with a milestoned event: https://github.community/t/feature-request-add-milestone-changes-as-activity-type-to-pull-request/16778/4. This means a maintainer would need to add a milestone and add a label to trigger the milestone check. If there is no appropriate label, to add we need to add a label and then remove it to trigger the milestone check.

This is not a problem: the check can be triggered at any synchronization and, generally pull requests are not finished when the horizon of a milestone arises.

1. I have the same concerns as @NicolasHug: [#19833 (comment)](https://github.com/scikit-learn/scikit-learn/pull/19833#issuecomment-815246257) when it comes to contributor experience. The milestone check can only be resolves by the triage team or a maintainer.

I trust contributors understanding: the true frustrating experience is when your pull request is all green and you wait for months to be reviewed. We had more questions about why reviewer are not answering with respect to "why the changelog is red", for after answering thrice, people understand that this is just a technical check, not a human problem

2. For some PRs, I am uncomfortable with setting the 1.0 milestone if I am the first reviewer. If I was the second reviewer, I would be a little more comfortable setting the milestone.

You can wait till the end and apply the milestone last minute if you want

3. As stated above, we are not able to trigger on milestone changes. This is not intuitive for maintainers.

Don't underestimate maintainers

4. Some PRs do not have a milestones because they are WIP and we do not know when they will land. For example, when we are waiting on a contributor to update or include a benchmark.

See above

5. Imagine a contributor opens a PR and sees the check and ask a maintainer to milestone it. But due to reason 4, we do not know when the PR will land. This may lead to a contributor being demotivated. If we do milestone it and it is not ready for release, again the contributor may be demotivated.

See point 1.

I think it is good to put items on the 1.0 checklist, but forcing all PRs to have a milestone does not make project management easier.

I'm in the process of automating backport to minor releases if I can't trust the milestone this automation is meaningful.
I was also modifying the changelog check taking into account this change: I don't see how this change can be tested in one night.

@glemaitre
Copy link
Member

Reverting. However, I think that we need to automate more the release process and the cherry-picking + in bug fix release is something that would be nice to have. As I said there #19833 (comment), I think that it should be possible to bypass these GitHub actions during "reviews required", "draft" or "WIP" PRs status (using branch protection, PRs title, and PRs status). It would make it less intimidating than a failure when opening your first PR.

IMO, point 3. is a blocker to me while I could live with a default strategy in 2. even if it requires moving to the next milestone at the time of the release. This might be part of the release process to decide to postpone or include a feature.

@NicolasHug
Copy link
Member

NicolasHug commented Apr 8, 2021

I trust contributors understanding: the true frustrating experience is when your pull request is all green and you wait for months to be reviewed

I agree with you that this is a frustrating experience. I definitely appreciate the effort towards having more responsive review cycles, but I don't think this check is a good solution for that. Making milestones mandatory will not change reviewers behaviors. Perhaps I should, but I personally never look at milestones unless we're in the last few weeks/days before a release, when things need to get in. Even when I was paid to work on scikit-learn full-time, I had too much on my plate to worry about milestone labels. I assume I'm not the only one.

@cmarmo, I'm still unclear on how this check will help the release process (I never took part of the release process so I'm missing some context). If what we care about here is about milestones for merged PR, as opposed to open PRs, then perhaps a solution would be to start adopting an oncall rotation where each week, one of us is responsible of making sure that the merged PRs have correct tags (triage + core devs). Another option is to have a check that ensures those things when merging, instead of something that's part of the CI - not sure if that's technically doable though.

I'm skeptical about adding more CI checks in general, and I think we need to be extremely careful about the CI. A flaky / difficult CI is a major source of frustration for both contributors and maintainers.

We should only allow the CI to go red for very good reasons. Failing tests and build issues are such good reasons, but a missing / incorrect label doesn't seem like one. Also, when the CI is failing, users should be able to a) understand the issue and b) be able to fix it themselves. This check does not allow that.

@cmarmo
Copy link
Contributor

cmarmo commented Apr 8, 2021

@cmarmo, I'm still unclear on how this check will help the release process (I never took part of the release process so I'm missing some context). If what we care about here is about milestones for merged PR, as opposed to open PRs, then perhaps a solution would be to start adopting an oncall rotation where each week, one of us is responsible of making sure that the merged PRs have correct tags (triage + core devs). Another option is to have a check that ensures those things when merging, instead of something that's part of the CI - not sure if that's technically doable though.

This is not technically doable for now, unless you block the merge protecting the branch: this will show a big red flag anyway so I don't see the difference.
The point here is that core-devs should not do oncall rotations manually checking boring things, they should have the time to review and code. And flags and labels should be applied automatically (as far as possible) to ensure consistency and readability from external people, with, ideally, documented workflows. Red flags can change habits, unless one is radically opposed: I have automatically processed thousands of rasters and let me say that a red flag at the beginning of the process will save you days and months of processing, suddenly the term 'best practices' means something.
But the path from almost 0% automation to almost 100% asks for some experimenting and changing of habits.
I already answered to questions about the changelog check, contributors understood, they are not shocked or worried: I can do the same thing for the milestone check. This is not a waste of time for maintainers.
The amount of time spent to block this minor experimentation (that I am committed to document and take responsibility for with respect to contributors, and that is already implemented in other open source projects to which contributions are quite enjoyable) is incredibly high....

About how this check can help the release and the changelog: I was planning to open pull requests for that, where I would have been glad to discuss the follow-up: this is a technical discussion and I think it is more relevant if an implementation is available.

This revert just shut me up.

thomasjpfan added a commit to thomasjpfan/scikit-learn that referenced this pull request Apr 19, 2021
@glemaitre glemaitre mentioned this pull request Apr 22, 2021
12 tasks
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.

4 participants