Skip to content

CI Add workflow_dispatch event to manually trigger wheel builder #18801

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
Dec 1, 2020

Conversation

alfaro96
Copy link
Member

@alfaro96 alfaro96 commented Nov 9, 2020

Reference Issues/PRs

See #18774 (comment).

What does this implement/fix? Explain your changes.

This PR adds the workflow_dispatch event so that we can manually trigger the Wheel builder workflow when needed. Thus, I think that we do not need the [cd build] commit trigger anymore.

CC @rth, @ogrisel and @thomasjpfan.

@alfaro96
Copy link
Member Author

alfaro96 commented Nov 9, 2020

I have been able to run the Wheel builder event in my fork.

Note that the workflow_dispatch event must be in the default branch before we can manually trigger the workflow. Once this PR is merged to master, only collaborators with write access should be able to manually trigger the workflow on any branch (even on forks).

@thomasjpfan
Copy link
Member

Is there a way to trigger workflow_dispatch on a pull request? It seems like it can only be triggered on a branch from the main repo.

@alfaro96
Copy link
Member Author

alfaro96 commented Nov 10, 2020

Is there a way to trigger workflow_dispatch on a pull request? It seems like it can only be triggered on a branch from the main repo.

Yes, you can trigger on a branch from the forked repository. Indeed, I have been able to trigger the workflow_dispatch event on the alfaro96:trigger_workflow_dispatch branch.

@thomasjpfan
Copy link
Member

Yes, I can see how a contributor can trigger the workflow on their fork. How does a reviewer trigger the workflow when they are reviewing a PR?

@alfaro96
Copy link
Member Author

Yes, I can see how a contributor can trigger the workflow on their fork. How does a reviewer trigger the workflow when they are reviewing a PR?

What do you about having a cd build tag to trigger the Wheel builder?

@thomasjpfan
Copy link
Member

What do you about having a cd build tag to trigger the Wheel builder?

The cd build tag allows maintainers to push directly to a branch to trigger the workflow. I am thinking of when we do a PR to a release branch and we want to make sure wheels build okay before merging. In this case, we would need to have the person that open the PR, navigate to through the github UI to trigger the workflow.

@thomasjpfan
Copy link
Member

I think the workflow of manually triggering would be:

  1. Create PR to branch and have the person that open that PR trigger the workflow on the branch and link it.
  2. Merge branch.
  3. Trigger workflow on branch from main repo
  4. If wheels failed to build, open another PR to fix.

@alfaro96
Copy link
Member Author

alfaro96 commented Nov 10, 2020

Sorry @thomasjpfan, I misunderstood you 😞.

Nevertheless, I think that I get your point now.

I think the workflow of manually triggering would be:

  1. Create PR to branch and have the person that open that PR trigger the workflow on the branch and link it.

Alternatively, we could go to the Actions tabs of the fork to check if the workflow works (previously triggered by the person that open the PR).

  1. Merge branch.
  2. Trigger workflow on branch from main repo

Yes, a maintainer should manually trigger the workflow_dispatch event in the main repository. Alternatively, we could automatically trigger using the push event to release branches (but I am not sure if we want to trigger the Wheel builder workflow on all pushes to release branches).

  1. If wheels failed to build, open another PR to fix.

Yes if it is related with the PR itself (and not with external issues).

Going back to your comment in #18801 (comment), the question is how a reviewer (in this case, maintainer with write access) could trigger the Wheel builder workflow in a PR?

@thomasjpfan
Copy link
Member

thomasjpfan commented Nov 10, 2020

Sorry @thomasjpfan, I misunderstood you 😞.

It's okay! These things happen! I will do better with structuring my questions. :)

Going back to your comment in #18801 (comment), the question is how a reviewer (in this case, maintainer with write access) could trigger the Wheel builder workflow in a PR?

Yes this was the original question. If there was a way, I would be +1 on this PR.

@alfaro96
Copy link
Member Author

I have not found a way to allow reviewers (maintainers with write access) to trigger the Wheel builder workflow on their own on PRs.

Maybe @rth could help us.

@ogrisel
Copy link
Member

ogrisel commented Nov 12, 2020

I have not found a way to allow reviewers (maintainers with write access) to trigger the Wheel builder workflow on their own on PRs.

I would be in favor of re-introducing the [cd build] commit message trigger then (in addition to the Github UI trigger). WDYT?

@rth
Copy link
Member

rth commented Nov 12, 2020

Yes, I suggested this, but it's true that this use case might be problematic. It's probably possible to trigger it from upstream branches, but that's not the worflow we use. Sorry )

@alfaro96 alfaro96 force-pushed the trigger_workflow_dispatch branch from fa443f1 to 5e9e85d Compare November 18, 2020 18:14
@alfaro96
Copy link
Member Author

Easy review @ogrisel and @thomasjpfan?

In the end, I have only included the workflow_dispatch event.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

LGTM!

As a reference, the trigger looks like this:

Screen Shot 2020-11-18 at 6 18 10 PM

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

Thanks for the screenshot @thomasjpfan. I guess it does not hurt and it can spare a bit of commit noise.

LGTM.

@ogrisel ogrisel merged commit 2d294af into scikit-learn:master Dec 1, 2020
@alfaro96 alfaro96 deleted the trigger_workflow_dispatch branch December 1, 2020 10:15
@ogrisel
Copy link
Member

ogrisel commented Dec 1, 2020

Hum I should have edited the commit message to reflect what this PR actually does...

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