[CI] Compare codegen backend to default #1154
Conversation
|
Warning Rate limit exceeded@runtingt has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 21 minutes and 33 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughModified the CVMFS integration action to support output directory setup with workspace mounting. Enhanced the CI workflow to support multiple build paths (CMSSW AL9 and LCG variants) with conditional execution, expanded matrix configuration, and artifact collection. Introduced a new codegen output comparison utility script. Changes
Sequence DiagramsequenceDiagram
participant CI as CI Workflow
participant Matrix as Matrix Dispatch
participant Build as Build Step
participant Tests as Test Execution
participant Output as Output Collection
CI->>Matrix: Determine build type<br/>(CMSSW or LCG)
alt CMSSW_VERSION set
Matrix->>Build: Build Combine (CMSSW)<br/>use cms.cern.ch
else CMSSW_VERSION empty
Matrix->>Build: Build Combine (LCG)<br/>use sft.cern.ch
end
Build->>Build: Setup output directory<br/>via run-in-cvmfs
Build->>Tests: Execute test suite<br/>(counting, parametric, etc.)
Tests->>Output: Generate outputs
Output->>Output: Collect artifacts<br/>per matrix configuration
Output->>Output: Compare codegen outputs<br/>validate consistency
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
runtingt
force-pushed
the
trunting/AD-in-CI
branch
2 times, most recently
from
8abc0e3 to
2c05352
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/actions/run-in-cvmfs/action.yml(1 hunks).github/workflows/development-tests-ci.yml(2 hunks)test/codegen/compare_codegen.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
test/codegen/compare_codegen.py
79-79: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
80-80: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
110-110: Parenthesize a and b expressions when chaining and and or together, to make the precedence clear
Parenthesize the and subexpression
(RUF021)
runtingt
force-pushed
the
trunting/AD-in-CI
branch
from
2c05352 to
da00003
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/development-tests-ci.yml(2 hunks)test/codegen/compare_codegen.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.2)
test/codegen/compare_codegen.py
80-80: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
81-81: Do not call getattr with a constant attribute value. It is not any safer than normal property access.
Replace getattr with attribute access
(B009)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: LCG_108 - ROOT 6.36.02
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: LCG_102 - ROOT 6.26.04
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: LCG_106 - ROOT 6.32.02
🔇 Additional comments (8)
.github/workflows/development-tests-ci.yml (5)
12-32: LGTM!The matrix configuration appropriately covers multiple environments (CMSSW and LCG) with different ROOT versions. Disabling
fail-fastensures all configurations are tested even if one fails.
36-93: LGTM!The conditional build logic appropriately separates LCG and CMSSW builds. The output directory detection correctly handles the different filesystem layouts for each environment.
95-262: LGTM!The test execution steps provide comprehensive coverage of different analysis scenarios (counting, parametric, template, RooMultiPdf, etc.) with consistent patterns for comparing nominal and codegen backends.
264-283: LGTM!The artifact handling is well-structured with proper name sanitization and conditional upload. The comparison step correctly invokes the Python utility to validate outputs.
5-11: Review comment is incorrect. Remove this concern.The workflow does not execute untrusted PR code. The
actions/checkout@v4action on line 36 lacks an explicitrefparameter, which means it defaults togithub.ref. Forpull_request_targetevents,github.refresolves to the base branch, not the PR branch. All subsequent steps execute scripts and data from the trusted base branch only—no PR branch code is accessed. The label gate (if: github.event.label.name == 'testAD') provides additional safety by requiring explicit approval. The security concern is not applicable here.Likely an incorrect or invalid review comment.
test/codegen/compare_codegen.py (3)
7-12: LGTM!The
ComparisonResultenum appropriately categorizes all possible comparison outcomes.
25-31: LGTM!The function correctly identifies keys without codegen counterparts.
34-47: LGTM!The set-based file comparison correctly identifies missing files on both sides.
runtingt
force-pushed
the
trunting/AD-in-CI
branch
2 times, most recently
from
fdfc37c to
59bf89d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/codegen/compare_codegen.py (1)
58-96: Resource management is fixed, but POI detection could be more robust.The try/finally block correctly ensures ROOT files are closed, and direct attribute access for
deltaNLLis appropriate. However, the POI detection on lines 76-77 using"r_" in b.GetName()could unintentionally match branches containing "r_" anywhere in the name (e.g., "trigger", "error", "parent").Consider this diff for more precise POI matching:
- pois_1 = [b.GetName() for b in tree1.GetListOfBranches() if "r_" in b.GetName() or "r" == b.GetName()] - pois_2 = [b.GetName() for b in tree2.GetListOfBranches() if "r_" in b.GetName() or "r" == b.GetName()] + pois_1 = [b.GetName() for b in tree1.GetListOfBranches() if b.GetName().startswith("r_") or b.GetName() == "r"] + pois_2 = [b.GetName() for b in tree2.GetListOfBranches() if b.GetName().startswith("r_") or b.GetName() == "r"].github/workflows/development-tests-ci.yml (1)
95-263: Consider adding error handling to test commands.The test steps follow a consistent pattern, which is good. However, the commands don't check for success before moving outputs. If a
combinecommand fails, the subsequentmvmight silently succeed with no files or fail itself, potentially masking the original failure until the comparison step.Consider adding explicit error checking:
$COMBINE_COMMAND +if [ $? -ne 0 ]; then + echo "ERROR: Combine command failed" + exit 1 +fi mv higgsCombine*.root $OUTDIR/countingOr use shell's error-on-fail mode at the start of each script:
script: |- + set -e COMBINE_COMMAND="..."This ensures that any command failure immediately stops the workflow step, making debugging easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/development-tests-ci.yml(2 hunks)test/codegen/compare_codegen.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: CMSSW_14_1_0_pre4 - ROOT 6.30.07
- GitHub Check: Compile (py3.10, root6.26.4)
- GitHub Check: Compile (py3.10, root6.32.2)
- GitHub Check: Compile (py3.12, root6.34.4)
- GitHub Check: LCG_106 - ROOT 6.32.02
- GitHub Check: LCG_102 - ROOT 6.26.04
- GitHub Check: CMSSW_14_0_0_pre1 - ROOT 6.26.11
- GitHub Check: dev3/latest - ROOT LCG master
- GitHub Check: CMSSW_11_3_4 - ROOT 6.22.09
- GitHub Check: LCG_108 - ROOT 6.36.02
🔇 Additional comments (9)
test/codegen/compare_codegen.py (5)
1-4: LGTM!The imports are appropriate for the script's functionality.
7-12: LGTM!The enum is well-defined with clear result states.
15-30: LGTM!The function correctly handles both nominal and codegen directories, and the modification time sorting includes a safe fallback for missing nominal directories. Previous review concerns have been addressed.
33-55: LGTM!Both functions correctly validate the presence and file contents of codegen counterpart directories.
109-137: LGTM!The comparison logic correctly validates codegen counterparts, file mismatches, and file contents. The empty directory check (lines 125-129) now properly validates both nominal and codegen directories, addressing previous review concerns.
.github/workflows/development-tests-ci.yml (4)
1-33: LGTM!The workflow configuration is well-structured with appropriate triggers (workflow_dispatch and label-based), a comprehensive matrix covering multiple ROOT and build environment combinations, and clear job naming.
34-93: LGTM!The build steps correctly handle both LCG and CMSSW paths with appropriate conditional execution and CVMFS repository selection. The output directory setup properly distinguishes between the two build environments.
264-277: LGTM!The artifact handling is well-designed. The name sanitization prevents path issues, and the
if: always()condition ensures outputs are collected even when tests fail, which is valuable for debugging.
279-283: LGTM!The comparison step properly integrates the validation script, providing automated verification of codegen backend outputs against the default backend.
runtingt
force-pushed
the
trunting/AD-in-CI
branch
from
59bf89d to
e113179
Compare
|
Thanks a lot! @anigamova, can you add the |
|
The label is already there, just attached to the PR, it will fail just because the workflow runs wrt to master. But I think we can merge this. Thanks @runtingt |
3 participants
Adding the
testADlabel to any PR should (hopefully) run these testsSummary by CodeRabbit
Tests
Chores