Add two scripts #119
Add two scripts #119
Conversation
Summary of ChangesHello @DubhepPan, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces two new shell scripts aimed at enhancing the automated testing capabilities for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 adds two test enumeration scripts. I've found a few issues, including a critical bug in enumerate_test_low_latency.sh where some test parameter loops are empty because the corresponding arrays are not defined, causing tests to be skipped silently. Both scripts also have issues with argument parsing, where they reference unhandled options and a non-existent show_help function. I've also included some suggestions for improving code consistency and safety.
| for ACTIVE_RANKS in "${ACTIVE_RANKS_LIST[@]}"; do | ||
| for ENABLE_DIAGNOSE in "${ENABLE_DIAGNOSE_LIST[@]}"; do |
There was a problem hiding this comment.
The script attempts to loop over ACTIVE_RANKS_LIST and ENABLE_DIAGNOSE_LIST, but these arrays are not defined in this script. This will cause the inner loops to be skipped silently, meaning a significant portion of the intended tests will not run. This is a critical bug, likely from a copy-paste error. Please either define these arrays with appropriate values for test_low_latency.py or remove the loops and the corresponding logic that uses ACTIVE_RANKS and ENABLE_DIAGNOSE variables.
scripts/enumerate_test_intranode.sh
Outdated
| # 默认值 | ||
| SKIP_BUILD=false | ||
|
|
||
| TEMP=$(getopt -o sw:t:h --long skip-build -n "$0" -- "$@") |
There was a problem hiding this comment.
The getopt string sw:t:h declares options w:, t:, and h which are not handled in the case statement below. If a user provides any of these options, the script will fail with an "Invalid option" error. Please remove the unhandled options from the getopt string or add handlers for them.
| TEMP=$(getopt -o sw:t:h --long skip-build -n "$0" -- "$@") | |
| TEMP=$(getopt -o s --long skip-build -n "$0" -- "$@") |
scripts/enumerate_test_intranode.sh
Outdated
| ;; | ||
| *) | ||
| echo "Invalid option: $1" >&2 | ||
| show_help |
There was a problem hiding this comment.
The show_help function is called here but is not defined in the script. This will cause a "command not found" error when this branch is executed. Please define the show_help function. A simple implementation could be:
show_help() {
echo "Usage: $0 [-s|--skip-build]"
}| # 默认值 | ||
| SKIP_BUILD=false | ||
|
|
||
| TEMP=$(getopt -o sw:t:h --long skip-build -n "$0" -- "$@") |
There was a problem hiding this comment.
The getopt string sw:t:h declares options w:, t:, and h which are not handled in the case statement below. If a user provides any of these options, the script will fail with an "Invalid option" error. Please remove the unhandled options from the getopt string or add handlers for them.
| TEMP=$(getopt -o sw:t:h --long skip-build -n "$0" -- "$@") | |
| TEMP=$(getopt -o s --long skip-build -n "$0" -- "$@") |
| ;; | ||
| *) | ||
| echo "Invalid option: $1" >&2 | ||
| show_help |
There was a problem hiding this comment.
The show_help function is called here but is not defined in the script. This will cause a "command not found" error when this branch is executed. Please define the show_help function. A simple implementation could be:
show_help() {
echo "Usage: $0 [-s|--skip-build]"
}
scripts/enumerate_test_intranode.sh
Outdated
|
|
||
| #遍历test_intranode.py | ||
| # 设置参数范围 | ||
| NUM_PROCESSES_LIST_=(4 8 16) |
There was a problem hiding this comment.
The variable name NUM_PROCESSES_LIST_ ends with an underscore, which is inconsistent with other list variables in the script (e.g., NUM_TOKENS_LIST). For better code consistency and readability, consider renaming it to NUM_PROCESSES_LIST.
| NUM_PROCESSES_LIST_=(4 8 16) | |
| NUM_PROCESSES_LIST=(4 8 16) |
|
|
||
| # 打印并执行命令 | ||
| echo "Running: $CMD" | ||
| eval $CMD |
There was a problem hiding this comment.
Using eval can be a security risk if the command string is constructed from untrusted input. While it seems safe in this context as the parameters are from predefined lists, it's a good practice to avoid eval. A safer alternative is to build an array of command arguments and execute it directly.
| done | ||
| echo "All jobs completed successfully" | ||
| exit 0 | ||
| - name: Prepare Deepep | ||
| run: bash scripts/prepare_deepep_in_container.sh | ||
|
|
||
2 participants
Add two scripts for testing