ci: Add LCG 106a job #3918
ci: Add LCG 106a job #3918
Conversation
WalkthroughNew job configuration, the pull request introduces. Named Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (
|
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.gitlab-ci.yml(1 hunks)
🔇 Additional comments (2)
.gitlab-ci.yml (2)
434-434: Hmmmm, proper spacing between jobs, there is.
Maintain readability with consistent spacing between job definitions, you do. Good practice, this is.
435-447: Dependencies on this job, verify we must.
Consider if downstream jobs depend on LCG test results, you should. Ensure proper job ordering in pipeline, we must.
✅ Verification successful
Let me search deeper into the Force, I shall. Understanding the job dependencies and hierarchy, crucial it is.
Clear, the path forward is. No downstream dependencies exist.
Through the Force, seen I have that this LCG job stands independent, hmm. No direct dependencies or needs upon lcg_106a do other jobs have. Extends the base configuration .lcg_base_job it does, following the way of its siblings. The only needs found in pipeline relate to build_linux_ubuntu and build_exatrkx, separate paths they follow.
Safe to proceed with these changes, it is. Disturb the Force's balance, it will not.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for downstream job dependencies
# Look for needs/dependencies referencing LCG jobs
echo "Searching for downstream dependencies..."
rg "needs.*lcg_" .gitlab-ci.yml
Length of output: 124
Script:
#!/bin/bash
# Search for any job that references lcg_106a
echo "Searching for direct references to lcg_106a..."
rg "lcg_106a" .gitlab-ci.yml -A 5 -B 5
# Search for dependencies pattern more broadly
echo -e "\nSearching for broader dependency patterns..."
rg "needs:|dependencies:" .gitlab-ci.yml -A 3
# Check the extends pattern to understand job hierarchy
echo -e "\nChecking job inheritance..."
rg "extends:.*lcg" .gitlab-ci.yml -A 2
Length of output: 994
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.gitlab-ci.yml (1)
434-436: Consider version pinning strategy, we should!For stability and reproducibility, a clear strategy for LCG version management, essential it is. Document the criteria for version selection and update frequency, you must.
Suggestions, these are:
- Document LCG version selection criteria in repository
- Establish update frequency guidelines
- Create process for compiler compatibility verification
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
.gitlab-ci.yml(2 hunks)
🔇 Additional comments (2)
.gitlab-ci.yml (2)
385-385: Approve this change, I do!
Simplified the platform logic is, yet maintains its purpose it does. Clear and direct, this approach is.
431-443:
Remove gcc14 from matrix, you must!
Concerned about gcc14 in LCG 106a configuration, I am. Documentation and evidence of its compatibility, exist they do not. A path to build failures, this may be.
Apply this change, you should:
parallel:
matrix:
- OS: [alma9]
COMPILER:
- gcc13
- - gcc14
- clang16Proceed with gcc13 and clang16, safe it is. When gcc14 compatibility confirmed it is, add it back you may.
github-actions
bot
added
Component - Examples
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (8)
Examples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cpp (1)
58-58: Strong with the Force, this type change is!Aligned with the compatibility changes, this modification is. Consistent with the broader measurement handling updates across the codebase, it remains.
Consider documenting the version compatibility requirements in the README or documentation, you should. Help future maintainers understand the version constraints, this will.
Examples/Io/EDM4hep/src/EDM4hepMeasurementReader.cpp (1)
73-73: Consistent with the new ways of the Force, this change is!Aligned with the TrackerHit3D migration, this type change is. Yet, consider documenting the collection name convention in the code, we should.
Consider adding a constant or enum for collection names, improve maintainability it would. Example, this is:
namespace ActsExamples { namespace EDM4hepCollections { constexpr char TrackerHitsRaw[] = "ActsTrackerHitsRaw"; constexpr char TrackerHitsPlane[] = "ActsTrackerHitsPlane"; } // namespace EDM4hepCollections } // namespace ActsExamplesExamples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepUtil.hpp (1)
27-37: Wise handling of compatibility, this is!Gracefully detect and adapt to the presence of TrackerHit3D, you do. Through type aliases, backward compatibility maintained is. Strong with the Force, this approach is!
Consider documenting the version requirements where TrackerHit3D became available, helpful for future Padawans, it would be.
Plugins/EDM4hep/src/EDM4hepUtil.cpp (1)
192-211: Clever version handling, you have implemented!Maintain backward compatibility while preparing for the future, this code does. Yet, documentation for these functions, missing it is.
Add documentation like this, you should:
+/// Retrieves the MCParticle from a SimTrackerHit +/// @param hit The SimTrackerHit to get the particle from +/// @return The associated MCParticle edm4hep::MCParticle getParticle(const edm4hep::SimTrackerHit& hit) { +/// Sets the MCParticle for a MutableSimTrackerHit +/// @param hit The MutableSimTrackerHit to set the particle for +/// @param particle The MCParticle to associate with the hit void setParticle(edm4hep::MutableSimTrackerHit& hit,Plugins/EDM4hep/include/Acts/Plugins/EDM4hep/EDM4hepUtil.hpp (1)
66-70: Wise addition of version compatibility functions, this is.Handle different versions of EDM4hep gracefully, these functions will. Clear and concise, their signatures are. The Force is strong with this implementation.
However, document the version-specific behavior in more detail, we should.
Add documentation like this, you should:
// Compatibility with EDM4hep < 0.99 and >= 0.99 +// For EDM4hep < 0.99: Uses MCParticle ID +// For EDM4hep >= 0.99: Uses direct MCParticle reference edm4hep::MCParticle getParticle(const edm4hep::SimTrackerHit& hit); void setParticle(edm4hep::MutableSimTrackerHit& hit, const edm4hep::MCParticle& particle);Examples/Io/EDM4hep/src/EDM4hepUtil.cpp (2)
175-175: Track this TODO, we must!Missing cell information accessibility, a gap in our knowledge it is. Track this requirement formally, we should.
Create a GitHub issue to track the cell information accessibility requirement, shall I?
211-211: Related to the previous TODO, this is!Write operation faces the same challenge with cell information. Together with the read operation's task, track this we should.
Include this write operation requirement in the same GitHub issue, shall I?
Examples/Io/EDM4hep/src/EDM4hepReader.cpp (1)
309-311: Enhanced debugging capabilities through improved logging, I sense!Clearer visibility into particle source information, this change brings. Help developers track the Force through their code, it will.
Consider wrapping verbose logging in a debug scope to avoid string construction overhead when debug logging is disabled:
- ACTS_VERBOSE( - "SimHit has source particle: " - << Acts::EDM4hepUtil::getParticle(hit).getObjectID().index); + if (logger().doPrint(Acts::Logging::VERBOSE)) { + ACTS_VERBOSE( + "SimHit has source particle: " + << Acts::EDM4hepUtil::getParticle(hit).getObjectID().index); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepMeasurementWriter.hpp(0 hunks)Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepUtil.hpp(3 hunks)Examples/Io/EDM4hep/src/EDM4hepMeasurementReader.cpp(2 hunks)Examples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cpp(2 hunks)Examples/Io/EDM4hep/src/EDM4hepReader.cpp(2 hunks)Examples/Io/EDM4hep/src/EDM4hepUtil.cpp(6 hunks)Plugins/EDM4hep/include/Acts/Plugins/EDM4hep/EDM4hepUtil.hpp(2 hunks)Plugins/EDM4hep/src/EDM4hepUtil.cpp(2 hunks)thirdparty/OpenDataDetector(1 hunks)
💤 Files with no reviewable changes (1)
- Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepMeasurementWriter.hpp
✅ Files skipped from review due to trivial changes (1)
- thirdparty/OpenDataDetector
🔇 Additional comments (12)
Examples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cpp (1)
21-29: Hmmmm, wise handling of versions, this is!
Clean and standard-compliant, the compatibility approach is. Through preprocessor checks and type aliases, backward compatibility maintained, it is.
Verify the version handling across the codebase, we must:
✅ Verification successful
Consistent across the codebase, the version handling is!
Through the Force, searched the codebase I have. Found the same compatibility pattern in multiple files, I did:
EDM4hepMeasurementReader.cpp: Same type alias it usesEDM4hepUtil.hpp: The pattern repeats, maintaining harmonyEDM4hepMeasurementWriter.cpp: Original implementation, correct it is
In balance, the code is. No disturbance in the Force, I sense. The version compatibility approach, consistently applied throughout the EDM4hep integration files, it is.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other potential EDM4hep version-specific code
rg "TrackerHit(3D)?Collection" --type cpp
# Check if the compatibility pattern is consistently applied
ast-grep --pattern 'using TrackerHit3DCollection = $_'
Length of output: 2535
Examples/Io/EDM4hep/src/EDM4hepMeasurementReader.cpp (1)
21-31: Wise approach to backward compatibility, this is!
Handle different EDM4hep versions gracefully, this conditional compilation does. Through the Force, I sense good practices in the type aliases used.
Verify the compatibility across EDM4hep versions, we must:
✅ Verification successful
Consistent across the codebase, the Force is!
Hmmmm, revealed by our investigation, it has, that similar conditional compilation exists in other files, yes. The same approach, they follow! In EDM4hepMeasurementWriter.cpp and EDM4hepUtil.hpp, the same __has_include pattern we find. Backward compatibility, they all maintain.
Three files share this wisdom, they do:
- Examples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cpp
- Examples/Io/EDM4hep/src/EDM4hepMeasurementReader.cpp
- Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepUtil.hpp
Consistent in their implementation, they are. Handle both old and new EDM4hep versions gracefully, they do. Strong with the Force, this approach is!
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check EDM4hep version compatibility
# Search for any other TrackerHit type usage
rg -l "TrackerHit[^3]" --type cpp
# Check if any other files need similar conditional compilation
rg -l "edm4hep.*TrackerHit" --type cpp
Length of output: 1071
Script:
#!/bin/bash
# Check the actual usage of TrackerHit types in these files
rg "TrackerHit" -B 2 -A 2 Examples/Io/EDM4hep/src/EDM4hepMeasurementWriter.cpp Examples/Io/EDM4hep/src/EDM4hepReader.cpp
# Check if similar conditional compilation exists in other files
rg "__has_include.*TrackerHit" --type cpp
# Check for any type conversions or direct usage of TrackerHit types
ast-grep --pattern 'TrackerHit$_'
Length of output: 3381
Examples/Io/EDM4hep/include/ActsExamples/Io/EDM4hep/EDM4hepUtil.hpp (2)
19-25: Organized well, these includes are. Approve them, I do!
A consistent pattern with angle brackets, they follow. Clear and orderly, the path to the Force is.
104-104: Verify the necessity of unused parameters, we must.
Parameters fromClusters and toCluster in readMeasurement, unused they appear to be. Yet in writeMeasurement, toClusters actively used it is.
Run this command to investigate the usage patterns:
Also applies to: 120-120
✅ Verification successful
Unused parameters, justified they are. Remove them, we must not!
In EDM4hepUtil.cpp, commented out but present the parameters are, serving as interface placeholders they do. A bridge between different measurement formats, this code is. In EDM4hepMeasurementReader.cpp, called the function is, with all parameters provided.
Future compatibility and interface consistency, these parameters maintain. Remove them, we should not, as breaking changes to the API, such action would cause.
Points of evidence, these are:
- In EDM4hepUtil.cpp, parameters marked as unused with comments they are:
/*fromClusters*/and/*toCluster*/ - In EDM4hepMeasurementReader.cpp, all parameters actively passed they are
- Interface symmetry with
writeMeasurementfunction, maintained it is
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for implementations and usages of these measurement functions
ast-grep --pattern 'readMeasurement($$$)'
ast-grep --pattern 'writeMeasurement($$$)'
Length of output: 88
Script:
#!/bin/bash
# Search for files containing measurement functions with ripgrep for better context
rg -l "readMeasurement|writeMeasurement" --type cpp
# Then examine the implementation and usage
rg "readMeasurement|writeMeasurement" --type cpp -A 5 -B 5
Length of output: 11574
Plugins/EDM4hep/src/EDM4hepUtil.cpp (2)
21-24: Wise addition of headers, this is!
Properly organized and necessary for the new particle handling functionality, these headers are. Clear intent and good practices, they show.
192-193: Verify version handling consistency across the codebase, we must!
Check for other EDM4hep version-dependent code that might need similar treatment, we should.
✅ Verification successful
Consistent, the version handling is, hmmmm
Through the Force, searched the codebase I have. Only one instance of EDM4HEP version check exists, in EDM4hepUtil.cpp it does. No other files require similar treatment, clear this is.
The getMCParticle and setMCParticle usage, also contained within EDM4hepUtil.cpp they are. Isolated, these changes remain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other EDM4hep version checks
rg -A 2 "EDM4HEP_VERSION" --type cpp
# Search for other uses of getMCParticle/setMCParticle
rg "(get|set)MCParticle" --type cpp
Length of output: 548
Plugins/EDM4hep/include/Acts/Plugins/EDM4hep/EDM4hepUtil.hpp (2)
29-32: Approve the new header inclusions, I do.
Properly organized, these new headers are. A clear purpose they serve, enhancing the EDM4hep integration capabilities, they do.
66-70: Verify the version compatibility implementation, we must.
Ensure that both versions of EDM4hep handle these functions correctly, we should.
✅ Verification successful
Properly handled, the version compatibility is, young Padawan.
Clear implementation in EDM4hepUtil.cpp, I see:
- Version check through preprocessor directives, properly done it is:
#if EDM4HEP_VERSION_MAJOR >= 1 || \
(EDM4HEP_VERSION_MAJOR == 0 && EDM4HEP_VERSION_MINOR == 99)
// Uses getParticle() and setParticle()
#else
// Uses getMCParticle() and setMCParticle()
#endif- For versions >= 0.99,
getParticle()andsetParticle()methods used they are - For older versions,
getMCParticle()andsetMCParticle()methods used they are - Header inclusion of version information present it is:
#include <edm4hep/EDM4hepVersion.h>
Proper backward compatibility maintained it is. No issues found in the Force.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for version-specific implementations and tests
echo "Searching for version-specific implementations..."
rg -A 5 "EDM4hep.*0\.99" --type cpp
echo "Searching for related test cases..."
fd -e cpp -e hpp test.*EDM4hep --exec rg -l "getParticle|setParticle" {}
Length of output: 1435
Script:
#!/bin/bash
# Search for implementation of these functions
echo "Implementation of getParticle and setParticle, we seek..."
ast-grep --pattern 'edm4hep::MCParticle getParticle(const edm4hep::SimTrackerHit& hit) {
$$$
}'
ast-grep --pattern 'void setParticle(edm4hep::MutableSimTrackerHit& hit, const edm4hep::MCParticle& particle) {
$$$
}'
echo "Version-specific header includes, we must find..."
rg -A 2 "#if.*has_include.*edm4hep" --type cpp
echo "Implementation files for EDM4hepUtil, search for we shall..."
fd EDM4hepUtil.cpp
Length of output: 1609
Script:
#!/bin/bash
# Implementation in EDM4hepUtil.cpp files, examine we must
echo "Content of Plugins/EDM4hep/src/EDM4hepUtil.cpp, reveal we shall..."
cat Plugins/EDM4hep/src/EDM4hepUtil.cpp
echo "\nContent of Examples/Io/EDM4hep/src/EDM4hepUtil.cpp, examine we must..."
cat Examples/Io/EDM4hep/src/EDM4hepUtil.cpp
echo "\nVersion handling in build system, search we must..."
fd -e cmake -e CMakeLists.txt . --exec rg -l "EDM4hep.*0\.99|EDM4hep.*VERSION" {}
Length of output: 19248
Examples/Io/EDM4hep/src/EDM4hepUtil.cpp (3)
73-76: Wise changes in particle handling, these are!
Improved abstraction through helper function Acts::EDM4hepUtil::getParticle, I see. Better compatibility and maintainability, this brings. Hmmmm.
120-120: Symmetry in the Force, I sense!
Matching the read operation's pattern with Acts::EDM4hepUtil::setParticle, you do. Balance between read and write operations, this brings.
269-280: Elegant handling of the covariance matrix, this is!
Lambda function for cleaner access and improved formatting for better readability, you have achieved. Strong with the Force, this code organization is!
Examples/Io/EDM4hep/src/EDM4hepReader.cpp (1)
13-13: Wise addition of EDM4hep utility header, this is!
Harmony with the Force, this header brings, as utility functions for EDM4hep processing, it provides.
paulgessinger
force-pushed
the
add_lcg_106
branch
from
cfa5829 to
e53e084
Compare
github-actions
bot
removed
Component - Examples
|
This adds jobs for LCG 106a to the GitLab CI.
I'm bumping ODD to the
v4.0.3where I cherry picked a tiny code fix that was breaking the build here.Blocked by:
0.99& >=0.99#3921Summary by CodeRabbit
lcg_106afor enhanced build configurations.lcg_105job by directly settingLCG_PLATFORMfor thealma9OS.OpenDataDetectorsubproject to the latest version.