Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis change introduces UV-based Docker image builds and corresponding CI/CD infrastructure. Two new GitHub Actions jobs ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
docker/Dockerfile-cloud-uv (2)
17-27: Minor: preferapt-getoveraptin Dockerfiles.
aptis designed for interactive use and may produce warnings in non-interactive (Dockerfile) contexts.apt-getis the standard for scripted/Dockerfile usage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/Dockerfile-cloud-uv` around lines 17 - 27, The RUN instruction that currently uses apt should use apt-get for non-interactive Dockerfile usage: change the sequence "apt update && apt install --yes --no-install-recommends ..." to use "apt-get update && apt-get install -y --no-install-recommends ..." (preserve the subsequent rm -rf and chmod/printf steps), and ensure the install uses the short -y flag; update any other occurrences of apt in the same RUN command to apt-get to avoid interactive warnings.
15-16: Usespipinstead ofuv pip— inconsistent with the UV image.This is a UV-based Dockerfile but Jupyter packages are installed with plain
pip. Consider usinguv pip installfor consistency and faster installs, matching the base image's tooling.♻️ Suggested fix
-RUN pip install jupyterlab notebook ipywidgets && \ - jupyter lab clean +RUN uv pip install jupyterlab notebook ipywidgets && \ + jupyter lab clean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/Dockerfile-cloud-uv` around lines 15 - 16, Replace the plain pip invocation in the Dockerfile RUN step with the UV wrapper so installs use the base image's tooling: change the RUN command that currently calls "pip install jupyterlab notebook ipywidgets && jupyter lab clean" to use "uv pip install ..." (preserving the same package list and the subsequent "jupyter lab clean") so installation runs via the UV environment and matches the base image's optimized installer.docker/Dockerfile-uv (1)
30-34: Quote the extras bracket expression to prevent shell glob expansion.The
.[...]syntax for pip extras uses square brackets, which are shell glob characters. If any filename in the working directory happens to match the pattern, the shell will expand it instead of passing it literally. Quoting the argument prevents this.♻️ Proposed fix
if [ "$AXOLOTL_EXTRAS" != "" ]; then \ - uv pip install --no-build-isolation -e .[$BASE_EXTRAS,$AXOLOTL_EXTRAS] $AXOLOTL_ARGS; \ + uv pip install --no-build-isolation -e ".[$BASE_EXTRAS,$AXOLOTL_EXTRAS]" $AXOLOTL_ARGS; \ else \ - uv pip install --no-build-isolation -e .[$BASE_EXTRAS] $AXOLOTL_ARGS; \ + uv pip install --no-build-isolation -e ".[$BASE_EXTRAS]" $AXOLOTL_ARGS; \ fi && \🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/Dockerfile-uv` around lines 30 - 34, The pip editable install argument .[...] can be subject to shell glob expansion; update both uv pip install lines to quote the extras expression so the brackets are passed literally (e.g. change -e .[$BASE_EXTRAS,$AXOLOTL_EXTRAS] and -e .[$BASE_EXTRAS] to -e ".[...]" preserving the variable contents and $AXOLOTL_ARGS). Ensure you only add quotes around the .[...] portion (not around $AXOLOTL_ARGS) so the two uv pip install invocations use -e ".[${BASE_EXTRAS},${AXOLOTL_EXTRAS}]" and -e ".[${BASE_EXTRAS}]" respectively.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/main.yml:
- Around line 101-170: The matrix for the build-axolotl-uv job references `${{
matrix.axolotl_args }}` but no `axolotl_args` key is defined; update each matrix
include entry in the build-axolotl-uv job to add an explicit `axolotl_args: ""`
(empty string or appropriate default) so static analysis sees the key, and apply
the same change to the `build-axolotl` job's matrix entries as suggested; locate
the matrix entries by the job name `build-axolotl-uv` (and `build-axolotl`) and
add `axolotl_args:` to each include block.
In `@docker/Dockerfile-cloud-uv`:
- Around line 4-6: HF_HOME is set incorrectly to the same path as HF_HUB_CACHE
causing other HF caches to nest under hub; change HF_HOME to the parent cache
dir (e.g. set HF_HOME to /workspace/data/huggingface-cache) and keep
HF_HUB_CACHE as the hub subdirectory (/workspace/data/huggingface-cache/hub),
leaving HF_DATASETS_CACHE as /workspace/data/huggingface-cache/datasets so
tokens and other caches live directly under HF_HOME rather than inside hub.
---
Nitpick comments:
In `@docker/Dockerfile-cloud-uv`:
- Around line 17-27: The RUN instruction that currently uses apt should use
apt-get for non-interactive Dockerfile usage: change the sequence "apt update &&
apt install --yes --no-install-recommends ..." to use "apt-get update && apt-get
install -y --no-install-recommends ..." (preserve the subsequent rm -rf and
chmod/printf steps), and ensure the install uses the short -y flag; update any
other occurrences of apt in the same RUN command to apt-get to avoid interactive
warnings.
- Around line 15-16: Replace the plain pip invocation in the Dockerfile RUN step
with the UV wrapper so installs use the base image's tooling: change the RUN
command that currently calls "pip install jupyterlab notebook ipywidgets &&
jupyter lab clean" to use "uv pip install ..." (preserving the same package list
and the subsequent "jupyter lab clean") so installation runs via the UV
environment and matches the base image's optimized installer.
In `@docker/Dockerfile-uv`:
- Around line 30-34: The pip editable install argument .[...] can be subject to
shell glob expansion; update both uv pip install lines to quote the extras
expression so the brackets are passed literally (e.g. change -e
.[$BASE_EXTRAS,$AXOLOTL_EXTRAS] and -e .[$BASE_EXTRAS] to -e ".[...]" preserving
the variable contents and $AXOLOTL_ARGS). Ensure you only add quotes around the
.[...] portion (not around $AXOLOTL_ARGS) so the two uv pip install invocations
use -e ".[${BASE_EXTRAS},${AXOLOTL_EXTRAS}]" and -e ".[${BASE_EXTRAS}]"
respectively.
| build-axolotl-uv: | ||
| if: ${{ ! contains(github.event.commits[0].message, '[skip docker]') && github.repository_owner == 'axolotl-ai-cloud' }} | ||
| strategy: | ||
| fail-fast: false | ||
| matrix: | ||
| include: | ||
| - cuda: 128 | ||
| cuda_version: 12.8.1 | ||
| python_version: "3.11" | ||
| pytorch: 2.9.1 | ||
| axolotl_extras: | ||
| platforms: "linux/amd64,linux/arm64" | ||
| is_latest: true | ||
| - cuda: 128 | ||
| cuda_version: 12.8.1 | ||
| python_version: "3.12" | ||
| pytorch: 2.10.0 | ||
| axolotl_extras: | ||
| platforms: "linux/amd64,linux/arm64" | ||
| - cuda: 130 | ||
| cuda_version: 13.0.0 | ||
| python_version: "3.11" | ||
| pytorch: 2.9.1 | ||
| axolotl_extras: | ||
| platforms: "linux/amd64,linux/arm64" | ||
| - cuda: 130 | ||
| cuda_version: 13.0.0 | ||
| python_version: "3.12" | ||
| pytorch: 2.10.0 | ||
| axolotl_extras: | ||
| platforms: "linux/amd64,linux/arm64" | ||
| runs-on: axolotl-gpu-runner | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| - name: Docker metadata | ||
| id: metadata | ||
| uses: docker/metadata-action@v5 | ||
| with: | ||
| images: | | ||
| axolotlai/axolotl-uv | ||
| tags: | | ||
| type=ref,event=branch | ||
| type=pep440,pattern={{version}} | ||
| - name: Set up Docker Buildx | ||
| uses: docker/setup-buildx-action@v3 | ||
| - name: Login to Docker Hub | ||
| uses: docker/login-action@v3 | ||
| with: | ||
| username: ${{ secrets.DOCKERHUB_USERNAME }} | ||
| password: ${{ secrets.DOCKERHUB_TOKEN }} | ||
| # guidance for testing before pushing: https://docs.docker.com/build/ci/github-actions/test-before-push/ | ||
| - name: Build and export to Docker | ||
| uses: docker/build-push-action@v5 | ||
| with: | ||
| context: . | ||
| platforms: ${{ matrix.platforms }} | ||
| build-args: | | ||
| BASE_TAG=${{ github.ref_type == 'tag' && 'main' || github.ref_name }}-base-py${{ matrix.python_version }}-cu${{ matrix.cuda }}-${{ matrix.pytorch }} | ||
| CUDA=${{ matrix.cuda }} | ||
| PYTORCH_VERSION=${{ matrix.pytorch }} | ||
| AXOLOTL_ARGS=${{ matrix.axolotl_args }} | ||
| AXOLOTL_EXTRAS=${{ matrix.axolotl_extras}} | ||
| file: ./docker/Dockerfile-uv | ||
| push: ${{ github.event_name != 'pull_request' }} | ||
| tags: | | ||
| ${{ steps.metadata.outputs.tags }}-py${{ matrix.python_version }}-cu${{ matrix.cuda }}-${{ matrix.pytorch }}${{ matrix.axolotl_extras != '' && '-' || '' }}${{ matrix.axolotl_extras }} | ||
| ${{ steps.metadata.outputs.tags }}-py${{ matrix.python_version }}-cu${{ matrix.cuda }}-${{ matrix.pytorch }} | ||
| ${{ (matrix.is_latest) && format('{0}-latest', steps.metadata.outputs.tags) || '' }} | ||
| labels: ${{ steps.metadata.outputs.labels }} |
There was a problem hiding this comment.
matrix.axolotl_args is not defined in the build-axolotl-uv matrix.
Line 162 references ${{ matrix.axolotl_args }} but no matrix entry defines axolotl_args. This resolves to an empty string at runtime, so it's functionally harmless, but it's flagged by static analysis. Note that the existing build-axolotl job (line 91) has the same pattern — consider adding axolotl_args: explicitly to matrix entries in both jobs for clarity, or suppress the lint.
🧰 Tools
🪛 actionlint (1.7.11)
[error] 132-132: label "axolotl-gpu-runner" is unknown. available labels are "windows-latest", "windows-latest-8-cores", "windows-2025", "windows-2025-vs2026", "windows-2022", "windows-11-arm", "ubuntu-slim", "ubuntu-latest", "ubuntu-latest-4-cores", "ubuntu-latest-8-cores", "ubuntu-latest-16-cores", "ubuntu-24.04", "ubuntu-24.04-arm", "ubuntu-22.04", "ubuntu-22.04-arm", "macos-latest", "macos-latest-xlarge", "macos-latest-large", "macos-26-xlarge", "macos-26-large", "macos-26", "macos-15-intel", "macos-15-xlarge", "macos-15-large", "macos-15", "macos-14-xlarge", "macos-14-large", "macos-14", "self-hosted", "x64", "arm", "arm64", "linux", "macos", "windows". if it is a custom label for self-hosted runner, set list of labels in actionlint.yaml config file
(runner-label)
[error] 158-158: property "axolotl_args" is not defined in object type {axolotl_extras: string; cuda: number; cuda_version: string; is_latest: bool; platforms: string; python_version: number; pytorch: string}
(expression)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/main.yml around lines 101 - 170, The matrix for the
build-axolotl-uv job references `${{ matrix.axolotl_args }}` but no
`axolotl_args` key is defined; update each matrix include entry in the
build-axolotl-uv job to add an explicit `axolotl_args: ""` (empty string or
appropriate default) so static analysis sees the key, and apply the same change
to the `build-axolotl` job's matrix entries as suggested; locate the matrix
entries by the job name `build-axolotl-uv` (and `build-axolotl`) and add
`axolotl_args:` to each include block.
| ENV HF_DATASETS_CACHE="/workspace/data/huggingface-cache/datasets" | ||
| ENV HF_HUB_CACHE="/workspace/data/huggingface-cache/hub" | ||
| ENV HF_HOME="/workspace/data/huggingface-cache/hub" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find and check HF_HOME/HF_HUB_CACHE values in both Dockerfile-cloud and Dockerfile-cloud-uv
echo "=== Dockerfile-cloud (non-uv) ==="
fd 'Dockerfile-cloud$' --type f --exec cat {} \; 2>/dev/null | head -50
echo -e "\n=== Dockerfile-cloud-uv ==="
fd 'Dockerfile-cloud-uv$' --type f --exec cat {} \; 2>/dev/null | head -50Repository: axolotl-ai-cloud/axolotl
Length of output: 2411
HF_HOME and HF_HUB_CACHE point to the same path—incorrect configuration.
HF_HOME is the root directory for all HuggingFace caches, while HF_HUB_CACHE should be a subdirectory. Currently both are set to /workspace/data/huggingface-cache/hub, which causes other HF caches (tokens, etc.) to nest incorrectly under the hub/ directory. Set HF_HOME to the parent directory:
Suggested fix
-ENV HF_DATASETS_CACHE="/workspace/data/huggingface-cache/datasets"
-ENV HF_HUB_CACHE="/workspace/data/huggingface-cache/hub"
-ENV HF_HOME="/workspace/data/huggingface-cache/hub"
+ENV HF_HOME="/workspace/data/huggingface-cache"
+ENV HF_HUB_CACHE="/workspace/data/huggingface-cache/hub"
+ENV HF_DATASETS_CACHE="/workspace/data/huggingface-cache/datasets"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ENV HF_DATASETS_CACHE="/workspace/data/huggingface-cache/datasets" | |
| ENV HF_HUB_CACHE="/workspace/data/huggingface-cache/hub" | |
| ENV HF_HOME="/workspace/data/huggingface-cache/hub" | |
| ENV HF_HOME="/workspace/data/huggingface-cache" | |
| ENV HF_HUB_CACHE="/workspace/data/huggingface-cache/hub" | |
| ENV HF_DATASETS_CACHE="/workspace/data/huggingface-cache/datasets" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docker/Dockerfile-cloud-uv` around lines 4 - 6, HF_HOME is set incorrectly to
the same path as HF_HUB_CACHE causing other HF caches to nest under hub; change
HF_HOME to the parent cache dir (e.g. set HF_HOME to
/workspace/data/huggingface-cache) and keep HF_HUB_CACHE as the hub subdirectory
(/workspace/data/huggingface-cache/hub), leaving HF_DATASETS_CACHE as
/workspace/data/huggingface-cache/datasets so tokens and other caches live
directly under HF_HOME rather than inside hub.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1 participant
Summary by CodeRabbit