Replaced the InputFilePeeker with MetaReader in Trigger
We replaced the InputFilePeeker with Metareader, a new faster tool
Merge request reports
Activity
This merge request affects 9 packages:
- Trigger/TrigAnalysis/TrigEgammaAnalysisTools
- Trigger/TrigAnalysis/TrigEgammaEmulationTool
- Trigger/TrigEvent/TrigNavTools
- Trigger/TrigMonitoring/TrigHLTMonitoring
- Trigger/TrigT1/L1Topo/L1TopoSimulation
- Trigger/TrigT1/TrigT1CTMonitoring
- Trigger/TrigT1/TrigT1CaloSim
- Trigger/TriggerCommon/TrigTier0
- Trigger/TriggerCommon/TriggerJobOpts
added Egamma Trigger master review-pending-level-1 labels
-
Athena AthSimulation externals 
cmake make required tests 
optional tests Full details available at NICOS MR-24196-2019-06-14-18-40
-
I think q221 and q440 failed due to a typo (I commented the offending line).
However, q431 looks different:
RAWtoESD 17:16:28 ToolSvc.HLT::HLTResultByteStreamTool ERROR Cannot store object with name in BS: we are expecting only HLTResult_(L2,EF,HLT) RAWtoESD 17:16:28 HLTResultByteStreamCnv ERROR Failed to create objectand then there's a segfault. Note the empty name string in the error message above. This may be caused by the changes in HLTTriggerResultGetter.py (although I'm not 100% sure).
-
Another comment - have you tested all the changes? I'm not sure if all the changed job options are still in use and functional, but some of the touched files are critical, like the HLTTriggerResultGetter.
Hopefully, the CI tests should cover all the critical cases, but tagging @wiedenma, @smh, @fwinkl, @stelzer, @fernando in case they want to have a look and suggest extra tests.
Cheers,
Rafal assigned to @mrenda
added review-user-action-required label and removed review-pending-level-1 label
Hello @rbielsky,
I am in charge on this issue.
I have testes all the modifications locally and we got success in every test_*.sh in the Trigger folder.
Unfortunately, it seems I missed something, but unfortunately, tomorrow I will not be in the office so I will not be able to give you a detailed answer until Wednesday.
I can ensure we analyzed more than 1 GB of log files for every test session because we know how important this section of code is (I will attach our test report).
Thank you very much for your support!
Michele
Hello @rbielsky,
I investigated this issue and it was a typo done just before we performed the commit for merge request. Due to the peculiarity of this task, we had to develop a different test mechanism than the usual ctest: we had to be sure not only the test succeeded, but that every call to metadata was returning the same value as the old tool.
We developed a custom solution, and we use this to compare everything is fine. Unfortunately, before we can perform the commit, we have to delete all the comments and, during this activity, we accidentally inserted the typo. We apologize for this event.
Attached there are the results of our test: result.csv
Columns where we have "NOT-OK" were manually checked, and they are minor issues (mainly, or wrong order in unordered list or additional elements, which is OK)
We corrected the typo, and we looked for similar occurrences, but we will have to wait for the finish of the pipeline to be sure everything is OK.
Best regards Michele
-
This merge request affects 9 packages:
- Trigger/TrigAnalysis/TrigEgammaAnalysisTools
- Trigger/TrigAnalysis/TrigEgammaEmulationTool
- Trigger/TrigEvent/TrigNavTools
- Trigger/TrigMonitoring/TrigHLTMonitoring
- Trigger/TrigT1/L1Topo/L1TopoSimulation
- Trigger/TrigT1/TrigT1CTMonitoring
- Trigger/TrigT1/TrigT1CaloSim
- Trigger/TriggerCommon/TrigTier0
- Trigger/TriggerCommon/TriggerJobOpts
added review-pending-level-1 label and removed review-user-action-required label
-
Hi @mrenda,
I appreciate your work and the effort in testing. It looks like we don't have a test in the Trigger directory currently in the master branch which could detect the failure which was observed in the CI pipeline. This is probably an oversight on our side, but fortunately we always have the "q-tests" in the CI, which test things like this. The failure I mentioned happened in the q431 test, in reading Trigger data in a Reconstruction job. This is a Reconstruction test, so it picked this up. If I may suggest an extension to your procedure, it would be good if you also run "Tier0ChainTests" even for changes in other domains:
https://gitlab.cern.ch/atlas/athena/tree/master/Tools/Tier0ChainTests/test
or at least run those which are part of the CI pipeline and failed for your MR, namely q221, q431 and q440.Thanks,
Rafal -
Athena AthSimulation externals cmake make required tests optional tests Full details available at NICOS MR-24196-2019-06-20-00-43
-
The same failure as mentioned above in !24196 (comment 2654328)
added review-user-action-required label
Yes, same failure but different root cause. Unfortunately, we can not perform the testing in our data center (we have an unscheduled maintenance shutdown) and, in next days, we have to perform testing on lxplus.
We will continue our testing and when we are ready for the merging, we will remove the label review-user-action-required.
Best regards Michele
