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

Auto-build Docker images before on-merge if setup.py was changed #17573

Merged
merged 7 commits into from
Jun 23, 2022

Conversation

muellerzr
Copy link
Contributor

What does this PR do?

This PR introduces a new workflow that will check if the setup.py was modified during a pull request merge. If so, it will trigger the docker images to be rebuilt before running the on-merge tests.

It also changes self-push to be ran on a workflow_run, specifically the new check-dependencies job. This new job also maintains the same "on-merge" check the previous job had, when it comes to determining if it should be ran and when.

Finally, build-docker-images is now also ran on a workflow_call, so that check-dependencies can trigger it.

This is the same as done in Accelerate recently, with the only difference being additional file filters huggingface/accelerate#424

Why is this needed?

A frustration I've noticed over the last few months in this repo is the main tests runners are failing for an entire day, due to a new dependency introduced. This solves this problem, since the issue roots from the docker images being used.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

@ydshieh @LysandreJik

@muellerzr muellerzr added the Tests Related to tests label Jun 6, 2022
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 6, 2022

The documentation is not available anymore as the PR was closed or merged.

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 6, 2022

Do not merge for now. The push CI in transformers is somehow tricky, after the recent change in #17369.
I will review tomorrow, but I think some changes have to be made.

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 7, 2022

@muellerzr Thank you for the PR.

The most direct approach would be

Integrate the check check-for-setup and build-docker-containers in
https://github.com/huggingface/transformers/blob/main/.github/workflows/self-push-caller.yml
(before the job run_push_ci)

Otherwise (if you really want to keep the logic you have), the following block

  workflow_run:
    workflows: ["Check for dependency modification"]
    branches: ["main"]
    types: [completed]

should go in self-push-caller.yml.

The main point is to run the actual push CI tests on another branch (push-ci), otherwise there will be more than 256 job results shown in the commit history page.

I would prefer the most direct approach (the first one).

@muellerzr
Copy link
Contributor Author

@ydshieh I believe I addressed what you wanted, let me know if otherwise 😄

Copy link
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

LGTM, but you can rebase on main first and double check if there are some redundant blocks in the final commit before merge.

Thank you for this work!

There are some changes recently (the 2 push workflow files)

.github/workflows/self-push.yml Outdated Show resolved Hide resolved
@muellerzr muellerzr reopened this Jun 23, 2022
@muellerzr muellerzr merged commit 893ab12 into main Jun 23, 2022
@muellerzr muellerzr deleted the docker-autobuild branch June 23, 2022 20:51
younesbelkada pushed a commit to younesbelkada/transformers that referenced this pull request Jun 25, 2022
…gingface#17573)

* Auto-build on setup modification

* Modify push-caller

* Make adjustments based on code review
younesbelkada pushed a commit to younesbelkada/transformers that referenced this pull request Jun 29, 2022
…gingface#17573)

* Auto-build on setup modification

* Modify push-caller

* Make adjustments based on code review
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tests Related to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants