-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Initial implementation of the FixMissingStreamerInfos service #43175
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43175/37502
|
A new Pull Request was created by @wddgit (W. David Dagenhart) for master. It involves the following packages:
@cmsbuild, @makortel, @smuzaffar, @Dr15Jones can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test with cms-data/IOPool-Input#2 |
-1 Failed Tests: RelVals RelVals-INPUT AddOn RelValsRelVals-INPUTAddOn Tests
Expand to see more addon errors ...
|
please test with cms-data/IOPool-Input#2 Try again. Maybe the failures are a glitch. It is hard to see how this could cause anything to fail outside the single unit test it adds. There is a modified unit test, a new script and a new service. The script and service are not currently used by anything other than the unit test. |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-973223/35584/summary.html Comparison SummarySummary:
|
Comparison differences are related to #39803 |
@@ -102,4 +102,6 @@ inputfile=$(edmFileInPath IOPool/Input/data/$file) || die "Failure edmFileInPath | |||
#root.exe -b -l -q file:$inputfile "${LOCAL_TEST_DIR}/testForStreamerInfo.C(gFile)" | sort -u | grep Missing > testForStreamerInfo2.log | |||
#grep "Missing" testForStreamerInfo2.log && die "Missing nested streamer info" 1 | |||
|
|||
cmsRun ${LOCAL_TEST_DIR}/SchemaEvolution_test_read_cfg.py --inputFile $inputfile --enableStreamerInfosFix || die "Failed to read old file $file with fix" $? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe the comment above could be rephrased now e.g. along "the test would fail without the --enableStreamerInfosFix
"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I edited that comment using your suggestion and also generally shortened the comment. It's better now I think. Thanks.
#include <iostream> | ||
|
||
void makeFileContainingStreamerInfos() { | ||
std::cout << "Executing makeFixitFile()" << std::endl; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name in the printout is different from the function and file name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed that so the printout and names match. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have mixed feelings about this file being in scripts
directory. Those files end up in $PATH
, but this macro needs to be explicitly ran via root ...
. But I wouldn't place it e.g. in tests
either, so maybe this is the "least bad" option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also have mixed feelings about that. I'll move it to tests if you want. It's not really a test though. It does not seem like it will be used often enough to deserve being in the PATH. I can't think of any other options though. Is there any other place I could put it? Maybe a subdirectory of scripts? Would that help? Or I could make up a new directory and call it rootScripts... If you want me to move it, make a suggestion and I will move it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Dr15Jones @smuzaffar Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe given @smuzaffar's reply in #43174 (comment) this placement is kind of ok as long as the file does not have execution permissions (which I think is already the case).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't give it executable permissions in my local working area so we should be good. Lets just leave it there.
d100af2
to
dcf316f
Compare
@cmsbuild, please test Let's see if we could get clean comparisons. The PR itself looks good. |
-1 Failed Tests: UnitTests Unit TestsI found 1 errors in the following unit tests: ---> test TestIOPoolInputSchemaEvolution had ERRORS Comparison SummarySummary:
|
Comparison failures are related to #39803 |
@cmsbuild, please test with cms-data/IOPool-Input#2 Forgot the external... |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-973223/35666/summary.html Comparison SummarySummary:
|
Comparison failures are related to #39803 |
+core |
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. @sextonkennedy, @antoniovilela, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
@cms-sw/orp-l2 Could this PR be merged (it needs also cms-data/IOPool-Input#2)? Thanks! |
+1 |
PR description:
The version of ROOT associated with CMSSW_13_0_0 had a bug that caused it to fail to write out StreamerInfo objects for some types in the output. This affected some other releases in the 13_0_X and 13_1_X release cycles. It was fixed in both of those release cycles and didn't affect others. See Issue #41246 for more details.
The missing StreamerInfos don't actually cause a problem until the data format of the associated class changes and someone tries to read the file. Then the schema evolution feature of ROOT is needed and that requires the StreamerInfo objects. This could occur much later.
This PR introduces a workaround fix that allows one to read problem objects in those files. It adds a new service that will read in a file containing only StreamerInfo objects. Then the problem objects become readable and schema evolution succeeds. One sets a parameter in the service to point to a file that contains only the missing StreamerInfo objects and that causes them to be stored in memory.
The PR includes a script that can be used to generate the file that contains the StreamerInfo objects.
One shortcoming of this PR is that it is hard to identify all problem types. We found all types missing the StreamInfo in one input file and there was one additional type identified in the initial problem reports. One can use the script to generate a new file if more problem types are encountered in the future.
There is a second PR associated with this one that will add 2 files to the cms-data repository associated with IOPool/Input. This PR should not be merged before the cms-data PR.
PR validation:
A new unit test is included to verify that this workaround will succeed.