[codex] fix A5 validation link discovery#837
Draft
HecreReed wants to merge 1 commit into
Draft
Conversation
Codex Review该评论由 review 机器人自动更新。
SummaryReview failed at stage Findings未生成结构化 findings，因为 review 过程提前失败。 Log Tail |
![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 dynamic discovery mechanism for CANN host link directories to improve the robustness of NPU validation across different environments. The changes include adding helper functions to search for standard and fallback CANN roots and updating CMake configurations to link against the discovered paths. The review feedback highlights significant code duplication of these helper functions across scripts, suggesting centralizing them into a shared utility file. Additionally, the feedback recommends declaring the _pto_list_dirs array as local to prevent side effects and refactoring duplicated glob patterns in the search loops to improve maintainability.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+21
to
+130
| append_unique_colon_item() { | ||
| local list="$1" | ||
| local item="$2" | ||
| [[ -n "${item}" && -d "${item}" ]] || { | ||
| echo "${list}" | ||
| return 0 | ||
| } | ||
| if [[ -z "${list}" ]]; then | ||
| echo "${item}" | ||
| return 0 | ||
| fi | ||
| case ":${list}:" in | ||
| *":${item}:"*) echo "${list}" ;; | ||
| *) echo "${list}:${item}" ;; | ||
| esac | ||
| } | ||
|
|
||
| list_contains_file() { | ||
| local list="$1" | ||
| local file_name="$2" | ||
| local dir | ||
| IFS=':' read -r -a _pto_list_dirs <<< "${list}" | ||
| for dir in "${_pto_list_dirs[@]}"; do | ||
| [[ -n "${dir}" && -e "${dir}/${file_name}" ]] && return 0 | ||
| done | ||
| return 1 | ||
| } | ||
|
|
||
| host_lib_arch() { | ||
| case "$(uname -m)" in | ||
| aarch64 | arm64) echo "aarch64" ;; | ||
| x86_64 | amd64) echo "x86_64" ;; | ||
| *) uname -m ;; | ||
| esac | ||
| } | ||
|
|
||
| collect_cann_host_link_dirs_for_root() { | ||
| local root="$1" | ||
| local arch="$2" | ||
| local dirs="$3" | ||
| local dir="" | ||
| for dir in \ | ||
| "${root}/lib64" \ | ||
| "${root}/${arch}-linux/lib64" \ | ||
| "${root}/runtime/lib64" \ | ||
| "${root}/fwkacllib/lib64" \ | ||
| "${root}/${arch}-linux/devlib" \ | ||
| "${root}/${arch}-linux/devlib/linux/${arch}"; do | ||
| [[ -d "${dir}" ]] || continue | ||
| if [[ -e "${dir}/libnnopbase.so" || -e "${dir}/libascendcl.so" \ | ||
| || -e "${dir}/libplatform.so" || -e "${dir}/libtiling_api.a" \ | ||
| || -e "${dir}/libtiling_api.so" ]]; then | ||
| dirs="$(append_unique_colon_item "${dirs}" "${dir}")" | ||
| fi | ||
| done | ||
| echo "${dirs}" | ||
| } | ||
|
|
||
| discover_cann_host_link_dirs() { | ||
| local arch="$1" | ||
| local dirs="" | ||
| local root="" | ||
| local current_base="" | ||
| local -a candidate_roots=() | ||
| shopt -s nullglob | ||
|
|
||
| if [[ -n "${ASCEND_HOME_PATH:-}" ]]; then | ||
| dirs="$(collect_cann_host_link_dirs_for_root "${ASCEND_HOME_PATH}" "${arch}" "${dirs}")" | ||
| current_base="$(basename "${ASCEND_HOME_PATH}")" | ||
| fi | ||
| if list_contains_file "${dirs}" "libnnopbase.so"; then | ||
| shopt -u nullglob | ||
| echo "${dirs}" | ||
| return 0 | ||
| fi | ||
|
|
||
| for root in \ | ||
| /usr/local/Ascend/cann \ | ||
| /usr/local/Ascend/cann-* \ | ||
| /usr/local/Ascend/ascend-toolkit/latest \ | ||
| /home/*/cann*/cann-* \ | ||
| /home/*/*/cann-* \ | ||
| /home/*/Ascend/*/cann-*; do | ||
| [[ -d "${root}" ]] || continue | ||
| [[ -n "${current_base}" && "${root}" == "${ASCEND_HOME_PATH:-}" ]] && continue | ||
| if [[ -n "${current_base}" && "${root}" == *"/${current_base}" ]]; then | ||
| candidate_roots+=("${root}") | ||
| fi | ||
| done | ||
| for root in \ | ||
| /usr/local/Ascend/cann \ | ||
| /usr/local/Ascend/cann-* \ | ||
| /usr/local/Ascend/ascend-toolkit/latest \ | ||
| /home/*/cann*/cann-* \ | ||
| /home/*/*/cann-* \ | ||
| /home/*/Ascend/*/cann-*; do | ||
| [[ -d "${root}" ]] || continue | ||
| [[ "${root}" == "${ASCEND_HOME_PATH:-}" ]] && continue | ||
| candidate_roots+=("${root}") | ||
| done | ||
| shopt -u nullglob | ||
|
|
||
| for root in "${candidate_roots[@]}"; do | ||
| dirs="$(collect_cann_host_link_dirs_for_root "${root}" "$(host_lib_arch)" "${dirs}")" | ||
| if list_contains_file "${dirs}" "libnnopbase.so"; then | ||
| break | ||
| fi | ||
| done | ||
| echo "${dirs}" | ||
| } |
There was a problem hiding this comment.
These helper functions are also defined in test/npu_validation/scripts/run_remote_npu_validation.sh. This large amount of duplicated code is a significant maintenance concern. Any bug fix or improvement would need to be applied in both places.
Consider moving these shared functions into a separate utility script (e.g., npu_validation_utils.sh) and sourcing it from both run_remote_npu_validation.sh and this template. This would centralize the logic and improve maintainability.
| local list="$1" | ||
| local file_name="$2" | ||
| local dir | ||
| IFS=':' read -r -a _pto_list_dirs <<< "${list}" |
There was a problem hiding this comment.
The array _pto_list_dirs is not declared as a local variable. This could cause side effects if another part of the script uses a variable with the same name. It's best practice to declare all function-scoped variables as local.
Suggested change
| IFS=':' read -r -a _pto_list_dirs <<< "${list}" | |
| local -a _pto_list_dirs | |
| IFS=':' read -r -a _pto_list_dirs <<< "${list}" |
Comment on lines
+114
to
+137
| for root in \ | ||
| /usr/local/Ascend/cann \ | ||
| /usr/local/Ascend/cann-* \ | ||
| /usr/local/Ascend/ascend-toolkit/latest \ | ||
| /home/*/cann*/cann-* \ | ||
| /home/*/*/cann-* \ | ||
| /home/*/Ascend/*/cann-*; do | ||
| [[ -d "${root}" ]] || continue | ||
| [[ -n "${current_base}" && "${root}" == "${ASCEND_HOME_PATH:-}" ]] && continue | ||
| if [[ -n "${current_base}" && "${root}" == *"/${current_base}" ]]; then | ||
| candidate_roots+=("${root}") | ||
| fi | ||
| done | ||
| for root in \ | ||
| /usr/local/Ascend/cann \ | ||
| /usr/local/Ascend/cann-* \ | ||
| /usr/local/Ascend/ascend-toolkit/latest \ | ||
| /home/*/cann*/cann-* \ | ||
| /home/*/*/cann-* \ | ||
| /home/*/Ascend/*/cann-*; do | ||
| [[ -d "${root}" ]] || continue | ||
| [[ "${root}" == "${ASCEND_HOME_PATH:-}" ]] && continue | ||
| candidate_roots+=("${root}") | ||
| done |
There was a problem hiding this comment.
The list of glob patterns for discovering CANN roots is duplicated in these two for loops. This makes the script harder to maintain, as any changes to the search paths must be made in two places.
To improve maintainability, you could define the list of paths in a variable or an array and reuse it in both loops.
local search_paths
read -r -d '' search_paths <<'EOF'
/usr/local/Ascend/cann
/usr/local/Ascend/cann-*
/usr/local/Ascend/ascend-toolkit/latest
/home/*/cann*/cann-*
/home/*/*/cann-*
/home/*/Ascend/*/cann-*
EOF
for root in $search_paths; do
[[ -d "${root}" ]] || continue
[[ -n "${current_base}" && "${root}" == "${ASCEND_HOME_PATH:-}" ]] && continue
if [[ -n "${current_base}" && "${root}" == *"/${current_base}" ]]; then
candidate_roots+=("${root}")
fi
done
for root in $search_paths; do
[[ -d "${root}" ]] || continue
[[ "${root}" == "${ASCEND_HOME_PATH:-}" ]] && continue
candidate_roots+=("${root}")
done| local list="$1" | ||
| local file_name="$2" | ||
| local dir | ||
| IFS=':' read -r -a _pto_list_dirs <<< "${list}" |
There was a problem hiding this comment.
The array _pto_list_dirs is not declared as a local variable. This could cause side effects if another part of the script uses a variable with the same name. It's best practice to declare all function-scoped variables as local.
Suggested change
| IFS=':' read -r -a _pto_list_dirs <<< "${list}" | |
| local -a _pto_list_dirs | |
| IFS=':' read -r -a _pto_list_dirs <<< "${list}" |
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
2 participants
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.
Fix the A5 remote NPU validation flow so it no longer mis-detects A3 from simulator fallback directories and can resolve libnnopbase.so from the actual CANN install.\n\nValidation: bash -n on test/npu_validation/scripts/run_remote_npu_validation.sh and test/npu_validation/templates/run_sh_template.sh, plus python3 -m py_compile test/npu_validation/scripts/generate_testcase.py.