Fix product labels for tokens in VolumeBasedMagneticFieldESProducer #27751
Conversation
These were in the get() calls before the consumes migration
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-27751/11401
|
|
A new Pull Request was created by @makortel (Matti Kortelainen) for master. It involves the following packages: MagneticField/GeomBuilder @perrotta, @cmsbuild, @slava77 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild, please test |
|
The tests are being triggered in jenkins. |
|
@makortel: What tests have been or will be done to validate this change? I ask because I would like to know the best way to test this code. I wrote a test config in my PR #27674: https://github.com/cms-sw/cmssw/pull/27674/files#diff-9221e02eecfe8f14258a20b915321e23 Do you think my test config is correctly written, or do you have a better one? |
|
Comparison job queued. |
As I wrote in the PR description, I only tested that the code compiles. Beyond that I'm relying on our e-mail discussion some weeks ago.
I don't know, I'm not an expert in magnetic field. I did, however, took your configuration file |
|
That's enough; however, to answer Carlo's question, the best test is MagneticField/Engine/test/regression.py (which by default uses the map builder from xml, so would require no modifications to test this PR). |
@namapane Thanks for pointing out that test. I can confirm that with this PR that configuration works (while without it does not). Would it make sense to make that configuration to be run as part of the unit tests? If it were, the problems in #27412 would have been found earlier through IBs. |
|
I'm not sure, what we are testing here is a special producer that is in place for development and special use cases only and not used in production. We could add it to unit tests but we should probably add the corresponding tests for the default producer as well. Before doing that I would prefer to complete the clean-up that is discussed in #27674. |
|
Comparison is ready Comparison Summary:
|
|
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. @davidlange6, @slava77, @smuzaffar, @fabiocos (and backports should be raised in the release meeting by the corresponding L2) |
|
@slava77: I don't think this producer is used in any workflow. It is only called from certain test scripts, like the |
If this esproducer would be tested in any IB tests, the infinite recursion etc caused by omitting the labels would have been caught earlier... |
I somehow thought that the ES get will give you a record with a label silently, if that's the only known/registered record for a given payload type. |
Nope, the product label is used in the product look-up in addition to the product type and the record (and an empty label is still a label). The module label, on the other hand, is not used in the product look-up, but if it is non-empty in the consuming ESInputTag, the label is checked after the product has been found (and an exception is thrown if they are not equal). |
|
+1 although this is not creating issues in the usual test workflows it is anyway a fix for an incorrect behaviour to be added |
PR description:
The PR #27412 that migrated VolumeBasedMagneticFieldESProducer to use ESGetToken introduced two bugs (thanks to @cvuosalo for reporting): the product labels for the
cpvToken_andparamFieldToken_were left unset while the correspondingget()calls did have values before #27412. This PR adds the labels back (and is part of #26748).PR validation:
Code compiles.