Bug fix in EMTF GEM and RPC unpacker blocks #39460
Conversation
|
type bugfix |
|
urgent |
|
@cmsbuild , please test
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-39460/32183
|
|
A new Pull Request was created by @eyigitba for master. It involves the following packages:
@epalencia, @rekovic, @cecilecaillol can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
+l1 |
cmsbuild
added
fully-signed
l1-approved
and removed
l1-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs after it passes the integration tests. This pull request will now be reviewed by the release team before it's merged. @perrotta, @dpiparo, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
+1
|
|
merge |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7b33a4/27690/summary.html Comparison SummarySummary:
|
|
On Sep 20, 2022, at 10:43 PM, Andrea Perrotta ***@***.***> wrote:
+1
• The fix is quite logical, and as we realized there is no way to actually test it with the IB tests.
Why is there "no way"? (There is now clearly as express (at least) crashes) - but even in that case one can ask the requestor how was the code checked/validated.. I guess it wasn't in this case.
|
|
@davidlange6 , I verified the fix by testing it using the offline recipe as described in #39456. Previously this error was missed in tests because I tested it using a run where GEM was offline (I wasn't aware). Since the firmware version change is quite recent, probably there were no IB tests using data collected with this firmware version. So the error did not appear in those tests. |
|
On Sep 21, 2022, at 9:21 AM, eyigitba ***@***.***> wrote:
@davidlange6 , I verified the fix by testing it using the offline recipe as described in #39456.
Previously this error was missed in tests because I tested it using a run where GEM was offline (I wasn't aware). Since the firmware version change is quite recent, probably there were no IB tests using data collected with this firmware version. So the error did not appear in those tests.
But nor did that test show that your code actually worked - thats part of my point:) [the integration procedure needs to include positive checks not just lack of negative ones..imho]
… —
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.
|
Hi David. Let me rephrase in hopefully some better English. That said, yes I fully agree that the PR and IB tests should include Run3 data samples from runs that also include GEM, as it is not the case for the current ones. I'd let @cms-sw/pdmv-l2 deal with it and find the appropriate input files for them. |
|
@davidlange6 , right that's a good point. I only verified the RPC part in my test and I didn't check the GEM part. On top of that, apparently I made a mistake when cleaning the RPC part which ended up being broken as well. I agree that checking for positive outcome is required (which I, apparently, failed to do properly). |
PR description:
This PR fixes a bug that was mentioned in #39456
The bug was introduced in #39388
It was a stupid mistake on my part that somehow went in. The index for the for loop does not match the if/else statements, hence causing an empty GEM/RPC TP to be created which then causes the crash when we try to create detID.
This should be backported to 12_4_X and 12_5_X to fix the same ug in those releases.
PR validation:
Tested with running the unpacker on the data files mentioned in #39456. Unpacker runs and assigns the correct values to GEM/RPC TPs.
File used:
/eos/cms/store/t0streamer/Data/Express/000/359/045/run359045_ls0144_streamExpress_StorageManager.dat