CI container improvements inc. standalone Cetmodules#661
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a Spack repo migration script, integrates it into Dockerfile configuration, updates GCC and Spack install parallelism settings, and adjusts the Spack environment package constraints. ChangesCI Spack environment upgrade
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
@phlexbot format |
Format Fixes Applied✅ ruff fixes pushed (commit b3edef0) |
|
31 fixed, 0 new since branch point (cb81c8b) ✅ 31 CodeQL alerts resolved since the previous PR commit
✅ 31 CodeQL alerts resolved since the branch point
Review the full CodeQL report for details. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/upgrade_repos.py`:
- Around line 1-24: Unblock the Python check for upgrade_repos.py by bringing
the new script into Ruff/style compliance: add the required module docstring and
function docstrings, include from __future__ import annotations at the top, and
add type hints for the public helper upgrade_spack_repo_api and any other
functions defined in this file. Also fix the import ordering and reflow any
overlong lines to satisfy the 99-character limit, keeping the existing
REPLACEMENTS mapping and spack.repo import in a clean, lint-friendly order.
- Around line 36-48: The migration in upgrade_repos.py currently swallows all
errors in the repo.yaml update path, so failed updates can be ignored and the
build keeps going with stale metadata. Update the exception handling around the
YAML load/dump logic in the repo upgrade flow (the try/except that prints “Error
updating …”) to fail the migration: either re-raise the exception immediately or
record failures and exit non-zero after processing, so the upgrade step cannot
succeed when repo.yaml writes fail.
- Around line 68-70: The import rewrite in upgrade_repos.py is stripping or
duplicating aliases when replacing the spack.llnl.util.filesystem prefix, which
can produce invalid Python like a repeated as clause. Update the rewrite logic
in the replacement loop that builds modified_content so it matches the full
import statement and preserves any alias by capturing and reusing it, or
otherwise only rewriting the module path portion. Use the REPLACEMENTS handling
in ci/upgrade_repos.py and the import-rewrite logic around modified_content to
locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: f5cfd868-4fdf-4c01-95e0-ab6d938d96be
📒 Files selected for processing (3)
ci/Dockerfileci/spack.yamlci/upgrade_repos.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Framework-R-D/action-configure-cmake(auto-detected)Framework-R-D/action-workflow-setup(auto-detected)Framework-R-D/action-complete-pr-comment(auto-detected)Framework-R-D/action-handle-fix-commit(auto-detected)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Analyze python with CodeQL
- GitHub Check: coverage
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Use ruff for Python formatting and linting (configured inpyproject.toml); follow Google-style docstring conventions; use line length of 99 characters; use double quotes for strings
Usefrom __future__ import annotationsto enable deferred evaluation of type annotations (avoids forward-reference issues; Python >=3.12)
Type hints are recommended; mypy is configured inpyproject.toml
Avoid naming Python test scriptstypes.pyor other names that shadow standard library modules, as this causes obscure import errors
Python test scripts should follow the test structure pattern: C++ driver provides data streams, Jsonnet config wires the graph, and Python script implements algorithms
**/*.py: Enforce 99-character line limit and double quotes in Python via ruff configured inpyproject.toml
Use Google-style docstrings in Python code
Use type hints in Python code and configure mypy for type checking
Usefrom __future__ import annotationsin Python to enable deferred evaluation of type annotations
Use PEP 8 naming in Python:CapWordsfor classes,snake_casefor everything else
When using thereadtool for Python files, always use integer values foroffsetandlimitparameters, never float/double values
Files:
ci/upgrade_repos.py
🪛 ast-grep (0.44.0)
ci/upgrade_repos.py
[warning] 43-43: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(yaml_file, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 64-64: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(file_path, "r", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 72-72: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(file_path, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 GitHub Actions: greenc-FNAL checking Python code / 1_python-check.txt
ci/upgrade_repos.py
[error] 1-1: ruff (D100) Missing docstring in public module.
[error] 3-3: ruff (I001) Import block is un-sorted or un-formatted. (Organize imports)
[error] 15-15: ruff (E501) Line too long (103 > 99).
[error] 18-18: ruff (E501) Line too long (100 > 99).
[error] 83-83: ruff (D103) Missing docstring in public function.
🪛 GitHub Actions: greenc-FNAL checking Python code / python-check
ci/upgrade_repos.py
[error] 1-1: ruff (D100): Missing docstring in public module
[error] 3-9: ruff (I001): Import block is un-sorted or un-formatted. (Organize imports)
[error] 15-15: ruff (E501): Line too long (103 > 99)
[error] 18-18: ruff (E501): Line too long (100 > 99)
[error] 83-83: ruff (D103): Missing docstring in public function
🪛 OpenGrep (1.23.0)
ci/upgrade_repos.py
[ERROR] 37-37: yaml.load() without SafeLoader can execute arbitrary Python code. Use yaml.safe_load() or yaml.load(..., Loader=SafeLoader) instead.
(coderabbit.deserialization.python-yaml-unsafe-load)
🔇 Additional comments (3)
ci/spack.yaml (1)
11-11: LGTM!Also applies to: 41-46
ci/upgrade_repos.py (1)
87-90: 🎯 Functional CorrectnessNo change needed:
builtinis the Spack repo namespace for the core packages repo.
Thespack-packages/repos/spack_repo/builtincheckout resolves to that namespace, so skippingrepo.namespace == "builtin"is the right way to leave core code alone.> Likely an incorrect or invalid review comment.ci/Dockerfile (1)
295-295: 🩺 Stability & AvailabilityNo package-concurrency fix needed here.
ci/Dockerfileonly sets Spack’s per-package-jbuild jobs; it doesn’t use-p, so the suggested-p "${SPACK_INSTALL_PROCESSES:-1}"change is unnecessary.> Likely an incorrect or invalid review comment.
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #661 +/- ##
==========================================
+ Coverage 83.27% 83.59% +0.32%
==========================================
Files 162 170 +8
Lines 5912 6613 +701
Branches 670 798 +128
==========================================
+ Hits 4923 5528 +605
- Misses 796 818 +22
- Partials 193 267 +74
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
greenc-FNAL
force-pushed
the
feature/container-tweaks-with-standalone-cetmodules
branch
from
b3edef0 to
90088e7
Compare
- Update Spack repo API to v2.2 and fix deprecated filesystem imports in package recipes - Add upgrade_repos.py script to automate repo and recipe modernization - Bump GCC from 15.2 to 15.3 and LLVM to use gcc@13.3 as compiler - Remove parallel build flag (-p 1) from spack install commands - Add cetmodules and target=x86_64_v3 constraints to spack.yaml - Reorder Dockerfile config steps to run repo upgrades after packages.yaml and before compiler/external discovery
greenc-FNAL
force-pushed
the
feature/container-tweaks-with-standalone-cetmodules
branch
from
90088e7 to
e250a00
Compare
Improve maintainability and static analysis support by adding type annotations and documentation to the repository upgrade utility. - Add `from __future__ import annotations` for deferred type evaluation. - Add type hints to `REPLACEMENTS` and function signatures. - Add module-level and function-level docstrings. - Reformat long replacement strings for better readability.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ci/Dockerfile`:
- Around line 291-294: Drop the duplicated Spack view cleanup blocks in the
Dockerfile so each section only performs the view removal once. In the repeated
cleanup sections around the environment setup logic, keep the unique cleanup
tied to the relevant step and remove the extra copy of the same rm -rf on
$PHLEX_SPACK_ENV/.spack-env/view, preserving the existing view.old.* cleanup
where needed.
In `@ci/upgrade_repos.py`:
- Around line 32-33: Update the helper docstrings to Google-style formatting: in
upgrade_spack_repo_api and the other helper referenced in this change, add a
proper Args section documenting repo_obj and keep the one-line summary concise.
Make sure the docstrings follow standard Google-style conventions throughout the
helper functions in this module.
- Around line 47-51: The upgrade logic in upgrade_repos.py is too broad: the
current_api check in the repo API migration block rewrites any non-v2.2 value,
including newer versions, back to v2.2. Update the comparison in the upgrade
path to only change repos whose API version is older than v2.2, and leave v2.2
or newer versions untouched; use the current_api handling near the existing
print and data["repo"]["api"] assignment to locate the fix.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 01d448ea-7490-48d9-b01d-a9720fd543fd
📒 Files selected for processing (3)
ci/Dockerfileci/spack.yamlci/upgrade_repos.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
Framework-R-D/action-configure-cmake(auto-detected)Framework-R-D/action-workflow-setup(auto-detected)Framework-R-D/action-complete-pr-comment(auto-detected)Framework-R-D/action-handle-fix-commit(auto-detected)
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Analyze python with CodeQL
- GitHub Check: coverage
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.py: Use ruff for Python formatting and linting (configured inpyproject.toml); follow Google-style docstring conventions; use line length of 99 characters; use double quotes for strings
Usefrom __future__ import annotationsto enable deferred evaluation of type annotations (avoids forward-reference issues; Python >=3.12)
Type hints are recommended; mypy is configured inpyproject.toml
Avoid naming Python test scriptstypes.pyor other names that shadow standard library modules, as this causes obscure import errors
Python test scripts should follow the test structure pattern: C++ driver provides data streams, Jsonnet config wires the graph, and Python script implements algorithms
**/*.py: Enforce 99-character line limit and double quotes in Python via ruff configured inpyproject.toml
Use Google-style docstrings in Python code
Use type hints in Python code and configure mypy for type checking
Usefrom __future__ import annotationsin Python to enable deferred evaluation of type annotations
Use PEP 8 naming in Python:CapWordsfor classes,snake_casefor everything else
When using thereadtool for Python files, always use integer values foroffsetandlimitparameters, never float/double values
Files:
ci/upgrade_repos.py
🪛 ast-grep (0.44.0)
ci/upgrade_repos.py
[warning] 51-51: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(yaml_file, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 72-72: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(file_path, "r", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
[warning] 80-80: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(file_path, "w", encoding="utf-8")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 OpenGrep (1.23.0)
ci/upgrade_repos.py
[ERROR] 45-45: yaml.load() without SafeLoader can execute arbitrary Python code. Use yaml.safe_load() or yaml.load(..., Loader=SafeLoader) instead.
(coderabbit.deserialization.python-yaml-unsafe-load)
🔇 Additional comments (9)
ci/spack.yaml (3)
11-11: LGTM!
41-42: LGTM!
43-45: 🎯 Functional CorrectnessLLVM target pin looks fine
target=x86_64_v3onllvmis separate from the top-leveltargets=x86backend spec, and the GCC 15 pin is still present onphlexandroot.> Likely an incorrect or invalid review comment.ci/upgrade_repos.py (4)
55-56: Still swallows repo.yaml migration failures.Same previously flagged failure mode: the Docker build can keep going after a failed metadata rewrite.
28-28: Import rewrite still corrupts aliased imports.Same previously flagged regex issue: prefix replacement can turn aliased imports into invalid double-
asstatements.Also applies to: 76-78
1-14: LGTM!
90-106: LGTM!ci/Dockerfile (2)
125-128: LGTM!Also applies to: 154-178, 249-249
228-230: 🩺 Stability & AvailabilityKeep a bounded Spack package-parallelism cap.
These installs run on GitHub-hosted runners with limited memory; without an explicit-plimit, Spack can fan out more aggressively than the old-p 1setting. Keep the cap here or add a measured runner-memory check for the GCC/LLVM layers.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Addressed CodeRabbit review comments: ci/upgrade_repos.py
ci/Dockerfile
|
- Update the regex for `spack.llnl.util.filesystem` replacements to better preserve the import statement. - Improve docstrings for `upgrade_spack_repo_api` and `clean_package_imports` to follow Google style. - Change exception handling in `upgrade_spack_repo_api` to raise a `RuntimeError` instead of printing, preventing silent failures during API upgrades. - Clean up minor formatting in version parsing and logging.
|
@coderabbitai Please consider the report at #661 (comment) and recent commits, closing out review comments where appropriate. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== Unresolved review threads on PR 661 =="
gh api graphql -F owner='Framework-R-D' -F name='phlex' -F number=661 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
isOutdated
path
line
comments(first: 10) {
nodes {
id
author { login }
body
createdAt
url
}
}
}
}
}
}
}' --jq '
.data.repository.pullRequest.reviewThreads.nodes
| map(select(.isResolved == false))
| map({
id, path, line, isOutdated,
comments: (.comments.nodes | map({author: .author.login, body: .body, url}))
})
'
echo
echo "== Outline: ci/upgrade_repos.py =="
ast-grep outline ci/upgrade_repos.py --view expanded || true
echo
echo "== ci/upgrade_repos.py =="
sed -n '1,240p' ci/upgrade_repos.py
echo
echo "== Relevant sections of ci/Dockerfile =="
rg -n -C4 'upgrade_repos\.py|spack install|rm -rf .spack-env/view' ci/Dockerfile || trueLength of output: 18680 🏁 Script executed: #!/bin/bash
set -euo pipefail
threads=(
PRRT_kwDOOauAKM6L8kzB
PRRT_kwDOOauAKM6L8kzT
PRRT_kwDOOauAKM6MDnhz
PRRT_kwDOOauAKM6MDnh5
)
for thread in "${threads[@]}"; do
echo "Resolving $thread"
gh api graphql \
-F threadId="$thread" \
-f query='
mutation($threadId: ID!) {
resolveReviewThread(input: {threadId: $threadId}) {
thread {
id
isResolved
}
}
}' --jq '.data.resolveReviewThread.thread'
doneLength of output: 935
Closed out:
All four now show as resolved. |
1 participant
CI / container setup
ci/Dockerfileto modernize the Spack-based CI/dev build flow.numpyandlibffito the CI container image.cmakefrom the base “essential build tools” apt package list.-p 1) while keeping existing flags like--fail-fast.GNUPGHOME/SPACK_GNUPGHOMEhandling during Spack configuration generation.Build system / Spack workflow
ci/Dockerfile, reordered the Spack configuration steps to:upgrade_repos.pyscript,/tmp/upgrade_repos.pyvia Spack’s Python,SPACK_CONFIGURE_DEFAULTS.gcc@15.2togcc@15.3and aligned the discovery step to locategcc@15.Spack environment configuration
ci/spack.yaml:cetmodulesto the environmentspecs.cmakeandllvmbuilds totarget=x86_64_v3.llvm, replaced the prior explicit compiler constraint (%c,cxx=gcc@15) with thetarget=x86_64_v3requirement.Spack repository maintenance tooling
ci/upgrade_repos.py:builtinSpack repos by rewritingrepo.yamltorepo.api: v2.2when needed (defaulting missing API tov2.0).packages/forpackage.pyfiles and applies regex-based import rewrites (REPLACEMENTS), rewriting only when content changes.Code quality / security
actions/untrusted-checkoutandactions/untrusted-checkout-toctou(e.g., clang-tidy, coverage, header-guards, markdown/yaml fixes, clang-format/cmake-format/jsonnet-format, and python fix workflows).Ongoing maintenance