Keep EventHeader on output

[michaelmackenzie]

No description provided.

[michaelmackenzie]

I checked it is now in the output:

mu2e -c Offline/Print/fcl/dumpDataProducts.fcl -s dig.owner.desc.version.sequencer.art | grep EventHeader
                                  mu2e::EventHeader                       ProcessCFOData                       Digitize  1259192821

[oksuzian]

Now let me check if EventHeader is produced somewhere to understand cross-repo implications and whether the placement in TriggerProducts is semantically correct.Now let me check whether EventHeader is currently produced in the GeneralProducts list, or if it should alternatively live there instead of TriggerProducts:Now I have all the information I need to produce a thorough review. Let me compile my findings.

---

PR Review Summary

Decision

• Comment only (leaning approve)

Scope understood

• This PR adds "keep mu2e::EventHeader_*_*_*" to the Digitize.TriggerProducts output commands list in JobConfig/digitize/prolog.fcl, so that mu2e::EventHeader data products are retained on digitization output files.
• Single file, single line addition. Minimal and focused.
• The author has confirmed validation: the EventHeader from ProcessCFOData now appears in the output of dumpDataProducts.

Findings

1. [S3] Nit — Placement choice: TriggerProducts vs GeneralProducts

   • Evidence: The EventHeader is produced by ProcessCFOData (the EventHeaderFromCFOFragment module in Offline/DAQ). Looking at the existing output product lists, GeneralProducts (line 192–195) already contains similar per-event bookkeeping products (EventWindowMarker, ProtonBunchTime). TriggerProducts is conceptually for trigger-specific outputs (trigger results, helix seeds, KalSeeds from triggers, etc.).
   • Impact: Placing EventHeader in TriggerProducts means it propagates everywhere TriggerProducts is referenced (including StripMC.fcl, reco/prolog.fcl's GeneralProducts which includes TriggerProducts, etc.), so functionality is correct. However, semantically EventHeader is general event-level metadata, not a trigger product.
   • Suggested fix: Consider moving the line to GeneralProducts instead, alongside EventWindowMarker and ProtonBunchTime, for conceptual consistency. This is purely a readability/maintainability suggestion—no functional difference since TriggerProducts is included in all the same output command sequences.

2. [S2] Minor — Potential for duplicate keep rules in downstream configs

   • Evidence: Reconstruction.GeneralProducts (in JobConfig/reco/prolog.fcl, line 324–327) includes @sequence::Digitize.TriggerProducts. The new EventHeader keep rule will therefore appear in reconstruction outputs as well. This is likely the desired behavior, but worth confirming that no downstream job intentionally excluded EventHeader to manage output file size.
   • Impact: Low risk. The EventHeader is a small data product, so the file-size impact is negligible.
   • Suggested fix: No code change needed, but the author should confirm that adding EventHeader to reco output files is also intentional (or at least acceptable).

3. [S3] Nit — The wildcard pattern is very broad

   • Evidence: "keep mu2e::EventHeader_*_*_*" will keep EventHeader from any module label, any instance, and any process. In the author's own validation, the product is from ProcessCFOData with process Digitize. If only the CFO-produced header is expected, a more specific pattern like "keep mu2e::EventHeader_ProcessCFOData_*_*" could be used.
   • Impact: Minimal—there's currently only one EventHeader producer in the digitize sequence. The wildcard approach is defensive and consistent with the style used for other products in this list (e.g., EventWindowMarker_*_*_*).
   • Suggested fix: No change required; the wildcard is consistent with convention. Flagging for awareness only.

4. [S2] Minor — PR description is empty

   • Evidence: The PR body is null. The author provided validation evidence as a comment rather than in the PR description.
   • Impact: Reduces discoverability for future readers trying to understand why this change was made.
   • Suggested fix: Best practice reminder: please add a meaningful PR description including the intent (why EventHeader needs to be on the output), scope, and the validation evidence you already posted in the comment.

Validation check

• Build/tests run: Partial — the author demonstrated the product is present in output via dumpDataProducts. No explicit build log or CI status referenced.
• Config contract check: Pass — the keep rule uses the standard art output command syntax and the mu2e::EventHeader type exists in artdaq-core-mu2e/Data/EventHeader.hh (confirmed in Offline/DAQ sources).
• Cross-repo consistency: Pass — no Offline or mu2e-trig-config changes appear needed; EventHeader is already produced by EventHeaderFromCFOFragment in Offline/DAQ, and ProcessCFOData is already in the digitize sequence (line 242). This PR simply retains the product on disk.

Residual risk

• Very low. This is a single additive keep rule for a small data product. No existing behavior is modified; events that previously dropped EventHeader will now retain it.
• Slight increase in output file size (negligible for a single EventHeader per event).

Author follow-ups

1. Please add a PR description with intent and validation summary.
2. Consider whether GeneralProducts is a more appropriate home than TriggerProducts for EventHeader (no functional difference, purely organizational).
3. Confirm that the propagation of this keep rule into reconstruction outputs (via Reconstruction.GeneralProducts) is acceptable/intended.

[FNALbuild]

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

• JobConfig

which require these tests: build.

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

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

About FNALbuild. Code review on Mu2e/Offline.

[FNALbuild]

☀️ The build tests passed at 8ffcbd0.

Test	Result	Details
test with	✅	Command did not list any other PRs to include
merge	✅	Merged 8ffcbd0 at 1d935bf
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.
FIXME, TODO	✅	TODO (0) FIXME (0) in 0 files
clang-tidy	✅	0 errors 0 warnings

N.B. These results were obtained from a build of this Pull Request at 8ffcbd0 after being merged into the base branch at 1d935bf.

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.

[michaelmackenzie]

@brownd1978 would you prefer the keep line in the trigger products or would you like me to move it to the general products? Downstream reco jobs keeping this object is intentional (responding to the AI comment)

[brownd1978]

Thanks for fixing this.