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

Restructure workflow files #20

Merged
merged 2 commits into from
Feb 29, 2024
Merged

Restructure workflow files #20

merged 2 commits into from
Feb 29, 2024

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Feb 28, 2024

The goal is to restructure the workflows to enable combining the image building/pushing with the testing.
Some tiny modifications are made (like using secrets: inherit etc)

A run

@ydshieh ydshieh mentioned this pull request Feb 28, 2024
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the workflow to be triggered. We can specify the (list) of torch, transformers and python version here. Easy to extend to include more parameters.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the workflow file to combine image building/pushing with testing

@ydshieh
Copy link
Collaborator Author

ydshieh commented Feb 28, 2024

Once this is merged. The last piece is to include 2 or 3 training examples (Mistral, Llama etc.) in the testing and we are good (at least good as a starting)

Copy link
Contributor

@shub-kris shub-kris 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 Nice work @ydshieh. Have very small comments.

.github/workflows/build_and_test.yml Show resolved Hide resolved
.github/workflows/test_model.yml Outdated Show resolved Hide resolved
on:
push:
branches:
- build_images_and_run_tests_*
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is just for now, but you will change it to whenever someone creates a PR or push to main

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, either the author following this pattern, or they change this line to their own branch name

Copy link
Contributor

Choose a reason for hiding this comment

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

  • If someone needs to change the line to their own branch name then I guess it will change the workflow all the time which I think shouldn't be the way.
  • Matching this pattern is also I think not a good way as then you are forcing people to have their branch name starting with build_images_and_run_tests_* all the time, which they cab easily forget when working.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I am not sure if we can use dynamic branch name here. And even if this is doable, it's also dangerous for people to easily push the images.

In transformers, the images are used for CI regularly. If anyone pushes an image with the tags as they are while the dockerfile or codebase being modified to produce different content in those images, the CI are likely to break while no one knows what happen (it happens!)

For this repository, the situation might be different, and we can discuss what's the practice to follow. But for this moment, let's probably move forward and finalize the (first) version with adding some examples with mistral and/or llama 😄

@ydshieh ydshieh merged commit 0618bbb into main Feb 29, 2024
2 checks passed
@philschmid philschmid deleted the build_images_and_run_tests_001 branch April 16, 2024 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants