Skip to content

增加coverage test并发数降低耗时 - #79927

Open
Ray961123 wants to merge 4 commits into
PaddlePaddle:developfrom
Ray961123:coverage-concurrency
Open

Ray961123 wants to merge 4 commits into
PaddlePaddle:developfrom
Ray961123:coverage-concurrency

Conversation

@Ray961123

Copy link
Copy Markdown
Collaborator

PR Category

User Experience

PR Types

Improvements

Description

增加coverage test并发数降低耗时

是否引起精度变化

否

@Paddle-Review-Bot

Paddle-Review-Bot commented Nov 6, 2026 •

Copy link
Copy Markdown
Contributor

Paddle-Bot Review Board (review完成)

序号 位置 优先级 规则来源 状态
1 覆盖率测试脚本新增的无效参数 P2 仓库规则:基础评审规则 ✅
2 覆盖率测试并发翻倍缺少验证数据 P3 仓库规则:基础评审规则 🚧
3 分片后评论重跑无法匹配作业名 P2 仓库规则:基础评审规则 ✅
4 合并阶段未校验分片报告完整性 P2 仓库规则:基础评审规则 ✅
5 分片列表生成失败被静默忽略 P2 仓库规则:基础评审规则 ✅
6 分片耗时数据直接拼接产生重复 P3 仓库规则:基础评审规则 🚧
7 矩阵作业逐个重跑会被拒绝 P2 仓库规则:基础评审规则 🚧

可维护性建议(总分 85.4 / 100,只给建议,不影响评审结论)

序号 位置 维度 状态
1 合并脚本复制覆盖率差异逻辑 设计一致性 💡
2 分片分支重复实现 Python 报告 底层优雅度 💡
3 分片总数在两处手工同步 逻辑清晰度 ✅
4 分片合并失败未中止流程 错误处理一致性 💡
5 全量覆盖率脚本仍内联安装 lcov 跨模块架构 💡
各维度得分(仅评本 PR 改动,点击展开)
82 设计一致性
1
83 错误处理一致性
4
84 底层优雅度
2
84 跨模块架构
5
85 逻辑清晰度
3
85 抽象适配度
85 AI 生成债务
85 高层优雅度
85 中层优雅度
85 初始化耦合
85 API 表面一致性
85 未完成迁移
85 包组织结构
86 契约一致性
87 命名质量
87 约定偏离
87 测试策略
88 鉴权一致性
90 依赖健康度
— 类型安全

Powered by Nyanpasu claude with Opus 5.5 默认推理级别, please check the suggestions carefully.

Comment thread .github/workflows/Coverage.yml Outdated
docker exec -t ${{ env.container_name }} /bin/bash -c '
source ${{ github.workspace }}/../../../proxy
bash $ci_scripts/coverage_test.sh
bash $ci_scripts/coverage_test.sh parallel

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 新增的 parallel 参数不会产生任何效果:ci/coverage_test.sh 只在第 73 行把 $1 传给 is_run_distribute_in_op_test,而该函数(第 15-40 行)不读取任何位置参数;第 93 行无条件调用 parallel_test_base_gpu_test。因此 bash coverage_test.sh parallel 与不带参数完全等价,容易让人误以为存在单独的并发模式。实际并发只由第 312 行的 CTEST_PARALLEL_LEVEL 控制。建议删除该参数;若确实需要按模式切换,请在脚本中显式解析并在 PR 描述说明。

CCACHE_DIR: "/root/.ccache/coverage"
FLAGS_fraction_of_gpu_memory_to_use: 0.15
CTEST_PARALLEL_LEVEL: 2
CTEST_PARALLEL_LEVEL: 4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 实际并发是 CTEST_PARALLEL_LEVEL 乘以分组倍数:ci/utils.sh 的 card_test 中 parallel_job = $3 * CTEST_PARALLEL_LEVEL,*_mem0 和 no_parallel_case_file 分组传入 4,因此同一张 V100 上的并发从 8 变为 16,其余分组按用例数倍数同样翻倍。覆盖率插桩会让用例变慢,并发翻倍可能增加超时、显存竞争和重试次数。建议在 PR 描述补充改动前后 Coverage test 步骤的耗时、失败及重试用例数量对比,作为验证依据。

strategy:
fail-fast: false
matrix:
shard: [0, 1, 2, 3]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 加上 matrix 后,GitHub 显示的 job 名会变成 Coverage test (0) 到 Coverage test (3)。.github/workflows/rerun.yml:185 仍传入 JOB_NAME: 'Coverage test',而 .github/actions/rerun-workflow/rerun.sh:60-61 用 select(.name == $job_name) 做精确匹配,所以评论触发的 coverage test 重跑将找不到任何 job。新增的 Coverage collect job 也没有对应的重跑入口。如果分支保护把 Coverage test 设为必需检查,也需要同步调整。建议同步更新 rerun.yml,例如为每个分片名称各加一项,或改为前缀匹配。

Comment thread ci/coverage_collect.sh Outdated
for f in ${SHARD_ROOT}/shard-*/coverage-full.info; do
[ -f "${f}" ] && lcov_args+=("-a" "${f}")
done
if [ ${#lcov_args[@]} -eq 0 ]; then

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 这里只在一个分片报告都没有时才报错。如果只有部分分片的 coverage-full.info 存在,脚本会用不完整的数据继续合并并做 diff 判定。这种情况是可能发生的:coverage_info.sh 全程 set +e,gen_full_report || true(第 187 行)失败或第 214 行 cp 失败时,仍会 exit 0,测试 job 照样显示成功。缺了分片,覆盖率会被低估,给出误导性的 exit 9 失败。建议把 NUM_SHARDS 传入 collect 容器,并检查 shard-0 到 shard-$((NUM_SHARDS-1)) 是否都存在 coverage-full.info,缺失时以非 0 退出并打印缺失的分片编号。

Comment thread ci/coverage_test.sh Outdated
if [ "${NUM_SHARDS}" -gt 1 ]; then
echo "::group::Generate shard ${SHARD_INDEX}/${NUM_SHARDS} ut_list"
ctest -N | awk -F ': ' '{print $2}' | sed '/^$/d' | sed '$d' > all_ut_list
python ${PADDLE_ROOT}/tools/shard_ut_list.py ${PADDLE_ROOT} ${SHARD_INDEX} ${NUM_SHARDS}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 shard_ut_list.py 失败时(例如空分片、参数越界)会特意以非 0 退出,但这里没有检查返回值,coverage_test.sh 也没有开启 set -e。这时 build/ut_list 不会生成,tools/group_case_for_parallel.py:55-65 会回退到 all_ut_list,导致该分片静默跑完整套测试。这正是脚本注释里说要避免的情况。建议改为 python ... || exit 1,让分片失败直接暴露。

Comment thread ci/coverage_collect.sh Outdated

# Concatenate shard cost data into the canonical CFS path
mkdir -p /home/data/cfs/coverage/${PR_ID}/${COMMIT_ID}
cat ${SHARD_ROOT}/shard-*/CTestCostData.txt > /home/data/cfs/coverage/${PR_ID}/${COMMIT_ID}/CTestCostData.txt 2>/dev/null || true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 每个分片的 CTestCostData.txt 都是从全量文件开始的:ci/utils.sh:787-789 先下载全量 CTestCostData.txt 并放入 Testing/Temporary,ctest 只更新本分片实际跑过的用例。因此直接 cat 4 个文件,每个用例会出现 4 条记录,其中 3 条是旧值,文件里还会有多段 --- 失败列表。这与第 104-105 行注释中“保留完整耗时数据”的说法不符,下游按这个文件统计耗时会读到重复或过期的数据。建议按用例名去重,优先取实际跑过该用例的分片中的记录;失败列表合并后只保留一段。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

新版本改为按用例名去重,但还有两个问题:

  1. 第 71-73 行注释说每个分片只记录了实际运行的用例,这与实际不符。ci/utils.sh:787-789 在测试前会把全量基线 CTestCostData.txt 复制到 Testing/Temporary,所以每个分片上传的文件都包含全部用例,未在本分片运行的用例保留的是基线旧值。
  2. lines[$1] = $0 保留最后读到的记录,shard-* 展开后 shard-3 最后读入,因此除 shard-3 实际运行的用例外,其它分片的真实耗时都会被基线旧值覆盖。另外 --- 之后的失败用例名会被丢弃,最终只输出一个空的 ---。

建议各分片上传 CostData 时一并上传本分片的 ut_list,合并时每个用例只取其所属分片的记录,失败用例名合并去重后放在 --- 之后。

Comment thread ci/coverage_collect.sh Outdated
NO_PYTHON_COVERAGE_DATA=1
fi

# ---- C++ diff part (same logic as coverage_info.sh) ----

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 可维护性建议(设计一致性,不阻塞合入):第 66-103 行的 C++/Python diff 抽取逻辑,以及第 25-36 行的 lcov 安装逻辑,与 ci/coverage_info.sh 第 20-31 行和第 224-287 行基本逐行相同,第 66 行注释也写了 “same logic as coverage_info.sh”。以后调整 diff 规则需要同时改两处,容易不一致。建议把这两段抽成函数供两边复用,例如放到 ci/utils.sh 或新建一个 coverage_common.sh。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

最新提交删除了 ci/coverage_common.sh,这条建议的问题又出现了:ci/coverage_collect.sh 第 76-110 行与 ci/coverage_info.sh 第 210-241 行的 gen_coverage_diff() 再次逐行重复,lcov 安装代码也在 coverage_collect.sh:25-36、coverage_info.sh:20-31、coverage_all_info.sh:20-31 三处各有一份。如果有意不引入新的公共文件,可以考虑把这两段函数放进 ci/utils.sh,coverage_collect.sh 只 source 后直接调用。

Comment thread ci/coverage_info.sh
# "Coverage collect" job merges all shard reports and does the diff/judge part.
if [ "${COVERAGE_SHARD_ONLY:-OFF}" == "ON" ]; then
echo "::group::Gen python full report (shard ${SHARD_INDEX})"
coverage combine $(ls python-coverage.data.*) || NO_PYTHON_COVERAGE_DATA=1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 可维护性建议(底层优雅度,不阻塞合入):第 196-209 行重新写了一遍同文件第 244-267 行 gen_python_full_report() 的处理流程(combine、xml、extract、remove)。建议把函数定义移到分片分支之前直接调用,这样 Python 覆盖率的过滤规则只需维护一份。

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gen_python_full_report 已移到公共文件,原问题中的 extract/remove 部分已消除。但分片分支第 188-190 行与普通路径第 208-214 行仍重复 coverage combine、coverage xml、coverage_python_coverage.py 三步,而且后续处理已经出现差异(分片分支用 if 判断 NO_PYTHON_COVERAGE_DATA,普通路径用 || true)。建议把这三步连同 gen_python_full_report 放进 coverage_common.sh 的同一个函数,两条路径共用。

Comment thread .github/workflows/Coverage.yml Outdated
timeout-minutes: 180
env:
SHARD_INDEX: ${{ matrix.shard }}
NUM_SHARDS: 4

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 可维护性建议(逻辑清晰度,不阻塞合入):NUM_SHARDS: 4 与第 306 行的 shard: [0, 1, 2, 3] 必须手动保持一致。如果只扩展 matrix 而忘了改这里,新增分片会因 shard_index 越界而失败;而且由于前面提到的返回值未检查,该分片会静默回退为跑全量测试。建议改为 NUM_SHARDS: ${{ strategy.job-total }}。

# fi
if [ -z "$block_jobs" ]; then
block_jobs=$(echo "$jobs_response" | jq -r --arg job_name "$JOB_NAME" \
'.jobs[] | select(.name | startswith($job_name + " (")) | .id')

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 前缀匹配会命中 Coverage test (0) 到 Coverage test (3) 四个 job,但下方循环是对每个 job 单独调用 POST /actions/jobs/{job_id}/rerun。根据 GitHub 社区反馈(https://github.com/orgs/community/discussions/176756 ),第一次调用会为整个 run 创建新的 attempt,之后再重跑上一 attempt 中的其它 job 会返回 403 Only jobs from the current attempt can be re-run。这样评论重跑实际只会重跑第一个分片及其依赖的 collect,真正失败的分片可能没有被重跑;第 74 行的 curl 也没有检查返回码,日志里不容易发现。建议匹配到多个 job 时改为调用一次 /actions/runs/$run_id/rerun-failed-jobs。

Comment thread ci/coverage_collect.sh
for f in ${SHARD_ROOT}/shard-*/coverage-full.info; do
[ -f "${f}" ] && lcov_args+=("-a" "${f}")
done
lcov "${lcov_args[@]}" -o coverage-full.info --rc lcov_branch_coverage=0

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 可维护性建议(错误处理一致性,不阻塞合入):脚本开头是 set +e,这里和第 63 行的 lcov 合并失败(例如某个分片的 tracefile 损坏)时都不会中止。C++ 合并失败而 Python 数据正常时,coverage-full.info 和 coverage-diff.info 都不会生成,第 84 行的 cp 仍会成功;随后 coverage_judge.sh 调用的 coverage_lines.py 遇到不存在的文件会直接返回 0,C++ 覆盖率门禁会静默通过。第 40-48 行已经对缺失分片显式 exit 101,建议对两处 lcov 合并也检查返回值并以同样方式失败。

Comment thread ci/coverage_common.sh Outdated
# defines functions and does not execute anything by itself.

# Install lcov from the cached tarball (shared CFS cache at /root/.cache).
function install_lcov() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 可维护性建议(跨模块架构,不阻塞合入):install_lcov 抽成公共函数后,ci/coverage_all_info.sh:21-31 仍保留一份逐行相同的安装代码(全量覆盖率流程由 ci/utils.sh:1022 调用)。以后升级 lcov 版本或更换下载地址需要同时改两处。建议 coverage_all_info.sh 也 source coverage_common.sh 并调用 install_lcov。

Comment thread ci/coverage_collect.sh Outdated

# Concatenate shard cost data into the canonical CFS path
mkdir -p /home/data/cfs/coverage/${PR_ID}/${COMMIT_ID}
cat ${SHARD_ROOT}/shard-*/CTestCostData.txt > /home/data/cfs/coverage/${PR_ID}/${COMMIT_ID}/CTestCostData.txt 2>/dev/null || true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

新版本改为按用例名去重,但还有两个问题:

  1. 第 71-73 行注释说每个分片只记录了实际运行的用例,这与实际不符。ci/utils.sh:787-789 在测试前会把全量基线 CTestCostData.txt 复制到 Testing/Temporary,所以每个分片上传的文件都包含全部用例,未在本分片运行的用例保留的是基线旧值。
  2. lines[$1] = $0 保留最后读到的记录,shard-* 展开后 shard-3 最后读入,因此除 shard-3 实际运行的用例外,其它分片的真实耗时都会被基线旧值覆盖。另外 --- 之后的失败用例名会被丢弃,最终只输出一个空的 ---。

建议各分片上传 CostData 时一并上传本分片的 ut_list,合并时每个用例只取其所属分片的记录,失败用例名合并去重后放在 --- 之后。

Comment thread ci/coverage_info.sh
# "Coverage collect" job merges all shard reports and does the diff/judge part.
if [ "${COVERAGE_SHARD_ONLY:-OFF}" == "ON" ]; then
echo "::group::Gen python full report (shard ${SHARD_INDEX})"
coverage combine $(ls python-coverage.data.*) || NO_PYTHON_COVERAGE_DATA=1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gen_python_full_report 已移到公共文件,原问题中的 extract/remove 部分已消除。但分片分支第 188-190 行与普通路径第 208-214 行仍重复 coverage combine、coverage xml、coverage_python_coverage.py 三步,而且后续处理已经出现差异(分片分支用 if 判断 NO_PYTHON_COVERAGE_DATA,普通路径用 || true)。建议把这三步连同 gen_python_full_report 放进 coverage_common.sh 的同一个函数,两条路径共用。

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants