Address 3 MAJOR review findings on the docker PR

1. Stop leaking secrets via docker run -e VAR=VALUE argv (run.sh, test_locally.sh)

   `docker run ... -e HF_TOKEN=hf_xxx ...` puts the literal token in
   the docker CLI's argv, which is visible to any user on the host
   via `ps auxe` / `/proc/<pid>/cmdline` for the lifetime of the
   process. Switch to the dash-only form `-e HF_TOKEN`, which tells
   docker to read the value from the parent shell's env and never
   appears in argv. Same fix for WANDB_API_KEY and UNSLOTH_LICENSE in
   run.sh and HF_TOKEN in test_locally.sh.

2. Stop stripping numpy/tests/ in the runtime layer (Dockerfile)

   The Dockerfile explicitly upgrades numpy >= 2.4 because numpy 2.2.6
   shipped a stripped wheel where `from numpy._core.tests._natype
   import pd_NA` fails. Numpy 2.4 restores `numpy/_core/tests/`, then
   the existing `find ${VENV} -name tests -exec rm -rf {} +` deleted
   it again -- re-introducing the same broken-import state on the
   deployed image (the build-time verification at line 220 runs
   BEFORE the strip so it passed). Whitelist numpy's tests directories
   from the strip; keep stripping the rest.

3. Align :latest tag gate between merge and smoke-test jobs
   (.github/workflows/docker-publish.yml)

   merge job:       enable = is-default-branch AND unsloth_ref == ''
   smoke-test job:  enable = is_default_branch only

   On `workflow_dispatch`, `github.event.inputs.unsloth_ref` defaults to
   "main" (not ""), so the merge step skipped `:latest` but the smoke
   step still emitted `:latest` as tags[0]. The smoke step then
   `docker pull`-ed a prior `:latest` from Docker Hub instead of the
   image just merged -- so the smoke test verified the OLD image, not
   the new one. Copy the merge step's exact `enable=` expression into
   the smoke-test step so the two stay byte-identical and a workflow_
   dispatch run validates whatever was actually merged.
This commit is contained in:
Daniel Han 2026-05-25 13:36:56 +00:00
commit cceeeb1e1b
4 changed files with 33 additions and 6 deletions

View file

@ -216,13 +216,20 @@ jobs:
# Re-compute the tag list deterministically from the same metadata-action
# config the merge job used, so tag/schedule/SHA runs pull the image
# they just published instead of an unrelated `:latest` from a prior run.
# IMPORTANT: keep this `enable=` expression byte-identical to the merge
# job's :latest gate above. The two used to differ
# (merge: ref + unsloth_ref guard; smoke: is_default_branch only),
# which meant workflow_dispatch with unsloth_ref defaulting to "main"
# would skip :latest on merge but still emit :latest as tags[0] on
# smoke -- so docker pull would fetch a previously-published :latest
# from Docker Hub, not the image just merged.
- name: Resolve published tag
id: meta
uses: docker/metadata-action@v5
with:
images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}
tags: |
type=raw,value=latest,enable={{is_default_branch}}
type=raw,value=latest,enable=${{ github.ref == format('refs/heads/{0}', github.event.repository.default_branch) && github.event.inputs.unsloth_ref == '' }}
type=ref,event=tag
type=schedule,pattern=nightly
type=sha,prefix=sha-,format=short

View file

@ -230,8 +230,20 @@ RUN ${VENV}/bin/pip freeze --exclude-editable > ${VENV}/requirements.lock.txt \
&& head -50 ${VENV}/requirements.lock.txt
# 6) Strip pip cache & __pycache__ to shrink the layer copied to runtime.
#
# Note on the `-name tests` strip: numpy 2.4 ships `numpy/_core/tests/`
# back into the wheel (numpy 2.2.6 had stripped it, which is what the
# explicit upgrade earlier in this Dockerfile was meant to fix). Blowing
# away every `tests/` directory under the venv would re-introduce the
# same `from numpy._core.tests._natype import pd_NA` ImportError on the
# deployed image. Exclude numpy's tests directories explicitly so the
# upgrade fix stays in effect; keep stripping the rest.
RUN find ${VENV} -depth -type d -name __pycache__ -exec rm -rf {} + \
&& find ${VENV} -depth -type d -name tests -exec rm -rf {} + \
&& find ${VENV} -depth -type d -name tests \
! -path "*numpy/_core/tests*" \
! -path "*numpy/tests*" \
! -path "*numpy/ma/tests*" \
-exec rm -rf {} + \
&& rm -rf /root/.cache/pip /root/.cache/uv
# Build-time verification.

View file

@ -51,10 +51,15 @@ fi
# Forward common secrets only if they're set in the host environment.
# Empty strings would shadow whatever is already inside the image.
# IMPORTANT: use the dash-only form `-e VAR` (no `=VALUE`). Docker reads
# the value from the parent shell, so the literal secret never lands in
# argv where it would be visible to any user on the host via
# `ps auxe` / `/proc/<pid>/cmdline` for the lifetime of the docker CLI
# process.
declare -a ENV_FORWARD=(-e HF_HUB_ENABLE_HF_TRANSFER=1)
[[ -n "${HF_TOKEN:-}" ]] && ENV_FORWARD+=(-e "HF_TOKEN=${HF_TOKEN}")
[[ -n "${WANDB_API_KEY:-}" ]] && ENV_FORWARD+=(-e "WANDB_API_KEY=${WANDB_API_KEY}")
[[ -n "${UNSLOTH_LICENSE:-}" ]] && ENV_FORWARD+=(-e "UNSLOTH_LICENSE=${UNSLOTH_LICENSE}")
[[ -n "${HF_TOKEN:-}" ]] && ENV_FORWARD+=(-e HF_TOKEN)
[[ -n "${WANDB_API_KEY:-}" ]] && ENV_FORWARD+=(-e WANDB_API_KEY)
[[ -n "${UNSLOTH_LICENSE:-}" ]] && ENV_FORWARD+=(-e UNSLOTH_LICENSE)
# Only attach -t when our own stdin/stdout are a TTY; CI / piped invocations
# otherwise hit `the input device is not a TTY` and never reach the entrypoint.

View file

@ -362,8 +362,11 @@ INNER
# Only forward HF_TOKEN if the host has one set, so an empty
# `-e HF_TOKEN=` does not shadow whatever is already inside the image.
# Use the dash-only form `-e HF_TOKEN` so the secret value never
# lands in argv (visible via /proc/<pid>/cmdline to any user on
# the host for the lifetime of the docker CLI process).
HF_ARGS=()
[[ -n "${HF_TOKEN:-}" ]] && HF_ARGS+=(-e "HF_TOKEN=${HF_TOKEN}")
[[ -n "${HF_TOKEN:-}" ]] && HF_ARGS+=(-e HF_TOKEN)
docker run --rm \
--gpus all \
--ipc=host \