higher scrutiny on changes to RAW data formats

[missirol]

Early on in the review of #42634, @fwyzard caught by eye a change which affected one of the data formats of the RAW data tier, and would have changed the meaning of existing data when reading it with the latest releases (see #42634 (comment) and replies).

In fact, it was later realised that a similar mistake did go in in CMSSW_12_3_0_pre2 (#42634 (comment)). In our understanding, this means that any later release would misinterpret the content of older RAW data for that specific data format.

A while go, Core-sw introduced tests to ensure the backward-compatibility of changes to data formats of the RAW data tier (see cms-sw/framework-team#530 and links therein). As far as I understand, a problem such as #42634 (comment) goes beyond the scope of those unit tests. Still, I'm left wondering how we should avoid this kind of issue moving forward.

Questions:

• Would it be useful/feasible for Core-Sw to review all PRs that involve changes to the RAW (and RawPrime ?) data formats ?
• Would it be possible to build on tests such as Add unit test for GlobalObjectMapRecord format #41565 to spot issues like the one discussed in L1T ZDC and uGT Emulator to CMSSW_13_3_X #42634 (comment) ?
• Does Core-Sw have any suggestions/recommendations ?

[cmsbuild]

A new Issue was created by @missirol Marino Missiroli.

@Dr15Jones, @rappoccio, @smuzaffar, @makortel, @sextonkennedy, @antoniovilela can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

[fwyzard]

assign core, l1, hlt

[cmsbuild]

New categories assigned: core,hlt,l1

@epalencia,@mmusich,@missirol,@Dr15Jones,@smuzaffar,@aloeliger,@makortel,@Martin-Grunewald you have been requested to review this Pull request/Issue and eventually sign? Thanks

[makortel]

• Would it be useful/feasible for Core-Sw to review all PRs that involve changes to the RAW (and RawPrime ?) data formats ?

For the particular case of #36018 (e.g. assuming the PR would have been assigned to core ) I'm not confident at all that I personally would have realized the enum is stored as part of RAW. I would also argue in this case the change is in how the data is interpreted rather than how it is stored (the enum is stored as an integer, and the additional enum element did not cause the integer width to change).

I don't think adding more eyes would be the best way forward. Core's responsibility has been to ensure the bits of the data formats are read correctly, while the correctness of the interpretation has been left to the owners of the data formats.

• Would it be possible to build on tests such as Add unit test for GlobalObjectMapRecord format #41565 to spot issues like the one discussed in L1T ZDC and uGT Emulator to CMSSW_13_3_X #42634 (comment) ?

If the tests added in #41565 would have used the enum representation of the data rather than always going through integer values (that was a reasonable choice for ensuring the bits are read correctly), it should have caught the new enum being added.

• Does Core-Sw have any suggestions/recommendations ?

I agree tests for the interpretation of the enum should be added.

I'd also suggest to add comments for all the relevant classes / enums / etc that these are part of the RAW data, and one needs to be careful when modifying them. And for enums especially instructions where to add new elements.

@Dr15Jones suggested to number the enum elements explicitly for enums in data formats, that would make it very clear the consequences of adding an element in the middle.

[makortel]

In the core software meeting today it was noted that at the time the #36018 was merged (CMSSW_12_3_0_pre2), the physics validation was still presumably done using Run 2 data. In principle that validation could have caught the mistake, but apparently it didn't.

A question on how the GlobalTriggerObjectMap is being monitored in DQM was also brought up. Perhaps there is room for improvement in general for monitoring the (unpacked) contents of RAW data?

@cms-sw/pdmv-l2 @cms-sw/dqm-l2

[makortel]

We should also understand how the GlobalTriggerObjectMap, and especially GlobalObjectMap::operandTokenVector() is used downstream. The main consumer seems to be HLTL1TSeed, that is used as part of the HLT menu, but it is currently unclear if this information is propagated onwards as part of higher-level data products.

One way to make the pre-CMSSW_12_3_0_pre2 GlobalTriggerObjectMap to be interpreted correctly again would be to add a configuration parameter to HLTL1TSeed to control whether a fix needs to be applied or not, and control that with e.g. an era modifier. A challenge is that the need of the fix is determined by the release used to write the GlobalTriggerObjectMap (e.g. while all Run 2 data need the fix, any Run 2 MC produced with 12_3_0_pre2 or afterwards do not). In principle the version information is part of the provenance information, but using that information as part of the data processing would have a cost.

[missirol]

@Dr15Jones suggested to number the enum elements explicitly for enums in data formats, that would make it very clear the consequences of adding an element in the middle.

Thanks for this suggestion. I tried to implement it in #43022.