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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Introduce a Dependency Checker to trigger new Docker Builds on main #424

Merged
merged 6 commits into from
Jun 6, 2022

Conversation

muellerzr
Copy link
Collaborator

@muellerzr muellerzr commented Jun 3, 2022

Introduce a Dependency Checker to trigger new Docker Builds on main

What does this add?

This PR introduces a new workflow that will do a few things:

  1. Introduces a workflow that will check if the setup.py was adjusted in a pull request
  2. Adjusts build-docker-images to be triggered by a workflow_call, allowing check_dependencies to trigger it
  3. Adjusts the on-merge workflow to be triggered on main by the completion of check_setup, rather than a push directly. This will ensure that the tests will be passing on main if a new version of anything is introduced with the other runners + nightlies

Why is it needed?

A very annoying frustration I've seen over the last few months in the transformers repo is people freaking out when the runners are a failure, due to a new dependency introduced. This solves this problem, and I'll be building a similar workflow for transformers on Monday. (This was just the "test")

cc @ydshieh as this'll be coming to transformers afterwards 馃槈

@muellerzr muellerzr added the enhancement New feature or request label Jun 3, 2022
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Jun 3, 2022

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

Copy link
Collaborator

@sgugger sgugger left a comment

Choose a reason for hiding this comment

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

Very nice idea, maybe a bit over-engineered? I don't think we need a bot comment to say the docker image is being update, it's just going to happen on the push and that's enough.

utils/check_setup.py Outdated Show resolved Hide resolved
.github/workflows/check_dependencies.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@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.

Great check! Left a comment about why we check setup on pull_request.
Thank you, @muellerzr !

Would be nice if you can test this PR works expected (change setup.py, for example). You might need to create a branch like test_ci_XXX, and modify the workflow files to be triggered on the experimentation branch(es). This is just a suggestion though.

(sorry, I guess I am the one who freaks out, haha)

.github/workflows/check_dependencies.yml Outdated Show resolved Hide resolved

build-docker-containers:
needs: check-for-setup
if: (github.event_name == 'push') && (needs.check-for-setup.outputs.changed == '1')
Copy link
Contributor

Choose a reason for hiding this comment

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

Very nice - we should build the image only for the push event (to main).

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I meanly mean push in this remark, and you are doing it as what I would expect. (to main is to make it more clear only 馃槃 )

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

Choose a reason for hiding this comment

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

I was thinking that completed means the CI will be run only if the workflow Check for dependency modification is successful.

After reading this doc, it is not the case. And this is good, as we want to run the push CI anyway (even if something wrong in the setup check).

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup exactly

@muellerzr
Copy link
Collaborator Author

muellerzr commented Jun 4, 2022

@ydshieh you can see my almost 250 debug logs here: https://github.com/muellerzr/accelerate/actions

Very very comfortable with this working

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants