docs(training_guides): e2e harness + verified doc fixes - #259
Merged
Merged
Conversation
WalkthroughThis PR adds Kueue configuration examples with documentation, updates TrainingRuntime manifests to source Ascend CANN environment setup, and introduces comprehensive end-to-end testing with a shared Bash library and 11 test cases across GPU and NPU clusters. ChangesKueue Configuration and Documentation
TrainingRuntime CANN Environment Setup
End-to-End Testing Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying alauda-ai with
|
| Latest commit: |
03099d5
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://626cad5c.alauda-ai.pages.dev |
| Branch Preview URL: | https://codex-training-guides-e2e-ve.alauda-ai.pages.dev |
Verified that the three NPU notebooks' base environments work end-to-end once their workbench images are pulled directly from `docker.io/alaudadockerhub/...` (the previously-attempted mirror `docker-mirrors.alauda.cn` consistently returned EOF on large arm64 blobs). - C8 fine-tune-with-trainer-v2-mindspeed-npu.ipynb → PASS (env smoke) - C9 qwen3_finetune_verify.ipynb → PASS (env smoke) - C10 qwen25_pretrain_verify.ipynb → covered by C9 - C11 qwen3_0.6b_finetune_verify.ipynb → PASS (env smoke) Each smoke replicates the notebook's "cell 1" environment check — source the CANN env scripts, import torch/mindspore + torch_npu/msadapter + mindspeed + mindspeed_llm, confirm the NPU is visible, and do a small matmul on the device. The C11 fix surfaced a subtle bundling-order constraint: `set_path.sh` deliberately puts `MindSpeed-LLM/` ahead of `MindSpeed/` on PYTHONPATH so the msadapter-aware patched copy of `mindspeed` wins; layering extra PYTHONPATH entries on top of it inverts the order and loads the upstream-unpatched `MindSpeed/mindspeed/`, which hits a hard `from torch.library import Library` that msadapter doesn't expose. The full HF→MCore conversion + training paths still need real Qwen3-0.6B / Qwen3-8B / Qwen2.5-7B HF checkpoints and remain tracked in e2e/TODO.md.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
docs/en/training_guides/assets/training-runtimes/llamafactory0.9-cann8.5-arm64-trainingruntime.yaml (1)
43-44: ⚖️ Poor tradeoffConsider the more defensive sourcing pattern from e2e tests.
The e2e test cases (c8, c9) use a loop that checks for file existence across multiple possible CANN paths before sourcing:
set +e for f in /usr/local/Ascend/cann/set_env.sh /usr/local/Ascend/ascend-toolkit/set_env.sh /usr/local/Ascend/nnal/atb/set_env.sh; do [ -f "$f" ] && source "$f" done set -eThis approach gracefully handles variations in CANN installation paths. The current YAML assumes
/usr/local/Ascend/ascend-toolkit/set_env.shalways exists and will fail if it's missing or at a different location. While the documentation explicitly states the expected path and testing confirms it works, adopting the loop pattern would make the runtime more robust against environment variations.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/en/training_guides/assets/training-runtimes/llamafactory0.9-cann8.5-arm64-trainingruntime.yaml` around lines 43 - 44, Replace the two unconditional source lines that reference /usr/local/Ascend/ascend-toolkit/set_env.sh and /usr/local/Ascend/nnal/atb/set_env.sh with a defensive loop that checks multiple possible CANN env script locations (e.g., /usr/local/Ascend/cann/set_env.sh, /usr/local/Ascend/ascend-toolkit/set_env.sh, /usr/local/Ascend/nnal/atb/set_env.sh) and only sources existing files; wrap the loop with set +e before it and set -e after it so missing files don’t cause the script to fail — update the block in llamafactory0.9-cann8.5-arm64-trainingruntime.yaml where those source lines appear.e2e/cases/c11_qwen3_06b_mindspore.sh (1)
110-118: 💤 Low valueConsider adding an explicit pod-not-found check.
Similar to c9, this script doesn't explicitly exit when no pod appears after the timeout. While the final phase check at line 131 will still fail, an explicit early exit improves clarity.
♻️ Suggested improvement
sleep 5 done +[ -z "${POD}" ] && { log "C11: no pod appeared"; exit 1; } log "C11: pod=${POD}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/cases/c11_qwen3_06b_mindspore.sh` around lines 110 - 118, The pod-wait loop sets POD but doesn't explicitly fail when no pod is found; after the while loop that assigns POD using npu_kc and before continuing (after log "C11: pod=${POD}"), add an explicit check for an empty POD (check the POD variable) and if empty call log with a clear error (e.g., "C11: no pod found after timeout") and exit with a non-zero status; reference the variables POD, deadline, SECONDS, NS, JOB_NAME and the log helper so you place the check immediately after the loop and before subsequent steps.e2e/cases/c9_qwen3_finetune_verify.sh (1)
94-102: 💤 Low valueConsider adding an explicit pod-not-found check.
Unlike c7 (line 46) and c8 (line 144), this script doesn't explicitly exit when no pod appears after the timeout. While the final phase check at line 115 will still fail, an explicit early exit with a clear message improves debuggability.
♻️ Suggested improvement
sleep 5 done +[ -z "${POD}" ] && { log "C9: no pod appeared"; exit 1; } log "C9: pod=${POD}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/cases/c9_qwen3_finetune_verify.sh` around lines 94 - 102, After the pod-wait loop that uses deadline, POD, npu_kc, JOB_NAME and NS, add an explicit check for an empty POD and fail fast: if POD is empty after the loop, call log with a clear message including JOB_NAME and NS, then exit non-zero so the script stops early instead of proceeding to later checks; place this check immediately after the existing while loop and before the existing log "C9: pod=${POD}" line.e2e/lib.sh (2)
64-64: 💤 Low valueReading all stdin into memory may fail for large YAML manifests.
The function reads the entire stdin into a bash variable, which could consume excessive memory for large YAML files (e.g., manifests with embedded data or large ConfigMaps). While this allows retries, it creates a scalability concern.
Consider documenting a maximum expected manifest size, or if large files are anticipated, use a temporary file instead:
Alternative approach using temp file
- local data - data="$(cat)" + local tmpfile + tmpfile="$(mktemp)" + trap "rm -f '${tmpfile}'" RETURN + cat > "${tmpfile}" local attempts=0 max=20 delay=30 rc out while [ "${attempts}" -lt "${max}" ]; do - if out="$(printf '%s' "${data}" | $kfn "${verb}" -f - "$@" 2>&1)"; then + if out="$($kfn "${verb}" -f "${tmpfile}" "$@" 2>&1)"; then🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/lib.sh` at line 64, The current code reads all stdin into the bash variable data via data="$(cat)", which can OOM for large YAML manifests; change the implementation to stream stdin into a temporary file instead (use mktemp to create a file, cat > "$TMPFILE" or redirect stdin to it), update all places that reference the in-memory variable (data) to read from the temp file (e.g., pass the filename to retry loops or commands), ensure the temp file is removed on exit (trap 'rm -f "$TMPFILE"' EXIT), and add a short comment documenting expected max manifest size or that a temp file is used for large inputs.
72-72: Keep the current transient-error retry regex (no evidence of missing webhook/TLS variants in this repo)
Ine2e/lib.sh(_retry_kubectl_stdin, line 72), the retry regex includesfailed calling webhook|x509|connection refused|EOF|context deadline exceeded|webhook.* connect: connection refused. The targetede2e/logssearch returnederror: context deadline exceededbut no webhook/TLS/x509/connection-refused variants, so there’s no current evidence the pattern is incomplete for this codebase; expand it only when new message forms show up in future logs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/lib.sh` at line 72, Leave the transient-error retry regex in _retry_kubectl_stdin unchanged; the current pattern (matching failed calling webhook|x509|connection refused|EOF|context deadline exceeded|webhook.* connect: connection refused) already covers the observed "context deadline exceeded" and there is no evidence of missing webhook/TLS/x509 variants in this repo, so do not expand or modify the regex now—only revisit and broaden it if new log message formats appear in future runs.e2e/TODO.md (1)
33-33: 💤 Low valueDocument the DeepSpeed workaround limitation.
Line 33 mentions that DeepSpeed's CUDA check fails on images without the toolkit, and notes that C5 works around this with a stub module. While the TODO correctly identifies the "cleaner fix" (baking nvcc into the image), it would be helpful to document whether the stub workaround has any functional limitations (e.g., does it prevent JIT compilation of custom ops?).
Consider adding a sentence about the functional impact of the stub workaround:
Suggested clarification
-5. `traininghub0.1-cu126-amd64` and `llamafactory0.9-cu126-amd64` ship CUDA runtime but not the toolkit. DeepSpeed's import-time CUDA check fails on both with `MissingCUDAException: CUDA_HOME does not exist`. C5 works around this with a stub `deepspeed` module on `PYTHONPATH`; the cleaner fix is to bake the CUDA toolkit (or at least `nvcc`) into the runtime image so DeepSpeed JIT op compilation works as advertised. +5. `traininghub0.1-cu126-amd64` and `llamafactory0.9-cu126-amd64` ship CUDA runtime but not the toolkit. DeepSpeed's import-time CUDA check fails on both with `MissingCUDAException: CUDA_HOME does not exist`. C5 works around this with a stub `deepspeed` module on `PYTHONPATH` that bypasses the check but disables JIT custom op compilation; the cleaner fix is to bake the CUDA toolkit (or at least `nvcc`) into the runtime image so DeepSpeed JIT op compilation works as advertised.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/TODO.md` at line 33, Update the sentence that describes DeepSpeed's import-time CUDA check failing on images without the toolkit (the paragraph mentioning traininghub0.1-cu126-amd64, llamafactory0.9-cu126-amd64 and C5's stub) to explicitly state the functional limitation of the C5 workaround: note that the stub deepspeed module only bypasses the import-time check and does not provide an actual CUDA toolchain (nvcc), so JIT compilation of DeepSpeed/custom CUDA ops will fail and any features relying on compiled kernels are not supported; mention nvcc as the preferred fix.e2e/cases/c4_traininghub_osft.sh (1)
183-187: ⚡ Quick winInconsistent failure diagnostics: missing container logs tail.
On failure, this script dumps pod state and describe but omits container logs. For consistency with C3 (which includes
logs --tail=200on line 212), consider adding a logs tail here to aid debugging.📋 Suggested addition
if [[ "${status}" != *Complete* ]]; then log "C4: ==== pod final state ====" gpu_kc -n "${NS}" get pod -l "job-name=${JOB_NAME}" -o wide 2>&1 | tail -5 || true + log "C4: ==== container logs ====" + gpu_kc -n "${NS}" logs -l "job-name=${JOB_NAME}" --tail=200 2>&1 || true gpu_kc -n "${NS}" describe pod -l "job-name=${JOB_NAME}" 2>&1 | tail -40 || true fi🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/cases/c4_traininghub_osft.sh` around lines 183 - 187, On failure when status != *Complete*, add a container logs tail to match C3: after the pod describe call in the block that checks "${status}", call gpu_kc -n "${NS}" logs -l "job-name=${JOB_NAME}" --tail=200 (or equivalent) to dump recent container logs; update the failure diagnostics block containing gpu_kc get pod and gpu_kc describe pod so it also captures logs for the same label selector (JOB_NAME) and namespace (NS).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/en/training_guides/fine-tune-with-trainer-v2.mdx`:
- Around line 43-45: The sed commands currently use a no-op placeholder
substitution ('s/<your-namespace>/<your-namespace>/')—update them so users
replace the placeholder with their real namespace or use an env var: change the
substitution target to a concrete placeholder like 'my-namespace' and add a
comment prompting users to replace it, or show using an environment variable
(e.g., NS=my-namespace; sed "s/<your-namespace>/$NS/" ...) when invoking sed on
$base/local-queue.yaml and $base/trainjob-kueue-example.yaml so resources are
created in the correct namespace.
In `@e2e/cases/c1_smoke_gpu.sh`:
- Line 44: The current gpu_kc wait invocation using
--for=jsonpath='{.status.phase}' only waits for the field to be present and is
misleading; update the wait call that references gpu_kc, the NS variable and pod
variable to wait for a terminal phase by adding an explicit equality (e.g.,
change the --for=jsonpath to check for =Succeeded) so it actually blocks until
the pod reaches Succeeded (or alternatively remove that wait line and rely on
the existing poll loop) — locate the line that invokes gpu_kc -n "${NS}" wait
... pod/"${pod}" and either append =Succeeded to the jsonpath check or delete
the line.
In `@e2e/cases/c5_trainer_v2_llamafactory.sh`:
- Around line 16-23: The script reuses fixed names RUNTIME and PVC_NAME causing
RWX PVC and /mnt/models collisions across runs; make the runtime and PVC unique
per run by appending a run-specific suffix (timestamp or random ID) to RUNTIME
and PVC_NAME (and to any related mount paths), ensure the script creates the
per-run PVC (instead of assuming the shared one) and update cleanup to delete
the generated TrainJob plus the per-run PVC and any runtime resources tied to
the unique RUNTIME; locate and update the RUNTIME and PVC_NAME assignments and
any cleanup code that currently only deletes the TrainJob so they use the new
unique names and perform proper deletion.
- Around line 313-324: The background kubectl logs follower started via "gpu_kc
-n \"${NS}\" logs -f \"${POD}\"" is stored in LPID and can hang wait "${LPID}"
indefinitely if the pod never transitions; after the deadline loop (which sets
ph), ensure the background logger is terminated before waiting: check if LPID is
still running (e.g., kill -0 "${LPID}" >/dev/null 2>&1), send a polite kill
(kill "${LPID}" 2>/dev/null || true) and then wait "${LPID}" with a fallback
wait timeout or a final wait "${LPID}" 2>/dev/null || true so the script cannot
hang; update the block that defines LPID, the loop that inspects ph, and the
final wait to perform this kill-then-wait cleanup for the gpu_kc logs follower.
In `@e2e/cases/c6_volcanojob_llamafactory.sh`:
- Around line 13-15: PVC_NAME is shared across runs causing stale data/races;
change it to a per-run unique PVC (e.g., derive from JOB_NAME or $$) and update
all other occurrences (the similar block around the lines noted as also applies
to 210-213) so each test run creates and later deletes its own PVC; ensure the
PVC creation and teardown logic references the new PVC_NAME variable
consistently (search for PVC_NAME and JOB_NAME in this script to locate and
update create/delete steps).
- Around line 226-253: The background `kubectl logs -f` processes (started into
LP1 and LP2 via `gpu_kc ... logs -f ... &`) can block forever even after the
deadline loops exit; update the post-deadline handling in both the initContainer
loop (init_ph) and the pod phase loop (ph) to detect if the log follower is
still running and terminate it before waiting: after the deadline loop ends,
check the follower PID (LP1/LP2) with kill -0 to see if it's alive, send a
polite SIGTERM and after a short grace send SIGKILL if still alive, then wait on
the PID (wait "$LP1"/"$LP2") to reap it; implement this in the blocks around the
init_ph handling and the ph handling so the timeout actually stops the
background log follower and the script can fail fast.
In `@e2e/cases/c7_smoke_npu.sh`:
- Around line 24-29: The comment above TJ_NAME incorrectly says "Override CPU
request to 200m" while the yq mutation sets
.spec.trainer.resourcesPerNode.requests.cpu to "50m"; fix by making the comment
and code consistent—either change the comment to state "50m" (or explain why a
lower request is used) or update the yq value to "200m" so
.spec.trainer.resourcesPerNode.requests.cpu matches the comment; locate the
TJ_NAME assignment and the yq '.spec.trainer.resourcesPerNode' expression to
apply the change.
In `@e2e/lib.sh`:
- Line 13: The default NPU_KUBECONFIG assignment uses a personal absolute path;
update the NPU_KUBECONFIG variable (the assignment of NPU_KUBECONFIG in
e2e/lib.sh) to use a portable default or require the env to be set: either use a
generic fallback like $HOME/.kube/npu-env.yaml or ./npu-env.yaml as the default,
or make the variable mandatory by using shell parameter expansion that errors
when unset (so callers must set NPU_KUBECONFIG). Modify the assignment
accordingly and add a brief comment indicating that consumers should override it
if needed.
---
Nitpick comments:
In
`@docs/en/training_guides/assets/training-runtimes/llamafactory0.9-cann8.5-arm64-trainingruntime.yaml`:
- Around line 43-44: Replace the two unconditional source lines that reference
/usr/local/Ascend/ascend-toolkit/set_env.sh and
/usr/local/Ascend/nnal/atb/set_env.sh with a defensive loop that checks multiple
possible CANN env script locations (e.g., /usr/local/Ascend/cann/set_env.sh,
/usr/local/Ascend/ascend-toolkit/set_env.sh,
/usr/local/Ascend/nnal/atb/set_env.sh) and only sources existing files; wrap the
loop with set +e before it and set -e after it so missing files don’t cause the
script to fail — update the block in
llamafactory0.9-cann8.5-arm64-trainingruntime.yaml where those source lines
appear.
In `@e2e/cases/c11_qwen3_06b_mindspore.sh`:
- Around line 110-118: The pod-wait loop sets POD but doesn't explicitly fail
when no pod is found; after the while loop that assigns POD using npu_kc and
before continuing (after log "C11: pod=${POD}"), add an explicit check for an
empty POD (check the POD variable) and if empty call log with a clear error
(e.g., "C11: no pod found after timeout") and exit with a non-zero status;
reference the variables POD, deadline, SECONDS, NS, JOB_NAME and the log helper
so you place the check immediately after the loop and before subsequent steps.
In `@e2e/cases/c4_traininghub_osft.sh`:
- Around line 183-187: On failure when status != *Complete*, add a container
logs tail to match C3: after the pod describe call in the block that checks
"${status}", call gpu_kc -n "${NS}" logs -l "job-name=${JOB_NAME}" --tail=200
(or equivalent) to dump recent container logs; update the failure diagnostics
block containing gpu_kc get pod and gpu_kc describe pod so it also captures logs
for the same label selector (JOB_NAME) and namespace (NS).
In `@e2e/cases/c9_qwen3_finetune_verify.sh`:
- Around line 94-102: After the pod-wait loop that uses deadline, POD, npu_kc,
JOB_NAME and NS, add an explicit check for an empty POD and fail fast: if POD is
empty after the loop, call log with a clear message including JOB_NAME and NS,
then exit non-zero so the script stops early instead of proceeding to later
checks; place this check immediately after the existing while loop and before
the existing log "C9: pod=${POD}" line.
In `@e2e/lib.sh`:
- Line 64: The current code reads all stdin into the bash variable data via
data="$(cat)", which can OOM for large YAML manifests; change the implementation
to stream stdin into a temporary file instead (use mktemp to create a file, cat
> "$TMPFILE" or redirect stdin to it), update all places that reference the
in-memory variable (data) to read from the temp file (e.g., pass the filename to
retry loops or commands), ensure the temp file is removed on exit (trap 'rm -f
"$TMPFILE"' EXIT), and add a short comment documenting expected max manifest
size or that a temp file is used for large inputs.
- Line 72: Leave the transient-error retry regex in _retry_kubectl_stdin
unchanged; the current pattern (matching failed calling webhook|x509|connection
refused|EOF|context deadline exceeded|webhook.* connect: connection refused)
already covers the observed "context deadline exceeded" and there is no evidence
of missing webhook/TLS/x509 variants in this repo, so do not expand or modify
the regex now—only revisit and broaden it if new log message formats appear in
future runs.
In `@e2e/TODO.md`:
- Line 33: Update the sentence that describes DeepSpeed's import-time CUDA check
failing on images without the toolkit (the paragraph mentioning
traininghub0.1-cu126-amd64, llamafactory0.9-cu126-amd64 and C5's stub) to
explicitly state the functional limitation of the C5 workaround: note that the
stub deepspeed module only bypasses the import-time check and does not provide
an actual CUDA toolchain (nvcc), so JIT compilation of DeepSpeed/custom CUDA ops
will fail and any features relying on compiled kernels are not supported;
mention nvcc as the preferred fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff658388-5c2d-4684-854a-b0da3a4c0cb9
⛔ Files ignored due to path filters (10)
e2e/logs/C1.logis excluded by!**/*.loge2e/logs/C11.logis excluded by!**/*.loge2e/logs/C2.logis excluded by!**/*.loge2e/logs/C3.logis excluded by!**/*.loge2e/logs/C4.logis excluded by!**/*.loge2e/logs/C5.logis excluded by!**/*.loge2e/logs/C6.logis excluded by!**/*.loge2e/logs/C7.logis excluded by!**/*.loge2e/logs/C8.logis excluded by!**/*.loge2e/logs/C9.logis excluded by!**/*.log
📒 Files selected for processing (22)
docs/en/training_guides/assets/kueue/cluster-queue.yamldocs/en/training_guides/assets/kueue/local-queue.yamldocs/en/training_guides/assets/kueue/trainjob-kueue-example.yamldocs/en/training_guides/assets/training-runtimes/llamafactory0.9-cann8.5-arm64-trainingruntime.yamldocs/en/training_guides/assets/training-runtimes/mindspeed-llm-cann8.5-arm64-trainingruntime.yamldocs/en/training_guides/assets/training-runtimes/torch2.6-cann8.5-arm64-trainingruntime.yamldocs/en/training_guides/fine-tune-with-trainer-v2.mdxdocs/en/training_guides/training-hub-fine-tuning.mdxdocs/en/training_guides/training-runtimes.mdxe2e/TODO.mde2e/cases/c11_qwen3_06b_mindspore.she2e/cases/c1_smoke_gpu.she2e/cases/c2_kubeflow_trainer_mnist.she2e/cases/c3_traininghub_sft.she2e/cases/c4_traininghub_osft.she2e/cases/c5_trainer_v2_llamafactory.she2e/cases/c6_volcanojob_llamafactory.she2e/cases/c7_smoke_npu.she2e/cases/c8_trainer_v2_mindspeed_npu.she2e/cases/c9_qwen3_finetune_verify.she2e/lib.she2e/run_all.sh
|
|
||
| # Wait until container has run (pod terminates Succeeded or Failed) then dump logs. | ||
| log "C1: waiting for pod terminal state..." | ||
| gpu_kc -n "${NS}" wait --for=jsonpath='{.status.phase}' pod/"${pod}" --timeout=600s 2>&1 | tee /tmp/c1_wait || true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify kubectl wait syntax requirements
kubectl wait --help 2>&1 | grep -A5 "^\s*--for"Repository: alauda/aml-docs
Length of output: 41
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the script around the referenced line(s)
echo "---- File: e2e/cases/c1_smoke_gpu.sh (lines 35-70) ----"
nl -ba e2e/cases/c1_smoke_gpu.sh | sed -n '35,70p'
# 2) Check whether kubectl exists and show relevant help for 'kubectl wait'
echo
echo "---- kubectl wait --help (relevant lines) ----"
if command -v kubectl >/dev/null 2>&1; then
kubectl wait --help 2>&1 | rg -n --context 0 "(--for|jsonpath|condition|delete|Examples)" || true
else
echo "kubectl not found in sandbox; skipping kubectl --help."
fi
# 3) If gpu_kc exists in the sandbox, show help; otherwise, skip.
echo
echo "---- gpu_kc wait --help (if available) ----"
if command -v gpu_kc >/dev/null 2>&1; then
gpu_kc wait --help 2>&1 | rg -n --context 0 "(--for|jsonpath|condition|delete|Examples)" || true
else
echo "gpu_kc not found in sandbox; skipping gpu_kc --help."
fiRepository: alauda/aml-docs
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="e2e/cases/c1_smoke_gpu.sh"
echo "---- File: $FILE (lines 35-70) ----"
if [ -f "$FILE" ]; then
awk 'NR>=35 && NR<=70 {printf "%d\t%s\n", NR, $0}' "$FILE"
else
echo "File not found: $FILE"
fi
echo
echo "---- kubectl existence ----"
if command -v kubectl >/dev/null 2>&1; then
echo "kubectl found at: $(command -v kubectl)"
else
echo "kubectl not found in sandbox."
fi
echo
echo "---- kubectl wait --help (relevant lines) ----"
if command -v kubectl >/dev/null 2>&1; then
kubectl wait --help 2>&1 | head -n 120 | rg -n "(--for|jsonpath|condition|delete|Examples)" || true
else
echo "Skipping kubectl help (kubectl not available)."
fi
echo
echo "---- gpu_kc existence ----"
if command -v gpu_kc >/dev/null 2>&1; then
echo "gpu_kc found at: $(command -v gpu_kc)"
else
echo "gpu_kc not found in sandbox."
fi
echo
echo "---- gpu_kc wait --help (if available) ----"
if command -v gpu_kc >/dev/null 2>&1; then
gpu_kc wait --help 2>&1 | head -n 120 | rg -n "(--for|jsonpath|condition|delete|Examples)" || true
else
echo "Skipping gpu_kc help (gpu_kc not available)."
fiRepository: alauda/aml-docs
Length of output: 1416
🌐 Web query:
gpu_kc wait --for jsonpath syntax
💡 Result:
The syntax for using JSONPath with the kubectl wait command is: --for=jsonpath='{JSONPath expression}'[=value] You can use this flag to wait for a specific field in a Kubernetes resource to reach a desired value [1][2]. Key details regarding this syntax: 1. Basic Usage: To wait for a field to equal a specific value, append =value to the JSONPath expression [1][3]. Example: kubectl wait --for=jsonpath='{.status.phase}'=Running pod/my-pod 2. Existence Check: If you omit the =value part, the command will wait until the field exists (i.e., is not empty/null) [1][3]. Example: kubectl wait --for=jsonpath='{.status.loadBalancer.ingress}' service/my-service 3. Formatting and Quotes: The JSONPath expression is typically enclosed in single quotes. Be careful with shell escaping and quoting of the entire --for argument to avoid parsing errors [4]. Recommended syntax: --for=jsonpath='{.status.phase}'=Running Note that some complex JSONPath filter expressions (e.g., those using array index filters like?(@.name=="x")) are not supported because the command naively splits the input string using the = character [3][5]. 4. Case Sensitivity: The comparison of the value is performed after Unicode simple case folding, making it a form of case-insensitive comparison [1][6]. Multiple --for flags can be used in a single command, and they will be evaluated sequentially [1][3].
Citations:
- 1: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_wait/
- 2: https://kubernetes.ltd/docs/reference/kubectl/generated/kubectl_wait/
- 3: https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/wait/wait.go
- 4:
waitcommand produces error on quoted conditions kubernetes/kubectl#1646 - 5: kubectl wait doesn't support complex jsonpath kubernetes/kubectl#1259
- 6: https://man.archlinux.org/man/extra/kubectl/kubectl-wait.1.en
🌐 Web query:
kubectl wait --for=jsonpath syntax --help condition delete Examples .status.phase
💡 Result:
The kubectl wait command supports waiting for specific resource statuses or conditions using the --for flag [1][2]. When using JSONPath, the syntax allows you to wait for a specific field to match a desired value [1][3]. Syntax structure: kubectl wait --for=jsonpath='{JSONPath expression}'=value resource/name Key details: - JSONPath expression: An expression enclosed in single quotes, such as '{.status.phase}' [1][4]. - Value comparison: The desired value is specified after an equal sign (=). The command waits until the field matches this value [3][5]. - Case-insensitivity: Comparisons are performed after Unicode simple case folding, which provides a general form of case-insensitivity [1][2]. - Existence checks: If no value is provided (i.e., only the JSONPath expression is given), the command waits for the field to exist or be present (non-empty) [1][3]. Examples: - To wait for a pod's phase to reach "Running": kubectl wait --for=jsonpath='{.status.phase}'=Running pod/busybox1 [1][6] - To wait for a service to have an assigned ingress (checking for existence): kubectl wait --for=jsonpath='{.status.loadBalancer.ingress}' service/loadbalancer [1][7] - To wait for a specific status condition using JSONPath: kubectl wait --for='jsonpath={.status.conditions[?(@.type=="Ready")].status}=True' pod/busybox1 [1][4] Note: The --for=delete flag is a distinct, non-JSONPath option used to wait for a resource to be fully removed from the cluster [1][2]. If multiple conditions are provided (e.g., --for=create --for=jsonpath=...), they are evaluated sequentially, and the command proceeds only if all conditions are met [1][2].
Citations:
- 1: https://kubernetes.io/docs/reference/kubectl/generated/kubectl_wait/
- 2: https://man.archlinux.org/man/extra/kubectl/kubectl-wait.1.en
- 3: https://github.com/kubernetes/kubectl/blob/master/pkg/cmd/wait/wait.go
- 4: https://www.mankier.com/1/kubectl-wait
- 5: https://linuxcommandlibrary.com/man/kubectl-wait
- 6: https://kubernetes.ltd/docs/reference/kubectl/generated/kubectl_wait/
- 7: kubectl wait on arbitrary jsonpath kubernetes/kubernetes#83094
Fix misleading gpu_kc wait usage for pod phase (lines 44-45).
kubectl wait treats --for=jsonpath='{.status.phase}' as “wait until the JSONPath field exists/non-empty” (the =value comparison is optional). So line 44 won’t actually wait for Succeeded/Failed, and the comment saying it watches Ready by default is misleading. Either specify a terminal value (e.g., ...wait --for=jsonpath='{.status.phase}'=Succeeded ...) or remove line 44 and rely on the existing poll loop.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/cases/c1_smoke_gpu.sh` at line 44, The current gpu_kc wait invocation
using --for=jsonpath='{.status.phase}' only waits for the field to be present
and is misleading; update the wait call that references gpu_kc, the NS variable
and pod variable to wait for a terminal phase by adding an explicit equality
(e.g., change the --for=jsonpath to check for =Succeeded) so it actually blocks
until the pod reaches Succeeded (or alternatively remove that wait line and rely
on the existing poll loop) — locate the line that invokes gpu_kc -n "${NS}" wait
... pod/"${pod}" and either append =Succeeded to the jsonpath check or delete
the line.
Comment on lines
+24
to
+29
| # Override CPU request to 200m — kubeos2 runs at ~99% CPU when shared with the dev workloads | ||
| # in this cluster, but has 4 free NPUs. The smoke probe just runs a matmul, 200m is plenty. | ||
| TJ_NAME=$(sed -e "s/namespace: kubeflow-admin-cpaas-io/namespace: ${NS}/" \ | ||
| -e 's/name: torch2.6-cu126-amd64/name: torch2.6-cann8.5-arm64/' \ | ||
| "${ASSETS}/trainjob-smoke.yaml" \ | ||
| | yq '.spec.trainer.resourcesPerNode = {"requests":{"cpu":"50m","memory":"512Mi"},"limits":{"cpu":"500m","memory":"4Gi","huawei.com/Ascend910":"1"}}' \ |
There was a problem hiding this comment.
Comment mentions 200m CPU but code requests 50m.
Line 24-25 comment states "Override CPU request to 200m" but line 29 actually sets requests.cpu: "50m". Update the comment to match the actual value or clarify the intent.
📝 Suggested comment fix
-# Override CPU request to 200m — kubeos2 runs at ~99% CPU when shared with the dev workloads
-# in this cluster, but has 4 free NPUs. The smoke probe just runs a matmul, 200m is plenty.
+# Override CPU request to 50m — kubeos2 runs at ~99% CPU when shared with the dev workloads
+# in this cluster, but has 4 free NPUs. The smoke probe just runs a matmul, 50m is plenty.📝 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.
Suggested change
| # Override CPU request to 200m — kubeos2 runs at ~99% CPU when shared with the dev workloads | |
| # in this cluster, but has 4 free NPUs. The smoke probe just runs a matmul, 200m is plenty. | |
| TJ_NAME=$(sed -e "s/namespace: kubeflow-admin-cpaas-io/namespace: ${NS}/" \ | |
| -e 's/name: torch2.6-cu126-amd64/name: torch2.6-cann8.5-arm64/' \ | |
| "${ASSETS}/trainjob-smoke.yaml" \ | |
| | yq '.spec.trainer.resourcesPerNode = {"requests":{"cpu":"50m","memory":"512Mi"},"limits":{"cpu":"500m","memory":"4Gi","huawei.com/Ascend910":"1"}}' \ | |
| # Override CPU request to 50m — kubeos2 runs at ~99% CPU when shared with the dev workloads | |
| # in this cluster, but has 4 free NPUs. The smoke probe just runs a matmul, 50m is plenty. | |
| TJ_NAME=$(sed -e "s/namespace: kubeflow-admin-cpaas-io/namespace: ${NS}/" \ | |
| -e 's/name: torch2.6-cu126-amd64/name: torch2.6-cann8.5-arm64/' \ | |
| "${ASSETS}/trainjob-smoke.yaml" \ | |
| | yq '.spec.trainer.resourcesPerNode = {"requests":{"cpu":"50m","memory":"512Mi"},"limits":{"cpu":"500m","memory":"4Gi","huawei.com/Ascend910":"1"}}' \ |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@e2e/cases/c7_smoke_npu.sh` around lines 24 - 29, The comment above TJ_NAME
incorrectly says "Override CPU request to 200m" while the yq mutation sets
.spec.trainer.resourcesPerNode.requests.cpu to "50m"; fix by making the comment
and code consistent—either change the comment to state "50m" (or explain why a
lower request is used) or update the yq value to "200m" so
.spec.trainer.resourcesPerNode.requests.cpu matches the comment; locate the
TJ_NAME assignment and the yq '.spec.trainer.resourcesPerNode' expression to
apply the change.
added 3 commits
June 9, 2026 06:16
Stub-and-disable approach (PYTHONPATH stub for `flash_attn` +
`disable_flash_attn=True` on `training_hub.osft(...)`) still fails because
`transformers._check_and_adjust_attn_implementation` calls
`importlib.metadata.version("flash_attn")` independently and raises
`PackageNotFoundError`. Real `flash_attn` install isn't an option either —
the sm_60 Tesla P100 dev GPU is below flash_attn 2.x's sm_75+ floor.
Keep the stub + `disable_flash_attn=True` in c4 for the next reviewer to
pick up on real Ampere/Hopper hardware; comment C4 out of `run_all.sh`
again and tighten the e2e/TODO.md entry with the exact transformers
call path that aborts.
…op reviewer-only e2e/ - Add `torch-distributed:v2.9.1-aml2` to the catalog — it's the `ClusterTrainingRuntime` image kubeflow-trainer-quick-start.md applies and the one C2 exercises via the published `ClusterTrainingRuntime`. - Add `alauda-workbench-jupyter-mindspore-cann-py312-ubi9:v0.1.7` to the catalog — qwen3_0.6b_finetune_verify.ipynb's documented MindSpore CANN workbench image; C11 verifies pull + env on it. - Stop tracking `e2e/TODO.md` and `e2e/logs/` — they're reviewer-side artefacts, not doc content; add an `e2e/.gitignore` so they stay local.
- c5 / c6: replace `wait` on the background `kubectl logs -f` with a new
`reap_logs` helper that `kill`s the follower first. Previously, if the
pod never reached a terminal phase the log follower stayed alive past
the polling deadline and `wait` blocked indefinitely.
- c5 / c6: derive `RUNTIME` / `PVC_NAME` from the run-id and tear them
down in the cleanup trap. Re-runs and concurrent runs no longer share
`/mnt/models` contents or fight over the same PVC / TrainingRuntime.
- lib.sh: default `NPU_KUBECONFIG` to `$HOME/.kube/npu-env.yaml` instead
of a personal `/workspaces/...` path, so the cases work outside one
workspace. The env override still wins.
- fine-tune-with-trainer-v2.mdx (Scheduling with Kueue): set `NS=my-namespace`
and substitute that into both `local-queue.yaml` and
`trainjob-kueue-example.yaml`. The previous `sed
's/<your-namespace>/<your-namespace>/'` was a no-op and left users
applying a literal placeholder namespace.
- fine-tune-with-trainer-v2.mdx: change the Kueue link from the absolute
`/kueue/intro.mdx` to the route-relative `../kueue/intro` used elsewhere
in `docs/en/`. Matches `jobset/{quickstart,intro}.mdx` and avoids a
broken route.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/cases/c4_traininghub_osft.sh (1)
195-195:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace conditional wait with
reap_logsto prevent indefinite hang.If the job times out without reaching
CompleteorFailed, the polling loop exits at line 194, but the backgroundkubectl logs -fprocess (started at line 184) continues streaming indefinitely. Thewait "${LOGS_PID}"then blocks forever, causing the test to hang instead of failing after the timeout.🔧 Recommended fix using the `reap_logs` helper
-[ -n "${LOGS_PID:-}" ] && wait "${LOGS_PID}" 2>/dev/null || true +reap_logs "${LOGS_PID:-}"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@e2e/cases/c4_traininghub_osft.sh` at line 195, The current conditional wait using LOGS_PID may block indefinitely; replace the conditional wait "[ -n \"${LOGS_PID:-}\" ] && wait \"${LOGS_PID}\" 2>/dev/null || true" with a call to the existing reap_logs helper so the background `kubectl logs -f` process is properly terminated after the polling loop times out; locate the code that starts the background logs (the `kubectl logs -f` invocation and the LOGS_PID assignment) and replace the final wait with a single call to reap_logs LOGS_PID (or the helper's expected signature) to ensure logs are reaped instead of hanging.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@e2e/cases/c4_traininghub_osft.sh`:
- Line 195: The current conditional wait using LOGS_PID may block indefinitely;
replace the conditional wait "[ -n \"${LOGS_PID:-}\" ] && wait \"${LOGS_PID}\"
2>/dev/null || true" with a call to the existing reap_logs helper so the
background `kubectl logs -f` process is properly terminated after the polling
loop times out; locate the code that starts the background logs (the `kubectl
logs -f` invocation and the LOGS_PID assignment) and replace the final wait with
a single call to reap_logs LOGS_PID (or the helper's expected signature) to
ensure logs are reaped instead of hanging.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e44452b1-78b9-4378-8930-2a16528baa56
📒 Files selected for processing (8)
docs/en/training_guides/fine-tune-with-trainer-v2.mdxdocs/en/training_guides/training-runtimes.mdxe2e/.gitignoree2e/cases/c4_traininghub_osft.she2e/cases/c5_trainer_v2_llamafactory.she2e/cases/c6_volcanojob_llamafactory.she2e/lib.she2e/run_all.sh
✅ Files skipped from review due to trivial changes (3)
- e2e/.gitignore
- docs/en/training_guides/fine-tune-with-trainer-v2.mdx
- docs/en/training_guides/training-runtimes.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/run_all.sh
- e2e/cases/c6_volcanojob_llamafactory.sh
This was referenced Jun 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
docs/en/kubeflow/how_to/anddocs/en/workbench/how_to/into a single top-leveldocs/en/training_guides/section, with a landingindex.mdxthat routes users by workflow.fine_tunning_using_notebooks.mdx→fine-tuning-using-notebooks.mdx, underscoredipynbnames → dashed).model_inference/inference_service/how_to/mlops_with_coding_agents.mdx.Notes
training-runtimesREADME links to asset paths underdocs/en/kubeflow/how_to/...; those will need a follow-up pointing atdocs/en/training_guides/....llms.txtandllmstxt-state.jsonstill reference the old paths; they regenerate fromllmstxt-config.yamlon the next docs build.Test plan
yarn buildagainst this branch — site builds, thetraining_guidessection appears in the sidebar, every moved page renders.mlops_with_coding_agents.mdxand from each moved page resolve.llms.txt/llmstxt-state.json.Summary by CodeRabbit
Release Notes
Documentation
Tests