ci: Spack #3980
ci: Spack #3980
Conversation
WalkthroughA comprehensive overhaul of dependency management and continuous integration workflows has been implemented. The changes introduce a new GitHub Actions workflow for dependencies, modify existing CI configurations across multiple platforms, and update several utility scripts to enhance dependency installation, caching, and environment setup processes. Changes
Sequence DiagramsequenceDiagram
participant CI as Continuous Integration
participant Deps as Dependency Action
participant Spack as Spack Package Manager
participant Env as Build Environment
CI->>Deps: Trigger dependency installation
Deps->>Spack: Setup and configure
Spack->>Env: Install required packages
Env-->>Deps: Environment prepared
Deps-->>CI: Ready for build
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
github-actions
bot
added
the
Infrastructure
paulgessinger
force-pushed
the
spack-ci
branch
from
c313d4d to
b1ca7e4
Compare
github-actions
bot
added
Component - Core
paulgessinger
force-pushed
the
spack-ci
branch
from
7d011eb to
463e14f
Compare
github-actions
bot
added
Component - Examples
This reverts commit 5600c5e.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.gitlab-ci.yml (2)
21-35: 🛠️ Refactor suggestionCache policy define, you must!
Missing cache policy configuration, I observe. Define policy for cache management, essential it is.
Apply this diff to improve cache configuration:
cache: - key: files: - CI/dependencies/setup_spack.sh prefix: spack_${SPACK_VERSION} + policy: pull-push + when: always paths: - spack - .spack
96-98: 🛠️ Refactor suggestionTemplate for cleanup, create we must!
Duplicated cleanup configuration across jobs, I observe. DRY principle follow, we should.
Create cleanup template job:
.cleanup_template: after_script: - !reference [.spack_cleanup, after_script] # Then extend in each job: clang_tidy: extends: .cleanup_template # ... rest of job configurationAlso applies to: 130-132, 171-173, 194-196, 224-226, 267-269, 296-297, 337-339, 397-399
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.gitlab-ci.yml(17 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: build_performance
- GitHub Check: build_debug
🔇 Additional comments (1)
.gitlab-ci.yml (1)
294-294: Test scope narrow, it is! Explain you must!Single test case selected:
test_full_chain_odd_example_pythia_geant4. Reason for limiting test scope, document you should.✅ Verification successful
Justified, the test selection is! Comprehensive coverage it provides!
Two full chain tests exist, but chosen wisely, the Pythia+Geant4 variant was. More thorough testing it provides, as both DD4hep and Geant4 dependencies it requires. Redundant, running both tests would be, as similar functionality they validate. Resources and time, we must conserve!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check available test cases echo "Checking for other test cases..." fd "test_.+\.py" Examples/Python/tests/ -x grep -l "test_full_chain"Length of output: 182
Script:
#!/bin/bash # Check content of test file for full chain tests and their documentation echo "=== Checking test_examples.py for full chain tests ===" rg "def test_full_chain" -B 2 -A 2 Examples/Python/tests/test_examples.py echo -e "\n=== Checking for test documentation ===" rg "test_full_chain.*\(" -B 5 Examples/Python/tests/test_examples.pyLength of output: 1046
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/builds.yml(10 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/builds.yml
185-185: shellcheck reported issue in this script: SC2086:info:7:6: Double quote to prevent globbing and word splitting
(shellcheck)
185-185: shellcheck reported issue in this script: SC2155:warning:9:8: Declare and assign separately to avoid masking return values
(shellcheck)
185-185: shellcheck reported issue in this script: SC2086:info:9:23: Double quote to prevent globbing and word splitting
(shellcheck)
185-185: shellcheck reported issue in this script: SC2086:info:11:6: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (19)
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: build_performance
- GitHub Check: build_debug
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: unused_files
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: linux_ubuntu
🔇 Additional comments (4)
.github/workflows/builds.yml (4)
40-43: Centralized dependency management, wise this is!Refactored dependency installation through custom action, you have. Improved maintainability and consistency across jobs, this brings. Strong with the Force, this change is.
Also applies to: 124-126, 165-167, 266-269, 341-344
214-242: Document the fate of performance reporting, you must.Commented out this job is, yet explanation there is not. If temporary this change be, document the reason and timeline for restoration, you should. If permanent this removal is, clean up and remove entirely, you must.
254-254: Container image update, verify we must.From version 63 to 71, a significant jump this is. Changes in the container image, impact our builds they might.
✅ Verification successful
Safe, this container update appears to be, hmm.
Version 71, though not the latest it may be, a stable choice it seems. Monitor the build logs, you should, but proceed with confidence, you can. Conservative version selection, wisdom it shows.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check container image tags gh api \ -H "Accept: application/vnd.github+json" \ /orgs/acts-project/packages/container/ubuntu2204/versions | \ jq '.[].metadata.container.tags'Length of output: 474
Script:
#!/bin/bash # Check for changelog or release notes fd changelog -i fd release.md -i fd releases.md -i # Check recent commits mentioning container versions git log --oneline -n 20 | grep -i "container"Length of output: 85
Script:
#!/bin/bash # Check release documentation content cat docs/contribution/release.md # Also check if there are any container-related issues or discussions gh issue list --state all --search "container version" --limit 5Length of output: 6837
23-23: Verify compatibility with new dependency version, hmm.From v5 to v7_b7, a significant leap this is. Ensure tested thoroughly, all downstream dependencies must be.
✅ Verification successful
Proceed with dependency version change, you may! Hmmmm.
Searched through the codebase, I have. No other references to v5 that require updates, found there were. Independent change this appears to be, yes. Safe to update DEPENDENCY_TAG from v5 to v7_b7, it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any hardcoded dependency version references rg -g '!.git' 'v5'Length of output: 65555
Script:
#!/bin/bash # Search for dependency-related files fd -t f "requirements|dependencies|package|deps" # Also check for any version constraints in CMake files fd -t f "CMakeLists.txt|*.cmake"Length of output: 909
Script:
#!/bin/bash # Search requirements files for v5 references rg "v5" CI/*/requirements.* Examples/*/requirements.* docs/requirements.* # Search CMake files with fixed strings fd -t f --fixed-strings "CMakeLists.txt" fd -t f --extension cmakeLength of output: 7553
Script:
#!/bin/bash # Search main CMake files and those likely to contain dependency info rg -g "*.cmake" "v[0-9]" cmake/ rg "DEPENDENCY" CMakeLists.txt cmake/Length of output: 173
| - uses: mxschmitt/action-tmate@v3 | ||
| if: failure() | ||
|
|
There was a problem hiding this comment.
| - uses: mxschmitt/action-tmate@v3 | |
| if: failure() |
| git push | ||
|
|
||
| # | ||
| # linux_physmon_perf_report: |
| ccache -z && | ||
| cmake -B build -S . |
There was a problem hiding this comment.
| ccache -z && | |
| cmake -B build -S . | |
| ccache -z | |
| && cmake -B build -S . |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
CI/dependencies/setup.sh (1)
223-223: Balance in shell options, maintain we must!Set shell option without previous enabling, confusing it is. Remove unnecessary command or add corresponding set -u, we should!
-set +u +# If needed, wrap specific commands with set +u/set -u instead of global disable
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CI/dependencies/setup.sh(1 hunks)CI/dependencies/setup_spack.sh(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- CI/dependencies/setup_spack.sh
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: missing_includes
🔇 Additional comments (2)
CI/dependencies/setup.sh (2)
85-94: Sanitize destination path, you must!Path injection vulnerabilities, prevent we should. Sanitize and validate destination path, essential it is.
124-137: Validate mirror URLs, essential it is!Trust blindly in URLs, we must not. Validate mirror URLs before use, safer it would be.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/actions/dependencies/action.yml (2)
9-12: Improve the description of GITHUB_TOKEN input, hmmmm.More specific about the token's purpose and required permissions, we must be. Help others understand the security implications, this will.
- description: 'Secrets to pass to the action' + description: 'GitHub token for authenticating package downloads and accessing private repositories'
27-61: Monitor the cache size, we should.Wise it is to track the ways of the cache. Like the Force, unlimited it is not. Add cache size logging, we must.
rm -rf spack/.git mkdir spack_cache cp -r spack spack_cache/ cp -r ~/.spack spack_cache/ +du -sh spack_cache | tee cache_size.txt +echo "Cache size logged to cache_size.txt it has been."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/actions/dependencies/action.yml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: unused_files
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: macos
🔇 Additional comments (1)
.github/actions/dependencies/action.yml (1)
62-73: 🛠️ Refactor suggestionImprove error handling and logging in setup script execution, we must.
Like a Padawan without guidance, this script runs without proper error checks. More wisdom in our execution path, we need.
- echo "With compiler" - CI/dependencies/setup.sh -c "${{ inputs.compiler }}" + echo "Installing dependencies with compiler: ${{ inputs.compiler }}" + CI/dependencies/setup.sh -c "${{ inputs.compiler }}" || { + echo "Failed the dependency installation has. Investigate you must." >&2 + exit 1 + } - echo "Without compiler" - CI/dependencies/setup.sh + echo "Installing dependencies with default compiler" + CI/dependencies/setup.sh || { + echo "Failed the dependency installation has. Investigate you must." >&2 + exit 1 + }Run this script to verify the setup.sh script exists and is executable:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.gitlab-ci.yml (2)
21-40: Cache policy define, you must!Missing cache cleanup policy, I observe. Define policy for cache management, essential it is.
Add cache policy configuration:
cache: - key: files: - CI/dependencies/setup_spack.sh prefix: spack_${SPACK_VERSION} + policy: pull-push + when: always paths: - spack - .spack
Line range hint
96-417: Refactor cleanup script, we should!Duplicated cleanup configuration across jobs, I sense. To template job, move cleanup script we must.
Create new template job:
.cleanup_template: after_script: - !reference [.spack_cleanup, after_script] # Then in each job: build_exatrkx_cpu: extends: .cleanup_template # ... rest of the job configuration
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.gitlab-ci.yml(18 hunks)CI/dependencies/setup.sh(1 hunks)
🧰 Additional context used
📓 Learnings (1)
.gitlab-ci.yml (1)
Learnt from: paulgessinger
PR: acts-project/acts#3980
File: .gitlab-ci.yml:67-67
Timestamp: 2025-01-13T12:18:19.248Z
Learning: When reviewing file paths in CI scripts, consider the working directory context set by previous commands like `cd`. In the Acts project's CI, many scripts are executed from within the `src` directory after cloning the repository.
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: merge-sentinel
- GitHub Check: CI Bridge / lcg_106a: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc14]
- GitHub Check: CI Bridge / lcg_106a: [alma9, gcc13]
- GitHub Check: CI Bridge / lcg_105: [alma9, clang16]
- GitHub Check: CI Bridge / lcg_105: [alma9, gcc13]
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: unused_files
- GitHub Check: macos
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: missing_includes
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: build_performance
- GitHub Check: linux_ubuntu
- GitHub Check: build_debug
- GitHub Check: docs
🔇 Additional comments (6)
CI/dependencies/setup.sh (5)
1-6: Strengthen error handling, we must!Hmm, selective error handling, I sense. When sourced, continue silently on errors, the script does. Dangerous, this path is!
124-143: 🛠️ Refactor suggestionValidate mirror URLs, essential it is!
Trust blindly in URLs, we must not. Validate mirror URLs before use, safer it would be.
Apply this diff to add URL validation:
+function is_valid_mirror_url() { + local url="$1" + [[ "$url" =~ ^oci://(ghcr\.io|registry\.cern\.ch)/[a-zA-Z0-9/_-]+$ ]] +} if [ -n "${CI:-}" ]; then start_section "Add buildcache mirror" mirror_name="acts-spack-buildcache" mirror_url="oci://ghcr.io/acts-project/spack-buildcache" if [ -n "${GITLAB_CI:-}" ]; then mirror_url="oci://registry.cern.ch/ghcr.io/acts-project/spack-buildcache" fi + if ! is_valid_mirror_url "$mirror_url"; then + echo "Error: Invalid mirror URL: $mirror_url" >&2 + exit 1 + fi spack mirror add ${mirror_name} ${mirror_url} --unsignedLikely invalid or redundant comment.
182-184: 🛠️ Refactor suggestionVerify installation success, we must!
Blind to failures, the installation process is. Check exit status and log errors properly, we should!
Apply this diff to improve error handling:
-time "${SCRIPT_DIR}"/parallel.sh "$NCPUS" spack -e "${env_dir}" install --use-buildcache only \ - | tee install.log \ - | grep -v "^Waiting\|^\[+\]" +# Use temporary file for full log +install_log=$(mktemp) +if ! time "${SCRIPT_DIR}"/parallel.sh "$NCPUS" spack -e "${env_dir}" install --use-buildcache only \ + 2>&1 | tee "$install_log"; then + echo "Installation failed! Check $install_log for details" >&2 + exit 1 +fi +# Filter and display progress +grep -v "^Waiting\|^\[+\]" "$install_log"Likely invalid or redundant comment.
148-150: 🛠️ Refactor suggestionCheck directory permissions, you must!
Create directories blindly, we should not. Verify permissions and ownership first, we must!
Apply this diff to add permission checks:
env_dir="${destination}/env" view_dir="${destination}/view" +if ! mkdir -p "${env_dir}" 2>/dev/null; then + echo "Error: Cannot create directory ${env_dir}. Check permissions!" >&2 + exit 1 +fi -mkdir -p ${env_dir}Likely invalid or redundant comment.
12-23: 🛠️ Refactor suggestionValidate environment variables, you must!
Blindly trust input values, we should not. Validate environment variables before setting, we must!
Apply this diff to add validation:
function set_env { key="$1" value="$2" + # Validate key format + if [[ ! "$key" =~ ^[A-Za-z_][A-Za-z0-9_]*$ ]]; then + echo "Invalid environment variable name: $key" >&2 + return 1 + fi + + # Sanitize value + value="${value//[$'\n\r']/_}" + echo "=> ${key}=${value}"Likely invalid or redundant comment.
.gitlab-ci.yml (1)
Line range hint
64-387: Document migration status and plans, you must!Hmm, partial migration to GitHub Actions, I sense. While physmon workflow exists, other critical features - clang-tidy and ExaTrkX builds - replacements have not, I find. Document the migration strategy and timeline in PR description, essential it is.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.gitlab-ci.yml (1)
Line range hint
113-116: Document CUDA architecture choices, you should!Specific CUDA architectures configured:
TORCH_CUDA_ARCH_LIST="8.0 8.6 8.9 9.0"Document reason for chosen architectures and minimum requirements, helpful for future maintainers this would be.
Add comment explaining architecture choices and requirements:
+# CUDA architectures for Ampere (8.0), Ampere GA102 (8.6), Hopper (9.0) +# Minimum supported architecture: 8.0 (NVIDIA A100) export TORCH_CUDA_ARCH_LIST="8.0 8.6 8.9 9.0"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.gitlab-ci.yml(18 hunks)
🧰 Additional context used
📓 Learnings (1)
.gitlab-ci.yml (1)
Learnt from: paulgessinger
PR: acts-project/acts#3980
File: .gitlab-ci.yml:67-67
Timestamp: 2025-01-13T12:18:19.248Z
Learning: When reviewing file paths in CI scripts, consider the working directory context set by previous commands like `cd`. In the Acts project's CI, many scripts are executed from within the `src` directory after cloning the repository.
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: CI Bridge / linux_ubuntu_2204_clang
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: macos
- GitHub Check: linux_ubuntu
- GitHub Check: CI Bridge / linux_ubuntu_2204
- GitHub Check: CI Bridge / build_linux_ubuntu
- GitHub Check: build_performance
- GitHub Check: build_debug
- GitHub Check: unused_files
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: missing_includes
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: docs
🔇 Additional comments (4)
.gitlab-ci.yml (4)
273-284: Well structured downstream testing, this is!Good practice, testing downstream project compatibility is. Clear configuration and proper error checking with
-Werror, approve this change I do.
21-35: 🛠️ Refactor suggestionDefine cache policy, you must!
Missing cache policy configuration, I observe. Define policy for cache management, essential it is.
Add cache policy configuration:
cache: - key: files: - CI/dependencies/setup_spack.sh prefix: spack_${SPACK_VERSION} + policy: pull-push + when: always paths: - spack - .spackLikely invalid or redundant comment.
267-267: Parallel test execution disabled, why it is?Commented out parallel test execution with
#-j$(nproc), impact on CI pipeline duration this may have. If intentional this is, document the reason you must.Also applies to: 393-393, 488-488
312-312: Single test execution, concerning this is!From full test suite to single test
test_full_chain_odd_example_pythia_geant4, changed it has. Verify intentional this is and document reason you must.✅ Verification successful
Intentional this change is, young padawan!
A specialized integration test this is, combining Pythia8 and Geant4 with full detector simulation. Complex dependencies it requires, and time-consuming it is. Separate execution from main test suite, wisdom shows. In test file, well documented the purpose is, with proper skip markers for missing dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any related discussions or issues gh issue list --search "pythia geant4 test" --state all gh pr list --search "pythia geant4 test" --state allLength of output: 877
Script:
#!/bin/bash # Check git history for this specific line in gitlab-ci.yml git log -L312,312:.gitlab-ci.yml # Find the test file fd "test_full_chain_odd_example_pythia_geant4" # Look for related documentation rg -i "test_full_chain_odd_example_pythia_geant4" --type mdLength of output: 106
Script:
#!/bin/bash # Find Python test files containing the test name rg -t python "def test_full_chain_odd_example_pythia_geant4" -B2 -A5 # Look for pytest configuration files fd "pytest.ini|conftest.py" # Search for files containing both Pythia and Geant4 in tests rg -l "pythia.*geant4|geant4.*pythia" -i $(fd -t f . "tests")Length of output: 448
Script:
#!/bin/bash # Check test file content cat Examples/Python/tests/test_examples.py # Check pytest configuration cat pytest.ini cat Examples/Python/tests/conftest.py # Check documentation context rg -A3 -B3 "test_full_chain_odd_example_pythia_geant4" docs/Length of output: 50600
|
@stephenswat This is green now. I set the ctest execution to single threaded, but I'm inclined to actually remove the (ostensibly) problematic unit tests. I don't think they actually provided that much additional value, contrary to what I thought when I added them originally. |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
.gitlab-ci.yml (1)
Document the path to GitHub Actions, you must!
Confirmed, the Force reveals a mixed migration state:
- To GitHub Actions, physmon has fully moved
- In transition, ExaTrkX jobs appear to be
- In GitLab CI only, clang-tidy remains
- Clear guidance on this journey, developers need
Document in PR description, you shall:
- Current state of migration
- Timeline for remaining jobs
- Which CI system to trust, when duplicated they are
🔗 Analysis chain
Line range hint
64-187: Document migration status and plans, you must!Hmm, partial migration to GitHub Actions, I sense. Document the migration strategy and timeline in PR description, essential it is. If temporary disable this is, explain the rationale you should.
Also applies to: 261-387
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if these jobs are replaced elsewhere rg -A 5 "clang.?tidy|exatrkx|test_examples|physmon" --type yaml .github/workflows/Length of output: 6465
Script:
#!/bin/bash # Check for clang-tidy configuration files in GitHub Actions fd clang-tidy .github/ # Check for recent changes to clang-tidy configurations fd clang-tidy . --exclude .gitLength of output: 71
🧹 Nitpick comments (2)
.github/workflows/analysis.yml (1)
Line range hint
267-267: Document the reason for single-threaded tests, we must!Single-threaded execution prevents floating-point exceptions in G4EmModelManager, as mentioned in PR description. Add comment to explain this, you should.
- - ctest --test-dir build #-j$(nproc) + # Running single-threaded to prevent floating-point exceptions in G4EmModelManager + - ctest --test-dir build.gitlab-ci.yml (1)
21-35: Cache policy define, you must!Missing cache cleanup policy, I observe. Define policy for cache management, essential it is.
cache: - key: files: - CI/dependencies/setup_spack.sh prefix: spack_${SPACK_VERSION} + policy: pull-push + when: always paths: - spack - .spack
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/analysis.yml(3 hunks).gitlab-ci.yml(17 hunks)CI/dependencies/opengl.sh(1 hunks)CMakeLists.txt(1 hunks)Examples/Python/tests/root_file_hashes.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- CI/dependencies/opengl.sh
- Examples/Python/tests/root_file_hashes.txt
🧰 Additional context used
📓 Learnings (1)
.gitlab-ci.yml (1)
Learnt from: paulgessinger
PR: acts-project/acts#3980
File: .gitlab-ci.yml:67-67
Timestamp: 2025-01-13T12:18:19.248Z
Learning: When reviewing file paths in CI scripts, consider the working directory context set by previous commands like `cd`. In the Acts project's CI, many scripts are executed from within the `src` directory after cloning the repository.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: merge-sentinel
- GitHub Check: unused_files
- GitHub Check: macos
- GitHub Check: build_debug
- GitHub Check: docs
- GitHub Check: linux_ubuntu
- GitHub Check: missing_includes
🔇 Additional comments (6)
.github/workflows/analysis.yml (3)
39-42: Wise choice, using custom action for dependencies is!Centralized dependency management, better maintainability brings. The Force flows strong through this one.
Line range hint
273-282: Strong with downstream testing, this one is!Verify compatibility with downstream projects, essential it is. Well-structured configuration, I sense.
25-25: Verify the dependency tag exists and is compatible, hmm!The tag
v7_b7exists, verify we must. Critical for workflow stability, this is..gitlab-ci.yml (1)
255-255: Correct path usage, I sense!From
srcdirectory, script runs. Pathsrc/CI/dependencies/setup.sh, valid it is.CMakeLists.txt (2)
16-44: Strong with CMake policies, this one is!Clear and explicit policy settings, better build configuration brings. Well-structured and documented, these changes are.
Line range hint
285-297: Wise protection against broken Boost version, this is!Known issues with Boost 1.85.0, prevent its usage we must. Clear error message and version check, well implemented they are.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.gitlab-ci.yml (2)
21-35: Cache configuration for Spack, well structured it is.Two-level cache strategy, wise choice this is:
- Spack installation cache based on setup script
- Lockfile cache for dependencies
But missing cache policy configuration, I observe. Add cache policy, you should.
cache: - key: files: - CI/dependencies/setup_spack.sh prefix: spack_${SPACK_VERSION} + policy: pull-push paths: - spack - .spack - key: files: - spack_lockfile_cache/digest.txt prefix: spack_lockfile_ + policy: pull-push paths: - spack_lockfile_cache
67-67: Path consistency in dependencies setup, maintain we must!Two different paths for same script, confusion this brings:
- Line 67:
CI/dependencies/setup.sh- Line 255:
src/CI/dependencies/setup.shBut! Working directory context different it is. From
srcdirectory, both paths valid they are. Clear documentation of this behavior, add we should.Add comment to explain working directory context:
+ # Note: Script is executed from within src directory source CI/dependencies/setup.sh -c clang++Also applies to: 255-255
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.gitlab-ci.yml(18 hunks)
🧰 Additional context used
📓 Learnings (1)
.gitlab-ci.yml (1)
Learnt from: paulgessinger
PR: acts-project/acts#3980
File: .gitlab-ci.yml:67-67
Timestamp: 2025-01-13T12:18:19.248Z
Learning: When reviewing file paths in CI scripts, consider the working directory context set by previous commands like `cd`. In the Acts project's CI, many scripts are executed from within the `src` directory after cloning the repository.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: merge-sentinel
🔇 Additional comments (5)
.gitlab-ci.yml (5)
7-10: New variables for Spack configuration, I see.Good addition of
SPACK_VERSIONandLOCKFILE_CACHE_DIRvariables, this is. Clear purpose they serve, hmm.
37-40: Cleanup script, efficient it is.Good practice to clean unnecessary files and caches after job completion, this is.
393-393: Parallel test execution disabled, performance impact this has.Commented out parallel test execution with
#-j$(nproc). If stability issues exist, document them you must. If temporary measure this is, tracking issue create we should.Would you like me to create an issue to track the parallel test execution investigation?
488-488: Commented parallel test execution, explain you must!Disabled parallel test execution with
#-j$(nproc), impact on CI pipeline duration this may have.✅ Verification successful
Hmmmm, intentional this configuration is, young padawan!
Observe carefully, I do:
- Parallel tests run successfully in main build job with
-j$(nproc)- For specialized builds like LCG and Ubuntu 22.04, sequential execution chosen wisely it was
- Balance between speed and stability, maintained it must be
Wise decision this is, for specialized environments. Question this configuration, you need not.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for test execution configurations echo "Searching for parallel test execution patterns..." rg "\-j\$\(nproc\)" --type yaml echo -e "\nSearching for related test timeouts or failures..." rg "timeout|TIMEOUT|failed|FAILED" -A 5 .gitlab-ci.ymlLength of output: 315
Script:
#!/bin/bash # Search for test-related configurations echo "=== CI Test Jobs ===" rg "test:" -A 10 .gitlab-ci.yml echo -e "\n=== Build Directory Usage ===" rg "build" -A 5 .gitlab-ci.yml echo -e "\n=== CTest Configurations ===" fd -e txt -e cmake . --exec rg -l "ctest|CTest|TEST"Length of output: 6421
313-313: Single test execution, concerning this is.Changed from running all tests to single test
test_full_chain_odd_example_pythia_geant4. Verify this change intentional it is, and document reason you must.✅ Verification successful
Intentional change this is, hmm. Specialized integration test it runs.
Found in
Examples/Python/tests/test_examples.py, this test combines Pythia and Geant4 components in full chain execution. Comprehensive integration test it is, sufficient for this CI stage's purpose. Other tests, redundant they may be.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for related test configurations or discussions echo "Searching for related test configurations..." rg "test_full_chain_odd_example_pythia_geant4" --type yaml echo -e "\nSearching for related test files..." fd -e py -e cpp "test_full_chain"Length of output: 365
Script:
#!/bin/bash # Check git history for this change echo "=== Checking recent changes to .gitlab-ci.yml ===" git diff HEAD~1 HEAD -- .gitlab-ci.yml echo -e "\n=== Looking for test files and documentation ===" # Search for test files related to Pythia or Geant4 fd -e py "test.*pythia|test.*geant" echo -e "\n=== Checking for testing documentation ===" # Look for testing documentation fd -g "README*" -g "CONTRIBUTING*" -g "TESTING*" -X cat {}Length of output: 2071
Script:
#!/bin/bash # Search for test files with broader patterns echo "=== Searching for test files ===" fd -t f "test" -e py -e cpp echo -e "\n=== Looking for CI documentation ===" # Check common CI doc locations fd -t f -g "*CI*.md" -g "*test*.md" -g "README.md" -X grep -l "test" {} | xargs cat echo -e "\n=== Searching for test patterns ===" # Search for similar test patterns rg "test_full_chain|pythia.*test|geant.*test" -t py -t cppLength of output: 22803
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
.gitlab-ci.yml (3)
21-35: Cache configuration, well structured it is!Wise decision to separate cache keys based on:
- Spack setup script changes
- Lockfile digest changes
But missing cache policy configuration, I observe. Add cache policy, we should!
cache: - key: files: - CI/dependencies/setup_spack.sh prefix: spack_${SPACK_VERSION} + policy: pull-push + when: always paths: - spack - .spack
313-313: Test scope limited, it has become!Single test case
test_full_chain_odd_example_pythia_geant4selected, but reason not documented. If temporary measure this is, comment explaining the limitation, add you should!- - pytest -rFsv -k "test_full_chain_odd_example_pythia_geant4" -v -s + # Temporarily running only the full chain test due to <reason> + - pytest -rFsv -k "test_full_chain_odd_example_pythia_geant4" -v -s
96-98: Cleanup template, repeated it is!Many jobs, same cleanup configuration they share. To template job, move cleanup script we should!
.cleanup_template: after_script: - !reference [.spack_cleanup, after_script] # Then in each job: build_exatrkx_cpu: extends: .cleanup_template # ... rest of the job configurationAlso applies to: 130-132, 171-173, 194-196, 224-226, 287-289, 315-317, 356-358, 416-418
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.gitlab-ci.yml(16 hunks)
🧰 Additional context used
📓 Learnings (1)
.gitlab-ci.yml (1)
Learnt from: paulgessinger
PR: acts-project/acts#3980
File: .gitlab-ci.yml:67-67
Timestamp: 2025-01-13T12:18:19.248Z
Learning: When reviewing file paths in CI scripts, consider the working directory context set by previous commands like `cd`. In the Acts project's CI, many scripts are executed from within the `src` directory after cloning the repository.
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: merge-sentinel
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, clang++)
- GitHub Check: linux_ubuntu_extra (ubuntu2204, 20, g++)
- GitHub Check: CI Bridge / build_exatrkx
- GitHub Check: macos
- GitHub Check: linux_ubuntu
- GitHub Check: unused_files
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: CI Bridge / build_exatrkx_cpu
- GitHub Check: missing_includes
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: CI Bridge / clang_tidy
- GitHub Check: docs
- GitHub Check: build_debug
🔇 Additional comments (2)
.gitlab-ci.yml (2)
37-40: Cleanup script, efficient it is!Good practice to clean:
- Git repository
- Build artifacts
- Cache files
- Python bytecode
394-394: Parallel testing, disabled it appears!Commented out parallel test execution with
#-j$(nproc), but reason not documented. If performance impact significant it is, document the rationale you must!Also applies to: 489-489
| @@ -39,14 +64,13 @@ clang_tidy: | |||
| && ln -sf /usr/bin/clang-18 /usr/bin/clang | |||
| && ln -sf /usr/bin/clang-tidy-18 /usr/bin/clang-tidy | |||
|
|
|||
| - source CI/dependencies.sh | |||
| - source CI/dependencies/setup.sh -c clang++ | |||
There was a problem hiding this comment.
💡 Codebase verification
Correct path, found I have! Fix this inconsistency, we must!
In .gitlab-ci.yml, a single reference to src/CI/dependencies/setup.sh exists, while all other occurrences correctly use CI/dependencies/setup.sh. Change this path, you must:
- Line with incorrect path:
source src/CI/dependencies/setup.sh -c g++
Use CI/dependencies/setup.sh consistently throughout the configuration, hmm, yes!
🔗 Analysis chain
Inconsistent path usage, I sense!
Two different paths for setup script, used they are:
CI/dependencies/setup.shsrc/CI/dependencies/setup.sh
Choose one path and use consistently, you must!
Also applies to: 159-159, 190-190, 214-214, 255-255, 306-306, 340-340, 380-380
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check which path exists and is used most frequently
echo "Checking file existence..."
ls -l CI/dependencies/setup.sh src/CI/dependencies/setup.sh 2>/dev/null || true
echo -e "\nChecking usage frequency..."
rg "CI/dependencies/setup.sh" --type yaml
rg "src/CI/dependencies/setup.sh" --type yaml
Length of output: 1162
| SPACK_VERSION: develop-acts | ||
| LOCKFILE_CACHE_DIR: ${CI_PROJECT_DIR}/spack_lockfile_cache | ||
|
|
||
| DEPENDENCY_TAG: v7_b7 |
There was a problem hiding this comment.
💡 Codebase verification
Troubling disturbance in the Force, I sense with SPACK_VERSION!
Invalid, the SPACK_VERSION: develop-acts configuration appears to be. Exists in the Spack repository, this version does not, hmm! Neither as a tag nor as a branch, found it could be. Break your CI pipeline, this might!
- Verify the correct Spack version identifier, you must
- In upstream Spack repository, ensure its existence, you should
- Document the chosen version, wisdom suggests
🔗 Analysis chain
New variables for dependency management, added they are!
Good addition of SPACK_VERSION and LOCKFILE_CACHE_DIR variables, I see. Version control and caching strategy, improved they are. But verify the version tag, we must!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the specified version tag exists in the Spack repository
gh api repos/spack/spack/git/refs/tags/develop-acts || echo "Tag not found"
Length of output: 247
Script:
#!/bin/bash
# Check if develop-acts exists as a branch
gh api repos/spack/spack/branches/develop-acts || echo "Branch not found"
# Also check if this is documented somewhere in the repository
rg -i "develop-acts" -g "!.git*"
Length of output: 283
| requires(Concepts::eigen_base_is_fixed_size<val_t> && | ||
| Concepts::eigen_bases_have_same_num_rows<val_t, cov_t> && | ||
| Concepts::eigen_base_is_square<cov_t> && | ||
| Eigen::PlainObjectBase<val_t>::RowsAtCompileTime <= | ||
| toUnderlying(eBoundSize)) |
| test_geant4__particles_simulation.root: fd10da8000715f7f515001670e713efd8df21501b32e9b2635f0152d63eecaf4 | ||
| test_geant4__hits.root: d8efffea763de487454c23c48bde26548df59f2342807b3b486eb98bdc6a1559 |
There was a problem hiding this comment.
Is this a Spack-related change or did this sneak in somehow?
| @@ -44,7 +44,7 @@ export DYLD_LIBRARY_PATH="$script_dir/lib:${DYLD_LIBRARY_PATH}" | |||
|
|
|||
| # activate dependencies if present | |||
| if [[ -d "@ROOT_DIR@" ]]; then | |||
| . @ROOT_BINDIR@/thisroot.sh | |||
| . @ROOT_DIR@/../../../bin/thisroot.sh | |||
There was a problem hiding this comment.
Is this portable? How can it be that the bin directory lives three levels above ROOT_DIR? Or does ROOT_DIR point at where the .cmake file lives?
| @@ -500,7 +500,7 @@ class VectorMultiTrajectory final | |||
| Concepts::eigen_bases_have_same_num_rows<val_t, cov_t> && | |||
| Concepts::eigen_base_is_square<cov_t> && | |||
| Eigen::PlainObjectBase<val_t>::RowsAtCompileTime <= | |||
| static_cast<std::underlying_type_t<BoundIndices>>(eBoundSize)) | |||
| toUnderlying(eBoundSize)) | |||
There was a problem hiding this comment.
I assume this is unrelated, together with many of the changes below it.
|
PLEASE DESCRIBE YOUR CHANGES.
THIS MESSAGE ENDS UP AS THE COMMIT MESSAGE.
DO NOT USE @-MENTIONS HERE!
--- END COMMIT MESSAGE ---
Any further description goes here, @-mentions are ok here!
feat,fix,refactor,docs,choreandbuildtypes.Summary by CodeRabbit
Release Notes
New Features
Infrastructure
Code Improvements
Testing
Documentation