CaloTowerStatus: Prioritize Direct URL Overrides#4266
Merged
pinkenburg merged 3 commits intoMay 12, 2026
Conversation
Updated the logic in InitRun to ensure that URLs provided via set_directURL_hotMap and set_directURL_chi2 take precedence over the default CDBInterface lookups. Previously, the code checked the CDB first, which caused user-specified URLs to act only as fallbacks rather than overrides. This made it difficult to test local calibration files when a CDB map already existed for the current run. The logic now: 1. Checks for a user-specified direct URL. 2. Falls back to the CDBInterface if no direct URL is provided. 3. Aborts or disables the masking if neither is found.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughInitRun now prioritizes non-empty direct URL string overrides for chi2 and hot-map; if absent it queries CDBInterface and constructs trees only when a URL is returned, preserving existing exit-or-disable behavior. Setters no longer toggle boolean flags. ChangesCalibration initialization refactor
Possibly related PRs
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
offline/packages/CaloReco/CaloTowerStatus.cc (2)
84-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMemory leak on repeated InitRun calls.
Lines 89, 96, 126, and 133 allocate new CDBTTree objects and assign to
m_cdbttree_chi2andm_cdbttree_hotMapwithout first deleting any existing allocation. IfInitRunis called multiple times (e.g., processing multiple runs in a single job), the old pointers are overwritten and the previously allocated memory is leaked until theCaloTowerStatusobject is destroyed.🔧 Proposed fix
Delete existing pointers before allocating new ones:
std::string calibdir_chi2; if (use_directURL_chi2) { calibdir_chi2 = m_directURL_chi2; std::cout << "CaloTowerStatus::InitRun: Using direct URL override for chi2: " << calibdir_chi2 << std::endl; + delete m_cdbttree_chi2; + m_cdbttree_chi2 = nullptr; m_cdbttree_chi2 = new CDBTTree(calibdir_chi2); } else { calibdir_chi2 = CDBInterface::instance()->getUrl(m_calibName_chi2); if (!calibdir_chi2.empty()) { + delete m_cdbttree_chi2; + m_cdbttree_chi2 = nullptr; m_cdbttree_chi2 = new CDBTTree(calibdir_chi2);Apply the same fix for
m_cdbttree_hotMapat lines 126 and 133.
84-152:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCDBTTree construction not protected by exception handling.
The
CDBTTreeconstructor at lines 89, 96, 126, and 133 can throw exceptions (e.g., file not found, invalid path, access denied), but these allocations occur before the try-catch block at lines 170-179. If construction fails—particularly when a user provides a malformed direct URL—the exception propagates uncaught, potentially crashing the framework ungracefully.🛡️ Proposed fix
Wrap the CDBTTree constructions in the existing try-catch block or add local error handling:
+ try + { std::string calibdir_chi2; if (use_directURL_chi2) { calibdir_chi2 = m_directURL_chi2; std::cout << "CaloTowerStatus::InitRun: Using direct URL override for chi2: " << calibdir_chi2 << std::endl; m_cdbttree_chi2 = new CDBTTree(calibdir_chi2); } // ... rest of chi2 and hotMap setup ... + } + catch (std::exception &e) + { + std::cout << "CaloTowerStatus::InitRun: Failed to load calibration: " << e.what() << std::endl; + return Fun4AllReturnCodes::ABORTRUN; + }Alternatively, move all calibration setup into the existing try-catch block starting at line 170.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1a4b2489-986b-41f0-bd4a-86e5bb99fc79
📒 Files selected for processing (1)
offline/packages/CaloReco/CaloTowerStatus.cc
Contributor
There was a problem hiding this comment.
Pull request overview
This PR updates CaloTowerStatus::InitRun so that user-specified calibration URLs (via set_directURL_hotMap / set_directURL_chi2) are treated as true overrides, with CDB lookups used only as a fallback.
Changes:
- Reordered chi2 calibration loading to use
m_directURL_chi2first when provided, otherwise queryCDBInterface. - Reordered hot map calibration loading to use
m_directURL_hotMapfirst when provided, otherwise queryCDBInterface. - Kept existing abort/disable behavior when no calibration source is available.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+122
to
+126
| if (use_directURL_hotMap) | ||
| { | ||
| m_cdbttree_hotMap = new CDBTTree(calibdir); | ||
| if (Verbosity() > 1) | ||
| { | ||
| std::cout << "CaloTowerStatus::Init " << m_detector << " hot map found " << m_calibName_hotMap << " Ddoing isHot" << std::endl; | ||
| } | ||
| calibdir_hotMap = m_directURL_hotMap; | ||
| std::cout << "CaloTowerStatus::InitRun: Using direct URL override for hot map: " << calibdir_hotMap << std::endl; | ||
| m_cdbttree_hotMap = new CDBTTree(calibdir_hotMap); |
"Ddoing" - > "Doing" Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Build & test reportReport for commit ecb697166f7676109c18a5efacd163e015409ed2:
Automatically generated by sPHENIX Jenkins continuous integration |
Build & test reportReport for commit b3467b76fb6610d0a571711955b2c56be083b27d:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
|
The proliferation of flags is really concerning. Here an approach of setting the filename and only call the cdb if that filename is empty is more appropriate. That will simplify the code a lot and prevent bad usages where the flag is set to true but the filename is not set. |
- Removed 'use_directURL_hotMap' and 'use_directURL_chi2' boolean flags. - Updated InitRun to check if URL strings are non-empty to determine if a direct override should be used. - Ensured that a non-empty direct URL string takes precedence over the default CDBInterface lookup. This change prevents potential bugs where a flag could be set to true without a valid filename, and streamlines the code by relying on the string's presence as the single source of truth for the override.
Build & test reportReport for commit ad25fc22e314edbeb0dbdaf01d7d2b9f0b6ee135:
Automatically generated by sPHENIX Jenkins continuous integration |
Contributor
Author
@pinkenburg , thank you for the feedback. I have simplified the module by removing the additional boolean flags. If you could confirm it looks good on your end that would be great. |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Types of changes
What kind of change does this PR introduce? (Bug fix, feature, ...)
Updated the logic in
InitRunto ensure that URLs provided viaset_directURL_hotMapandset_directURL_chi2take precedence over the defaultCDBInterfacelookups. Previously, the code checked the CDB first, which caused user-specified URLs to act only as fallbacks rather than overrides.The logic now:
CDBInterfaceif no direct URL is provided.TODOs (if applicable)
Links to other PRs in macros and calibration repositories (if applicable)
CaloTowerStatus: Prioritize Direct URL Overrides
Motivation
CaloTowerStatus previously queried the CDB first and treated user-provided direct URLs as fallbacks. That made it awkward to run with local calibration files when a CDB entry existed. This PR fixes the lookup order so explicit direct URLs reliably override CDB entries, enabling straightforward local testing and intended override behavior.
Key Changes
Potential Risk Areas
Possible Future Improvements
Notes