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

add model workflows #304

Merged
merged 1 commit into from
Apr 22, 2024
Merged

add model workflows #304

merged 1 commit into from
Apr 22, 2024

Conversation

sallyom
Copy link
Collaborator

@sallyom sallyom commented Apr 22, 2024

adding merlinite & granite model workflow

Signed-off-by: sallyom <somalley@redhat.com>
Copy link
Collaborator

@Gregory-Pereira Gregory-Pereira left a comment

Choose a reason for hiding this comment

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

@sallyom firstly this is awesome stuff. Also my appologies this should have been included with #300 so thanks for adding this! However I would like to move to a matrix based approach with all our workflows as it is easier and safer to extend. You dont have to take the following suggestions but I highly recommend:

Comment on lines +12 to +20
MERLINITE_MODEL_IMAGE_NAME: merlinite-7b-lab
MERLINITE_LABEL: Q4_K_M
MERLINITE_MODEL_URL: https://huggingface.co/instructlab/merlinite-7b-lab-GGUF/resolve/main/merlinite-7b-lab-Q4_K_M.gguf
MISTRAL_MODEL_IMAGE_NAME: mistral-7b-instruct
MISTRAL_LABEL: v0.1.Q4_K_M.gguf
MISTRAL_MODEL_URL: https://huggingface.co/TheBloke/Mistral-7B-Instruct-v0.1-GGUF/resolve/main/mistral-7b-instruct-v0.1.Q4_K_M.gguf
GRANITE_MODEL_IMAGE_NAME: granite-7b-lab
GRANITE_LABEL: Q4_K_M
GRANITE_MODEL_URL: https://huggingface.co/instructlab/granite-7b-lab-GGUF/resolve/main/granite-7b-lab-Q4_K_M.gguf
Copy link
Collaborator

@Gregory-Pereira Gregory-Pereira Apr 22, 2024

Choose a reason for hiding this comment

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

We should do these using our make targets for greater test coverage via a matrix rather than hardcoding values into the ENV, see example in ms workflows

Suggested change
MERLINITE_MODEL_IMAGE_NAME: merlinite-7b-lab
MERLINITE_LABEL: Q4_K_M
MERLINITE_MODEL_URL: https://huggingface.co/instructlab/merlinite-7b-lab-GGUF/resolve/main/merlinite-7b-lab-Q4_K_M.gguf
MISTRAL_MODEL_IMAGE_NAME: mistral-7b-instruct
MISTRAL_LABEL: v0.1.Q4_K_M.gguf
MISTRAL_MODEL_URL: https://huggingface.co/TheBloke/Mistral-7B-Instruct-v0.1-GGUF/resolve/main/mistral-7b-instruct-v0.1.Q4_K_M.gguf
GRANITE_MODEL_IMAGE_NAME: granite-7b-lab
GRANITE_LABEL: Q4_K_M
GRANITE_MODEL_URL: https://huggingface.co/instructlab/granite-7b-lab-GGUF/resolve/main/granite-7b-lab-Q4_K_M.gguf

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

matrix approach works for me! it'll be much better

id: build_granite_model_image
uses: redhat-actions/buildah-build@v2
with:
image: ${{ env.GRANITE_MODEL_IMAGE_NAME }}
Copy link
Collaborator

@Gregory-Pereira Gregory-Pereira Apr 22, 2024

Choose a reason for hiding this comment

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

Based on my above suggestion this will move to a matrix based approach, ex ms model downloads via matrix

platforms: linux/amd64, linux/arm64
context: models
labels: |
${{ env.GRANITE_LABEL }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another env --> matrix refactor

labels: |
${{ env.GRANITE_LABEL }}
build-args: |
MODEL_URL=${{ env.GRANITE_MODEL_URL }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another env --> matrix refactor

Comment on lines +147 to +155
image: ${{ env.MERLINITE_MODEL_IMAGE_NAME }}
tags: latest
platforms: linux/amd64, linux/arm64
context: models
labels: |
${{ env.MERLINITE_LABEL }}
build-args: |
MODEL_URL=${{ env.MERLINITE_MODEL_URL }}
containerfiles: ./models/Containerfile
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as discussed above

@Gregory-Pereira
Copy link
Collaborator

One thing you should also do is include is more targeted on policy to specify what we want to test for. Here is an of what this looks like for our chatbot workflow

It should look something like this:

on:
  schedule: # schedule the job to run at 12 AM daily
   - cron: '0 0 * * *'
   
  pull_request:
    branches:
      - main
    paths:
      - ./models/Makefile
      - .github/workflows/model_image_build_push.yaml
  push:
    branches:
      - main
    paths:
      - ./models/Makefile
      - .github/workflows/model_image_build_push.yaml

@Gregory-Pereira
Copy link
Collaborator

It was determined we want to push ahead and get these images in Quay ASAP. I will follow on with a refactor PR adressing the concerns laid out above.

Copy link
Collaborator

@Gregory-Pereira Gregory-Pereira left a comment

Choose a reason for hiding this comment

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

/lgtm
@Gregory-Pereira to refactor after images get created

@sallyom
Copy link
Collaborator Author

sallyom commented Apr 22, 2024

@Gregory-Pereira is going to refactor this and make it 1000% better, I'm merging this as/is to populate these images

@sallyom sallyom merged commit 7397906 into containers:main Apr 22, 2024
1 check passed
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