add continue_pretrain script#589
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new shell script, continue_pretrain_aquila_moe.sh, for pre-training Aquila MoE models using dlrover-run. The review feedback highlights several issues regarding script portability and maintainability, specifically pointing out redundant variable assignments, hardcoded user-specific and root directory paths, a hardcoded rendezvous ID, and the intrusive modification of global git configurations.
| EXPNAME=K74_BETA_Merge | ||
| EXPNAME=K74_BETA_Single | ||
| EXPNAME=K74_BETA_Single_Lin |
There was a problem hiding this comment.
The variable EXPNAME is assigned three times consecutively. This is redundant and appears to be leftover from development; only the final assignment is effective.
| EXPNAME=K74_BETA_Merge | |
| EXPNAME=K74_BETA_Single | |
| EXPNAME=K74_BETA_Single_Lin | |
| EXPNAME=K74_BETA_Single_Lin |
|
|
||
| CMD=" \ | ||
| dlrover-run \ | ||
| --rdzv-id 20240410_074637.966007 \ |
There was a problem hiding this comment.
The --rdzv-id is hardcoded to a specific timestamp. This can cause conflicts if multiple training jobs are launched or if the script is reused without modification. It is recommended to use a dynamic ID or an environment variable.
| --rdzv-id 20240410_074637.966007 \ | |
| --rdzv-id ${RDZV_ID:-$(date +%Y%m%d_%H%M%S)} \ |
| #ulimit -c unlimited | ||
| #ulimit -a | ||
|
|
||
| git config --global --add safe.directory /mnt/baai/ldwang/$FlagScale |
There was a problem hiding this comment.
Modifying the global git configuration (git config --global) is an intrusive side effect that persists across sessions and affects other repositories on the system. This should be avoided in a training script.
| export FORCE_ACTIVATE_WAIT=1 | ||
| export MHA_USE_BLAS=ON | ||
| #export MHA_BWD_NO_ATOMIC_F64=1 | ||
| export LD_LIBRARY_PATH=/root/FWD_76_BWD_79:/opt/maca/lib:/opt/maca/ompi/lib:/opt/maca/ucx/lib |
There was a problem hiding this comment.
The LD_LIBRARY_PATH includes a hardcoded path in the /root directory. This makes the script non-portable and will cause errors for users who do not have access to that directory or are not running as root.
| BASE_PATH=/mnt/baai/ldwang | ||
| # TODO Datasets | ||
| source /mnt/baai/ldwang/FlagScale/examples/aquila/datasets_mx.sh | ||
| DATA_PATH=$DATASET_K74_BETA | ||
| TOKENIZER_PATH=/mnt/baai/ldwang/FlagScale/examples/aquila/1_8B/tokenizer_hf |
There was a problem hiding this comment.
The script contains multiple hardcoded paths specific to a user's directory (/mnt/baai/ldwang). These should be parameterized or use environment variables to make the script usable in different environments.
1 participant
name: Pull Request
title: '[PR]'
assignees: 'BAAI-OpenPlatform,ftgreat'
Description
Please describe here what the PR does.
Checklist