Add unit test for TriggerResults format #41395
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41395/35262
|
|
A new Pull Request was created by @wddgit (W. David Dagenhart) for master. It involves the following packages:
@cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
A couple of comments on related history: The TriggerResults format has not changed since 2006. In 2014 something in ROOT changed that affected the checksum and we had to add a new version number in classes_def.html. We have an existing unit test that is similar to this new one. It is in IOPool/Input/test/TestPoolInput.sh. See this line and what follows it: "for file in ${IOPoolInputData}/IOPool/Input/data/raw*.root". It reads an old existing file with RAW and then writes a new file. A second process reads the new converted file. There is not an analyzer that reads content, just PoolSource and PoolOutputModule. Here is the content of the old file: Singularity> edmFileUtil file:/cvmfs/cms-ib.cern.ch/sw/x86_64/week1/el8_amd64_gcc11/cms/cmssw-patch/CMSSW_13_1_X_2023-04-18-1100/external/el8_amd64_gcc11/data/IOPool/Input/data/rawData_old_format_CMSSW_4_2_8.root -P I think the intent was that new versions of this file would be added if RAW formats changed. That never happened, there is only the CMSSW_4_2_8 file. Although only the RAW format L1GlobalTriggerObjectMapRecord changed. It is a completely different type now apparently. The other formats have not changed. |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-503e27/32113/summary.html Comparison SummarySummary:
|
There was a problem hiding this comment.
Thanks @wddgit, overall looks good. Could you also create a DataFormats/Common/README.md with content along
DataFormats/Common
edm::TriggerResultsThe
edm::TriggerResultsis part of the RAW data, and any changes must be backwards compatible. In order to ensure it can be read by all future CMSSW releases, there is aTestTriggerResultsFormatunit test, which makes use ofTestReadTriggerResultsanalyzer andTestWriteTriggerResultsproducer. The unit test checks that the object can be read properly from
- a file in the same release as it was written
- files written by (some) earlier releases can be read
If the
edm::TriggerResultsgets changed in the future, please adjust theTestReadTriggerResultsandTestWriteTriggerResultsmodules accordingly. It is important that every member container has some content in this test. Please also add a new file to https://github.com/cms-data/DataFormats-Common/ repository, and update theTestTriggerResultsFormatunit test to read the newly created file. The file name should contain the release or pre-release with which it was written.
@Dr15Jones Do you have comments on the README contents?
There was a problem hiding this comment.
I'd record the exact release version in the file name as things can, in principle, change within a release cycle. I think we should also prefer (pre-)releases over IBs.
I'd also try to include a file with e.g. 13_0_0 so that we'd have a test file affected by #41348.
| DataFormatsCommonData=$CMSSW_BASE/src | ||
| for dir in $(echo $CMSSW_SEARCH_PATH | tr : '\n') ; do | ||
| if [ -d ${dir}/DataFormats/Common/data ] ; then | ||
| DataFormatsCommonData=${dir} | ||
| break | ||
| fi | ||
| done | ||
|
|
||
| for file in ${DataFormatsCommonData}/DataFormats/Common/data/oldTestTriggerResults*.root | ||
| do | ||
| cmsRun ${LOCAL_TEST_DIR}/test_readTriggerResults_cfg.py "$file" || die "Failed to read old file $file" $? | ||
| done |
There was a problem hiding this comment.
How about listing the expected files explicitly here and using (recently added) edmFileInPath script to find the full path of DataFormats/Common/data/<file.root>? Then we'd see also failures if any of the files would be missing for any (e.g. infrastructural) reasons.
There was a problem hiding this comment.
Done. That is better.
|
@smuzaffar Could we get a https://github.com/cms-data/DataFormats-Common/ repository to store the root file(s)? |
|
https://github.com/cms-data/DataFormats-Common is available now |
|
@smuzaffar Thanks for the new repo. What is the procedure to get a data file into that repo? Do I submit a PR against that repo or is there some other way to do that? |
wddgit
force-pushed
the
testTriggerResults
branch
from
06b1d2d
to
cc027c6
Compare
cmsbuild
added
tests-pending
code-checks-pending
and removed
tests-approved
code-checks-approved
labels
|
I think the commit I just pushed addresses all the comments. I squashed it. The shell script changed significantly. The data file was removed. The documentation file was added. The rest of it remained exactly the same as before. |
|
test parameters: |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-41395/35273
|
cmsbuild
added
requires-external
code-checks-approved
and removed
code-checks-pending
labels
|
Pull request #41395 was updated. @cmsbuild, @smuzaffar, @Dr15Jones, @makortel can you please check and sign again. |
|
please test with cms-data/DataFormats-Common#1 Note the unit test will fail unless merged at the same time as or after the data file PR. |
|
Just for the record, I wanted to make the comment that the new test added by this PR will not be very good at detecting the problem discussed in issues #41348 and #41246. For it to detect that particular problem, a test input file would have to be written using a release with that problem (note that particular problem was associated with an internal ROOT change and not a data format change) and then later (potentially years later) if the persistent format of TriggerResults changed, then this test would detect the problem. That said, that problem motivated us to think about RAW data format testing and implement the improvements in this PR. Given that we are supposed to guarantee backward compatibility for RAW data formats forever and the importance of RAW data, the new test in this PR is a logical unit test to have and run. |
|
+core |
cmsbuild
added
core-approved
fully-signed
and removed
core-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). 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
|
PR description:
Add a new unit test for the TriggerResults data format. This generates a data file containing a TriggerResults object with known content. Then it reads it. It verifies that when we read it we obtain values that match the known written values for all the data fields in the object. In particular all containers have content so all contained types are also read.
It also reads the old files in the DataFormats/Common data repository which are listed in the shell script. The plan is that each time the data format of TriggerResults is modified a file will added.
PR validation:
This only adds a unit test in DataFormats/Common which passes. It shouldn't affect anything else.