Update CI container image to support Numba and libffi#657
Conversation
knoepfel
requested review from
aolivier23,
greenc-FNAL,
marcpaterno and
sabasehrish
as code owners
|
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:
📝 WalkthroughWalkthroughThe ChangesCI Image Rebuild and Workflow Pin
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
31 fixed, 0 new since branch point (a98ac3d) ✅ 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: 2
🤖 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 @.github/workflows/clang-tidy-check.yaml:
- Line 56: Replace the mutable image tag with a specific digest hash across all
five workflow files. For the ghcr.io/framework-r-d/phlex-ci image reference,
replace the `:2026-06-22` tag with the full image digest in the format
`@sha256:HASH` to ensure reproducibility and prevent silent image changes. Apply
this change in clang-tidy-check.yaml line 56, clang-tidy-fix.yaml,
cmake-build.yaml, coverage.yaml, and codeql-analysis.yaml wherever the same
image is referenced.
In `@phlex/app/run.cpp`:
- Around line 8-9: Remove the temporary comment "// Temporary comment to trigger
CI build" from lines 8-9 in the run.cpp file. This hardcoded comment was used to
force a CI rebuild and should not be shipped in the final code. Simply delete
these two lines before merging the pull request.
🪄 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: d4e399e5-fbef-47be-b01c-56e738b3b180
📒 Files selected for processing (8)
.github/workflows/clang-tidy-check.yaml.github/workflows/clang-tidy-fix.yaml.github/workflows/cmake-build.yaml.github/workflows/codeql-analysis.yaml.github/workflows/coverage.yamlci/Dockerfileci/packages.yamlphlex/app/run.cpp
🔗 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. (5)
- GitHub Check: Analyze cpp with CodeQL
- GitHub Check: Analyze actions with CodeQL
- GitHub Check: build (gcc, none)
- GitHub Check: coverage
- GitHub Check: clang-tidy-check
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{cpp,cc,cxx,h,hpp}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{cpp,cc,cxx,h,hpp}: Use clang-format tool for all C++ code formatting (VS Code auto-formats on save); configuration defined in.clang-formatwith 100-character line limit and 2-space indentation
Follow clang-tidy recommendations defined in.clang-tidy
Files:
phlex/app/run.cpp
**/*.{hpp,cpp}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{hpp,cpp}: Use.hppfor header files,.cppfor implementation, and*_test.cppfor test files in C++
Enforce 100-character line limit and 2-space indentation in C++ code via.clang-format
UseQualifierAlignment: Right(east-const) style:int const xnotconst int xin C++
UsePointerAlignment: Leftin C++ (pointer*attached to type, not variable name)
All C++ identifiers must uselower_casenaming: namespaces, classes, structs, enums, functions, variables, parameters, members, and constants
Exception to C++ naming: template parameters useCamelCase
Exception to C++ naming: macros useUPPER_CASE
Private, protected, and constant members in C++ must have a trailing underscore (_), no trailing underscore on anything else
Useenum classpreferred over plainenumin C++
Usestd::shared_ptrfor shared ownership,std::unique_ptrfor exclusive ownership, raw pointers for non-owning references only in C++
Use functors with agent-noun pattern:ModelEvaluator evaluate_model(...)in C++
Apply.clang-tidychecks for bugprone, cert, clang-analyzer, concurrency, cppcoreguidelines, misc, modernize, performance, portability, and readability as defined in the.clang-tidyconfiguration file
Usephlex::namespace for core code,phlex::experimental::for experimental features in C++
Files:
phlex/app/run.cpp
🪛 zizmor (1.26.1)
.github/workflows/cmake-build.yaml
[error] 155-155: unpinned image references (unpinned-images): container image is not pinned to a SHA256 hash
(unpinned-images)
.github/workflows/clang-tidy-fix.yaml
[error] 76-76: unpinned image references (unpinned-images): container image is not pinned to a SHA256 hash
(unpinned-images)
.github/workflows/clang-tidy-check.yaml
[error] 56-56: unpinned image references (unpinned-images): container image is not pinned to a SHA256 hash
(unpinned-images)
.github/workflows/coverage.yaml
[error] 78-78: unpinned image references (unpinned-images): container image is not pinned to a SHA256 hash
(unpinned-images)
.github/workflows/codeql-analysis.yaml
[error] 215-215: unpinned image references (unpinned-images): container image is not pinned to a SHA256 hash
(unpinned-images)
🔇 Additional comments (6)
.github/workflows/clang-tidy-fix.yaml (1)
76-76: Same mutable-tag container issue as noted in.github/workflows/clang-tidy-check.yaml(pin this reference to the same@sha256:digest)..github/workflows/cmake-build.yaml (1)
155-155: Same mutable-tag container issue as noted in.github/workflows/clang-tidy-check.yaml(pin this reference to the same@sha256:digest)..github/workflows/codeql-analysis.yaml (1)
215-215: Same mutable-tag container issue as noted in.github/workflows/clang-tidy-check.yaml(pin this reference to the same@sha256:digest)..github/workflows/coverage.yaml (1)
78-78: Same mutable-tag container issue as noted in.github/workflows/clang-tidy-check.yaml(pin this reference to the same@sha256:digest).ci/Dockerfile (1)
216-219: LGTM!Also applies to: 274-279, 328-333, 360-365, 393-398
ci/packages.yaml (1)
19-20: LGTM!
knoepfel
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #657 +/- ##
==========================================
+ Coverage 83.27% 83.53% +0.26%
==========================================
Files 162 170 +8
Lines 5912 6613 +701
Branches 670 798 +128
==========================================
+ Hits 4923 5524 +601
- Misses 796 820 +24
- Partials 193 269 +76
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 23 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
|
A couple of questions:
Is there a reason for these?
@knoepfel Is there a reason you removed this line? It means that binary packages built for the CI are no good for a general-purpose cache. |
Both variants, when specified, resulted in a concretization failure for
That line triggered an error that essentially said "change the padding length" (my paraphrase). I no longer have the exact message available. Since time is of the essence, I have not debugged the reason behind these failures. I chose the path of least resistance so that we can test Wim's Numba PR in time for 0.3. |
knoepfel
force-pushed
the
update-ci-numpy
branch
from
0000f13 to
3ae4979
Compare
|
More specific explanation:
The current # Graphite loop optimizations cause bootstrap comparison failures
conflicts("+graphite +bootstrap") and variant(
"profiled", default=False, description="Use Profile Guided Optimization", when="+bootstrap"
) So, we have a few options:
But the current specification of |
knoepfel
force-pushed
the
update-ci-numpy
branch
from
3ae4979 to
91fdeb6
Compare
knoepfel
force-pushed
the
update-ci-numpy
branch
from
91fdeb6 to
521ecd4
Compare
This works around an issue with using std::tuple types in CHECK statements
knoepfel
force-pushed
the
update-ci-numpy
branch
from
521ecd4 to
563cf8f
Compare
greenc-FNAL
left a comment
greenc-FNAL
left a comment
There was a problem hiding this comment.
I'm assuming the C++ changes were necessary/enabled by the LLVM change?
Yes |
Clang-Tidy Check ResultsFound 5469 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
Clang-Tidy Check ResultsFound 5469 issue(s); none are newly introduced by this patch. All issues by check:
See inline comments for details. Comment |
2 participants
Build System
+bootstrapand+profiledvariants while retaining~nvptxand+strip$PHLEX_SPACK_ENV/.spack-env/viewandview.old.*) before subsequent installs to prevent cross-layer environment view rename issuesPackage Configuration
targetlist format (target: - x86_64_v3) torequiredirective format (require: - target=x86_64_v3)py-numpyconstraintCode changes to accommodate LLVM 20 limitations
boost::pfr::getusage with wordier implementation when creating registrarsstd::tuple-based check with checks of each tuple elementstd::tuple-based check with checks of each tuple element