Add A4X MAX DeepSeek-V3 FP8mx 256 GPUs recipe - #276
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the DeepSeek-V3 training recipe on GKE, including upgrading the NeMo image version, refining model parallelism configurations, enabling host networking and GCS volumes by default, and introducing real-time log streaming. The review feedback highlights three key areas for improvement: first, avoid using tee -a (append mode) on GCS Fuse mounts to prevent performance overhead; second, implement a retry mechanism for git clone and git submodule update to prevent network or rate-limiting failures across 64 nodes; and third, use a shared directory for MEGATRON_CONFIG_LOCK_DIR instead of a rank-specific one to ensure proper serialization among local ranks.
| if [[ -n "${ARTIFACT_DIR}" && -n "${JOB_COMPLETION_INDEX}" ]]; then | ||
| if mkdir -p "${ARTIFACT_DIR}/logs/streaming-logs" 2>/dev/null; then | ||
| export STREAMING_LOGS_ENABLED=1 | ||
| exec > >(tee -a "${ARTIFACT_DIR}/logs/streaming-logs/${POD_NAME:-$(hostname -s)}.log") 2>&1 | ||
| else | ||
| echo "WARNING: ARTIFACT_DIR (${ARTIFACT_DIR}) is not writable or not mounted. Real-time GCS log streaming disabled." | ||
| fi | ||
| elif [[ -n "${JOBSET_NAME}" && -n "${JOB_COMPLETION_INDEX}" ]]; then | ||
| if mkdir -p "/runtime-logs/${JOBSET_NAME}/logs" 2>/dev/null; then | ||
| export STREAMING_LOGS_ENABLED=1 | ||
| exec > >(tee -a "/runtime-logs/${JOBSET_NAME}/logs/${POD_NAME:-$(hostname -s)}.log") 2>&1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Using tee -a (append mode) on a GCS Fuse mount (ARTIFACT_DIR) can cause significant performance overhead or write failures because Google Cloud Storage is an object store that does not natively support append operations. GCS Fuse emulates appends by reading and rewriting the entire object, which can be highly inefficient for continuous log streaming. Since each pod writes to a unique log file based on ${POD_NAME}, appending is likely unnecessary. Consider using tee without the -a flag to avoid append emulation overhead.
if [[ -n "${ARTIFACT_DIR}" && -n "${JOB_COMPLETION_INDEX}" ]]; then
if mkdir -p "${ARTIFACT_DIR}/logs/streaming-logs" 2>/dev/null; then
export STREAMING_LOGS_ENABLED=1
exec > >(tee "${ARTIFACT_DIR}/logs/streaming-logs/${POD_NAME:-$(hostname -s)}.log") 2>&1
else
echo "WARNING: ARTIFACT_DIR (${ARTIFACT_DIR}) is not writable or not mounted. Real-time GCS log streaming disabled."
fi
elif [[ -n "${JOBSET_NAME}" && -n "${JOB_COMPLETION_INDEX}" ]]; then
if mkdir -p "/runtime-logs/${JOBSET_NAME}/logs" 2>/dev/null; then
export STREAMING_LOGS_ENABLED=1
exec > >(tee "/runtime-logs/${JOBSET_NAME}/logs/${POD_NAME:-$(hostname -s)}.log") 2>&1
fi
fi| cd /opt | ||
| rm -rf Megatron-Bridge | ||
| git clone https://github.com/NVIDIA-NeMo/Megatron-Bridge.git | ||
| cd Megatron-Bridge | ||
| git checkout fcbb6031103d0ca845c1a54d4fee55ecfcca17b6 | ||
| git submodule update --init --recursive && sed -i 's/timeout=60/timeout=600/g' src/megatron/bridge/models/hf_pretrained/safe_config_loader.py | ||
| git checkout 5cb3444c43f7499cf3872b2d46870cf8bc2e00ce | ||
| git submodule update --init --recursive |
There was a problem hiding this comment.
Running git clone and git submodule update concurrently across 64 nodes (256 GPUs) during job startup is highly prone to transient network failures or GitHub rate-limiting. Consider implementing a simple retry helper function in bash and wrapping the git clone and git submodule update commands with it to make the recipe much more robust.
retry() {
local n=1
local max=3
local delay=5
while true; do
"$@" && break || {
if [[ $n -lt $max ]]; then
((n++))
echo "Command failed. Attempt $n/$max in $delay seconds..."
sleep $delay
else
echo "The command has failed after $max attempts."
return 1
fi
}
done
}
cd /opt
rm -rf Megatron-Bridge
retry git clone https://github.com/NVIDIA-NeMo/Megatron-Bridge.git
cd Megatron-Bridge
git checkout 5cb3444c43f7499cf3872b2d46870cf8bc2e00ce
retry git submodule update --init --recursive| export MEGATRON_CONFIG_LOCK_DIR="/tmp/\$LOCAL_RANK" | ||
| mkdir -p "\$MEGATRON_CONFIG_LOCK_DIR" |
There was a problem hiding this comment.
If MEGATRON_CONFIG_LOCK_DIR is used to serialize operations (such as config loading or downloading) among local ranks on the same node to prevent race conditions (e.g., in the shared Hugging Face cache), it must be a shared directory. Setting it to a rank-specific directory like /tmp/$LOCAL_RANK means each rank uses a different lock directory, which defeats the locking/serialization mechanism. Consider using a shared directory like /tmp/megatron_config_lock instead.
| export MEGATRON_CONFIG_LOCK_DIR="/tmp/\$LOCAL_RANK" | |
| mkdir -p "\$MEGATRON_CONFIG_LOCK_DIR" | |
| export MEGATRON_CONFIG_LOCK_DIR="/tmp/megatron_config_lock" | |
| mkdir -p "\$MEGATRON_CONFIG_LOCK_DIR" |
Add A4X MAX DeepSeek-V3 256 GPUs FP8mx recipe