[DD4hep] DDDetector ES producer from DB #34167
Conversation
|
please test |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-34167/23375
|
|
A new Pull Request was created by @ianna (Ianna Osborne) for master. It involves the following packages: DetectorDescription/Core @civanch, @Dr15Jones, @makortel, @cvuosalo, @ianna, @mdhildreth can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cvuosalo and @civanch - Please, add the following modifier in the GeometrySimDB_cfi: def _loadGeometryESProducers( process ) :
process.load('DetectorDescription/DDCMS/DDDetectorESProducerFromDB_cfi')
from Configuration.Eras.Era_Run3_dd4hep_cff import Run3_dd4hep
modifyGeometryConfigurationRun3DD4hep_cff_ = Run3_dd4hep.makeProcessModifier( _loadGeometryESProducers )For all other Eras the following configuration fragment should be kept, but I'm not sure how to do it - @fabiocos ? from GeometryReaders.XMLIdealGeometryESSource.cmsGeometryDB_cff import *As soon as it's done, current workflows should work with Perhaps, there is another way of configuring it? |
| confGeomXMLFiles_(iConfig.getParameter<FileInPath>("confGeomXMLFiles").fullPath()), | ||
| confGeomXMLFiles_(iConfig.existsAs<FileInPath>("confGeomXMLFiles") | ||
| ? iConfig.getParameter<FileInPath>("confGeomXMLFiles").fullPath() | ||
| : "none"), |
There was a problem hiding this comment.
Using the existence of parameters to control module behavior is generally considered as undesirable behavior. How about adding an explicit parameter to control whether the XML file is used or not?
There was a problem hiding this comment.
Nevermind, the parameter is already declared as optional in the fillDescriptions().
There was a problem hiding this comment.
Actually, coming back. Guessing a bit the desired behavior based on the fillDescriptions(), if the confGeomXMLFiles is required when fromDB == False, how about
confGeomXMLFiles_(fromDB_ ? "none" : iConfig.getParameter<FileInPath>("confGeomXMLFiles").fullPath()),?
If this is the case the fillDescriptions() could also be enhanced along (untested)
desc.ifValue(edm::ParameterDescription<bool>("fromDB", false, true),
false >> edm::ParameterDescription<FileInPath>("confGeomXMLFiles", true));https://twiki.cern.ch/twiki/bin/view/CMSPublic/SWGuideConfigurationValidationAndHelp#A_Switch
There was a problem hiding this comment.
I'd think, the latter is not needed, because there are two configurations produced: one for DB and another for XML files. It seems clear which one needs the confGeomXMLFiles.
|
@ianna I'd like to understand better the configuration issue. I see the |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-1ce2ef/16082/summary.html Comparison SummarySummary:
|
Thanks! Yes, as |
cmsbuild
added
code-checks-pending
tests-pending
and removed
code-checks-approved
tests-approved
labels
|
please test |
|
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. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
In principle the ESProducers for both DDD and DD4Hep can be unconditionally included in all configuration files (as long as the constructors of those ESProducers do not do anything "too fancy"). The consumers dictate fully the set of ESProducers that are run. I.e. if all consumers read the DD4Hep products, then only the DD4Hep ESProducers are run. My concern (that I tried to express in the simulation meeting), given that we're doing a migration, is that what if a mistake appears and some module reads a DDD ESProduct when it should have used DD4Hep product. If both the |
|
@makortel A DD4hep configuration should not be using old DD. It would be good to find out as soon as possible if such an error is occurring so it can be fixed. An old DD configuration should not be using DD4hep, though I think this error would be less serious. I would prefer that this condition would create a warning message but still allow the configuration to run. I am just imagining some obscure but important legacy workflow that starts partly using DD4hep after we change Run 3 to DD4hep. For it to suddenly and unexpectedly start failing after the changeover might be worse than for it to keep running in a non-optimal way. |
|
@cvuosalo Could you clarify what exactly do you mean with
? Would this be about changing the geometry payloads (that I've understood should be equal) in the CondDB? Or about using |
|
@makortel To convert a workflow to DD4hep, one adds the Note that not all DD4hep geometry payloads are identical to old DD payloads. The upcoming simulation geometry DB payload, which is not in the DB yet, differs between old DD and DD4hep. Because the Reco geometry is highly processed and simplified, all the Reco geometry DB payloads are identical between old DD and DD4hep. |
|
Thanks @cvuosalo, this is very helpful. I'm still a bit confused, so bear with me. Is your concern then a possible legacy workflow that would use the old DDD simulation payload even when configured with the Is the plan to have both DDD and DD4Hep simulation payloads in the same GT? What would happen if DD4hep workflow would use DDD simulation paylod, or vice versa? Failure with clear error message, crash, or silent "corruption"? (I'm overall pessimistic on the efficacy of warning messages, in my experience those tend to be ignored easily) |
|
@makortel One concern is that a workflow configured for DD4hep would use the DDD simulation payload, yes. But I shouldn't have used the word "legacy". The only workflows that will use DD4hep at this time are Run 3 workflows. We are trying to work out with AlCaDB that the DD4hep GT will NOT contain any DDD geometry payloads, though the only payloads of concern are the simulation ones, since Reco geometry payloads are identical. Similarly, the existing Run 3 GT contains only DDD geometry payloads. If a DD4hep workflow used a DDD simulation payload, I think it would run and produce results, but we have never examined how that works. It is not intended to work that way. Since DD4hep is new, I think it is best to write the code to simply prohibit this behavior. If a DDD workflow read the DD4hep simulation payload, it would probably work, but the results wouldn't be reliable since that usage has never been checked. I'm not sure whether to write the code to simply prohibit this case, or to let it run with an error message. It is not intended behavior, but in the case of error, what behavior is preferred, total failure or limping along? |
BTW, it is wrong calling it a "simulation" payload. It is an ideal geometry description payload. The only way to ensure a correct geometry description version usage is by choosing a GT. It shouldn't matter if a payload was produced using DDD or DD4hep. CMS geometry description is independent from the tools we use to process it.
No.
It should work either way. There isn't anything special introduced to the geometry description that is specific to DD4hep.
|
It is our intention to support run 1 and run 2 geometry descriptions, hence it must work.
Why would CMS detector description be different? IMHO, it would be more clear if we name the payload correctly - not simulation, but an ideal geometry description payload. If there is any intention to produce warnings or errors, I'd suggest that this is done through configuration fragments and not in the XML nor in C++. |
I think it is important to distinguish between usage that SHOULD work and that we will need to work later, and usage that has been validated to work. At the present time, DDD reading the DDD ideal geometry description has been validated (long ago), and DD4hep reading the DD4hep ideal geometry description payload is mostly validated but still in progress. The other combination of DD4hep reading the DDD ideal geometry description payload will need to be checked and validated when we get to the Runs 1 and 2 DD4hep migration. For Run 3, I think it would be helpful to be warned if a DD4hep workflow is reading a DDD ideal geometry description. Our intention is for Run 3 to soon use DD4hep exclusively. If a Run 3 DD4hep workflow is reading a DDD ideal geometry description payload, that would be an unintended oversight which would be helpful to track down and fix. That said, we should keep in mind that we eventually want to read the Runs 1 and 2 DDD ideal geometry description payloads with DD4hep, so any temporary mechanism in CMSSW that prohibits this mixing should be well-documented and very easy to change. |
|
Thanks for all the explanations. It sounds to me that for now an "exclusive or" between DDD and DD4Hep codes could be the best way to proceed. That could be done by extending what @ianna's #34167 (comment) to something along def _loadGeometryESProducers(process) :
process.load('GeometryReaders.XMLIdealGeometryESSource.cmsGeometryDB_cff')
def _loadGeometryESProducersDD4Hep(process) :
process.load('DetectorDescription.DDCMS.DDDetectorESProducerFromDB_cfi')
from Configuration.ProcessModifiers.dd4hep_cff import dd4hep
modifyGeometryConfiguration = (~dd4hep).makeProcessModifier(_loadGeometryESProducers)
modifyGeometryConfiguration_dd4hep = dd4hep.makeProcessModifier(_loadGeometryESProducersDD4HEP)As for detecting inconsistencies, my preference is to "fail early and loudly", especially for things like job configuration errors. But, given that both DDD and DD4Hep codes should in principle (or eventually) work (at least technically) with the geometry payload in CondDB (prepared with either DDD or DD4Hep), I don't have good ideas how to detect that DDD code would use payload prepared with DD4Hep or vice versa (unless the payload itself would know). I suspect the "tag name" in CondDB is not available for inspection in CMSSW (beyond CondDBESSource). To rephrase more generally, the GlobalTag and the (or I'm missing something) |
Thanks! This is exactly what I was looking for.
GT is our version control. IMHO, the only way to ensure consistency is via a GT that does not have any DDD - based payloads mixed with DD4hep - based ones. This will help to weed out the consumers which haven’t been customized yet. If we want to introduce a version recorded inside of a payload, a proper mechanism ought to be put in place. For example, it would be possible to introduce version control via CDL and DDL schemas.
I agree, it can get out of control easily. Especially that we rely on strings and their proper maintenance. If a version is described in an XML payload, it would be a string. If the C++ code needs to do something different it would be based on a string definition. A GT is defined by a string. I think, the latter is sufficient as long as we have correct payloads in the GT (and “exclusive or” in the configuration for now). It seems to me less error-pone and more maintainable. All what we need is a naming policy for the GT that can be used by a dd4hep Era: a dictionary? |
I agree,
Please, see my previous reply to @makortel . Any quick workaround in Python would be welcomed. |
|
@makortel When I put your config fragment into It looks like |
Great detective work! This is one more place that hasn't been ported to DD4hep. We have to introduce an Era there.
IMHO, we want to remove |
|
In addition to the problem of how to handle I'm not sure where such a producer should be put. |
It looks like a miss-configuration that mixes both DDD and DD4hep. |
An easy way would be to define For patching ConfigBuilder, I think it would be better to go with something along if self.geometryDBLabel:
self.executeAndRemember('from Configuration.ProcessModifiers.dd4hep_cff as dd4hep')
self.executeAndRemember('(~dd4hep).toModify(process.XMLFromDBSource, label="%s"'%(self.geometryDBLabel))
self.executeAndRemember('dd4hep.toModify(process.DDDetectorESProducerFromDB, label="%s"'%(self.geometryDBLabel))An alternative would be to parse |
|
@makortel The syntax for these statements is challenging. The syntax from the above comment generates this error: I've been trying several variations, but none work so far. Here's my latest version: This version gives this error: |
|
Dear all, I see, that the discussion is going to deep but we need limit ourselves. Our task for 12_0 is only to provide Run3 WF when we use DD4Hep and DB. In our plans the next step is to make DD4Hep for Phase-2 work with XML geometry, and only the last step make it for Run-1 and Run-2. My impression is that you are discussing the last step, which may be done in the next release. This last step updates may be validation using regression - it is required software configuration which has protections against incorrect custom configuration. THis is true but it may be too much. Let us discuss, if the solution is applicable to Run-3 scenario only and what is needed for 12_0 for Run-3. |
|
I am focusing on a way to get Run 3 DD4hep simulation to read from the DB. We need some fix in |
@civanch - IMHO, it’s almost impossible to run production without correct configuration. BTW, the discussion is not about this PR that has to be merged ASAP. |
|
Let us open a dedicated issue and ask @qliphy to merge this PR. |
|
Dear all, please go ahead to open an issue. If there is no objection, we can merge this PR first. |
|
#34206 is created |
|
+1 |
PR description:
Tidy up the ES producer configuration - drop an optional FileInPath.
StandardSequencesshould use an auto-generatedDDDetectorESProducerFromDBconfiguration fragment for dd4hep Era.@civanch, @cvuosalo - FYI
PR validation:
if this PR is a backport please specify the original PR and why you need to backport that PR:
Before submitting your pull requests, make sure you followed this checklist: