FIX [CI / Docker] Follow up from #1481
#1487
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
Here is a link of a job that was successful after modifying two dockerfiles to use python 3.10 instead of python 3.8: https://github.com/huggingface/peft/actions/runs/7969411897/job/21755184945 |
Co-authored-by: Guillaume LEGENDRE <glegendre01@gmail.com>
There was a problem hiding this comment.
Nice! A few nits and one question regarding security (despite @glegendre01 already approved)
|
|
||
| get_changed_files: | ||
| name: "Build all modified docker images" | ||
| runs-on: ubuntu-latest |
There was a problem hiding this comment.
Could use [intel-cpu, 8-cpu, ci] to avoid any potential disk full issue
| - name: Cleanup disk | ||
| run: | | ||
| sudo ls -l /usr/local/lib/ | ||
| sudo ls -l /usr/share/ | ||
| sudo du -sh /usr/local/lib/ | ||
| sudo du -sh /usr/share/ | ||
| sudo rm -rf /usr/local/lib/android | ||
| sudo rm -rf /usr/share/dotnet | ||
| sudo du -sh /usr/local/lib/ | ||
| sudo du -sh /usr/share/ |
There was a problem hiding this comment.
no need this once using [intel-cpu, 8-cpu, ci]
There was a problem hiding this comment.
not sure if peft have access to those :/
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v1 | ||
| - name: Check out code | ||
| uses: actions/checkout@v3 | ||
| - name: Build Docker image | ||
| uses: docker/build-push-action@v4 |
There was a problem hiding this comment.
having one empty line between each step would be nice to eyes 👀
| uses: docker/build-push-action@v4 | ||
| with: | ||
| file: ${{ matrix.docker-file }} | ||
| context: . |
There was a problem hiding this comment.
I am not so familiar, but probably we don't need context here if file is specified
| @@ -1,42 +1,61 @@ | |||
| name: Test Docker images (scheduled) | |||
| name: Test Docker images (on PR) | |||
|
|
|||
| on: | |||
| pull_request: | |||
There was a problem hiding this comment.
I am not sure if we really want to test this in each PR (all of its commits), or just when merged into main.
I know it's better to keep PR green. But for workflow run on each PR, we have to manually approve the run (for security run), right?
ping @glegendre01 again to see if the infra team prefers not to allow rununing on PRs whenever possible, or it doesn't really matter.
There was a problem hiding this comment.
I think as long as we don't push the docker image on docker hub it's all good, from the conversation I had with guillaume !
* Update test-docker-build.yml * Update test-docker-build.yml * Update Dockerfile * Update test-docker-build.yml * Update test-docker-build.yml * Update Dockerfile * Update .github/workflows/test-docker-build.yml * Update .github/workflows/test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update .github/workflows/test-docker-build.yml * Update Dockerfile * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * Update test-docker-build.yml * revert * Update .github/workflows/test-docker-build.yml Co-authored-by: Guillaume LEGENDRE <glegendre01@gmail.com> --------- Co-authored-by: Guillaume LEGENDRE <glegendre01@gmail.com>
As per title, this PR provides some fixes so that #1481 works as intended