SubProcess bug fix, updateLookup called too early #46871
Conversation
|
cms-bot internal usage |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46871/42899 |
|
A new Pull Request was created by @wddgit for master. It involves the following packages:
@Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
-1 Failed Tests: RelVals RelVals-INPUT
RelVals
Expand to see more relval errors ...
RelVals-INPUT
Expand to see more relval errors ...
|
|
please test Try again. Bunch of DAS Errors. I don't this could be related to the PR. I see them in one of the IBs also. |
|
Looks good to me. @Dr15Jones could you also take a look? |
|
+1 Size: This PR adds an extra 12KB to repository Comparison SummarySummary:
|
|
Comparison failures are very similar to those seen in the completely unrelated PR #46859 from about the same time period. So I don't think they are related to this PR. |
wddgit
force-pushed
the
bugFixESUpdateLookup
branch
from
0c1e0d5 to
126bf82
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-46871/42951 |
|
Pull request #46871 was updated. @Dr15Jones, @cmsbuild, @makortel, @smuzaffar can you please check and sign again. |
|
I added two comments to the code in EventSetupsController.cc as we discussed verbally earlier today. Let me know if there are any more review comments. |
|
please test |
|
+1 Size: This PR adds an extra 24KB to repository Comparison SummarySummary:
|
|
+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. @mandrenguyen, @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
+1 |
PR description:
This is a bug fix that should only affect jobs using SubProcess. For ESProducers, the updateLookup function was being called too early. For SubProcesses, there is a late step where sharing of ESProducers is checked and new ESProducers might be created at that point. We have been calling ESProducer::updateLookup just before that so ESProducers constructed at that late time would have missed the call to updateLookup.
This only affects jobs with SubProcess and none of the CMS production jobs use that feature. It is also somewhat of an unusual corner case even in a job with SubProcess. As far as I know, the Core unit tests are the only tests using SubProcess.
PR validation:
Existing unit tests pass.
I noticed this while developing a separate PR. I will submit the other PR in the near future and it contains code modifications that cause existing unit tests to fail because of this bug (that is how I noticed the bug in the first place, it has been lurking there for years).