remove run and subrun accessors

[rlcee]

The database conditions and proditions access are lazy-loaded for efficiency. Accessing it in beginRun risks caching a result which has subrun dependence. Accessing it in beginSubrun risks unnecessary loads for skimmed files with begin subruns with no associated events. The solution was to access in event methods only - if there is no change in conditions, there is just an iov check and return. This PR realizes the long-standing goal of emphasizing this by the interface. As you can see in the module, this rule can be defeated, which is OK enough for a debugging print. I surveyed Offline and I don't think this happens anywhere else.

[FNALbuild]

Hi @rlcee,
You have proposed changes to files in these packages:

• ProditionsService
• TrackerMC

which require these tests: build.

@Mu2e/write, @Mu2e/fnalbuild-users have access to CI actions on main.

⌛ The following tests have been triggered for 69b52bd: build (Build queue - API unavailable)

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 69b52bd.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 69b52bd at b188c42
build (prof)	✅	Log file. Build time: 04 min 07 sec
ceSimReco	✅	Log file.
g4test_03MT	✅	Log file.
transportOnly	✅	Log file.
POT	✅	Log file.
g4study	✅	Log file.
cosmicSimReco	✅	Log file.
cosmicOffSpill	✅	Log file.
ceSteps	✅	Log file.
ceDigi	✅	Log file.
muDauSteps	✅	Log file.
ceMix	✅	Log file.
rootOverlaps	✅	Log file.
g4surfaceCheck	✅	Log file.
trigger	❓	Log file. Return Code 1.
check_cmake	✅	Log file.
FIXME, TODO	➡️	TODO (0) FIXME (7) in 2 files
clang-tidy	➡️	2 errors 214 warnings
whitespace check	✅	no whitespace errors found

N.B. These results were obtained from a build of this Pull Request at 69b52bd after being merged into the base branch at b188c42.

For more information, please check the job page here.
Build artifacts are deleted after 5 days. If this is not desired, select Keep this build forever on the job page.

[oksuzian]

I'll review this pull request by examining its details and changes.Now let me look at the full files to understand the broader context:Now let me also check if there are any other callers of the removed get(RunID) or get(SubRunID) methods across the codebase.Now I have a comprehensive understanding of the PR. Let me also check if there are other callers using the RunID or SubRunID overloads across the repo.Now I have a thorough understanding of the PR. Let me also check whether there are other callers of the removed RunID/SubRunID overloads across the codebase.Now I have a thorough understanding of the PR. Here's my review:

Review of PR #1770 — "remove run and subrun accessors"

Summary

This PR makes two changes across 2 files (+2 lines, -7 lines):

1. ProditionsService/inc/ProditionsHandle.hh — Removes two convenience overloads of get():

   • get(art::RunID const&) which forwarded to get(art::EventID(rid.run(), 0, 0))
   • get(art::SubRunID const&) which forwarded to get(art::EventID(sid, 0))

2. TrackerMC/src/StrawDigisFromStrawGasSteps_module.cc — Updates the only call site that used the removed RunID overload, in beginRun(), to manually construct an art::EventID instead:

   // Before:
   auto const& strawphys = _strawphys_h.get(run.id());
   // After:
   auto const& strawphys = _strawphys_h.get(art::EventID(run.run(),0,0));

---

Issues and Observations

1. ⚠️ The workaround in beginRun() contradicts the purpose of the PR (Minor concern)

The PR description explains that the goal is to prevent calling ProditionsHandle::get() from beginRun or beginSubrun, because lazy-loaded conditions may cache results with incorrect subrun dependence. However, the replacement code in StrawDigisFromStrawGasSteps_module.cc line 450 still calls _strawphys_h.get(art::EventID(run.run(),0,0)) from beginRun() — it just does so by manually constructing a fake EventID.

This effectively replicates the exact same behavior as the deleted get(art::RunID const&) overload. The comment says "nominal subrun for debugging, calls in produce() give formal result" and the PR description acknowledges "this rule can be defeated, which is OK enough for a debugging print." This is acceptable but worth noting: the API removal is a soft guard, not a hard one. Any caller can still construct a synthetic art::EventID(run, 0, 0) and call get() from anywhere.

Suggestion: If you want a stronger guarantee, consider adding a [[deprecated]] or static_assert-style mechanism, or at minimum add a comment in ProditionsHandle.hh near get(art::EventID const&) explaining that it should only be called from event-level methods.

2. ⚠️ Potential downstream breakage for out-of-tree callers (Important)

The removed get(art::RunID const&) and get(art::SubRunID const&) overloads are part of a public template class header (ProditionsHandle.hh). Any out-of-tree code (other Mu2e packages, user analysis modules, etc.) that calls _handle.get(run.id()) or _handle.get(subrun.id()) will fail to compile after this change.

The PR author says "I surveyed Offline and I don't think this happens anywhere else," which covers this repository. However:

• Are there other Mu2e repositories or user code bases that include this header and use these overloads?
• If so, this is a breaking API change and should be communicated/coordinated.

3. ✅ The get(art::EventID const&) overload remains — correctness is preserved

The core get(art::EventID const&) method is unchanged. The removed overloads were trivial wrappers that constructed EventID with hardcoded subrun/event numbers of 0. The replacement in the module does the same thing explicitly. No functional change in behavior.

4. 🔍 Comment accuracy in the module change

// nominal subrun for debugging, calls in produce() give formal result

The comment says "nominal subrun" but the code is constructing a nominal event (art::EventID(run.run(), 0, 0)) where both the subrun and event number are 0. This is fine but could be slightly more precise:

// nominal EventID (subrun=0, event=0) for debugging; formal access in produce()

5. ✅ Build status looks good

The PR has the label build finished, confirming the change compiles cleanly within the Offline repository.

---

Verdict

This is a clean, well-motivated, low-risk PR. The change correctly enforces the design principle that ProditionsHandle conditions should only be formally accessed at the event level. The one remaining call from beginRun() is acknowledged as a debug-only workaround and is appropriately guarded by _printLevel > 0.

Main concern: Ensure there are no out-of-tree callers of the removed get(RunID) / get(SubRunID) overloads before merging.

[rlcee]

I don't think any changes need to be made based on the copilot review