DD4hep: Sim Application Based on DD4hep #26823
Conversation
|
The code-checks are being triggered in jenkins. |
|
-code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26823/9844
Code check has found code style and quality issues which could be resolved by applying following patch(s)
|
|
The code-checks are being triggered in jenkins. |
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-26823/9858
|
|
A new Pull Request was created by @ianna (Ianna Osborne) for master. It involves the following packages: Configuration/Geometry @civanch, @Dr15Jones, @cvuosalo, @ianna, @mdhildreth, @cmsbuild, @franzoni, @kpedro88, @fabiocos, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
The tests are being triggered in jenkins. |
|
-1 Tested at: 476e437 You can see the results of the tests here: I found follow errors while testing this PR Failed tests: ClangBuild
I found compilation warning while trying to compile with clang. Command used: See details on the summary page. |
|
Comparison not run due to Build errors/Fireworks only changes/No short matrix requested (RelVals and Igprof tests were also skipped) |
@ianna the second statement does not follow from the first statement. |
|
I agree - migrations should be supported in cmssw
(but this does imply some prior agreement on the design, end-state and timescale...)
… On May 21, 2019, at 5:49 PM, Ianna Osborne ***@***.***> wrote:
I'm not the only one who is working on the migration, so maintaining a separate test package is not an option.
@ianna the second statement does not follow from the first statement.
yes, it does. We need to make it easier for developers to contribute and not make it an exercise in DevOps where they would need to checkout and recompile the whole CMSSW on a regular basis:
***@***.*** src]$ ls
Alignment CondFormats EgammaAnalysis HLTriggerOffline L1Trigger RecoEcal RecoPixelVertexing SimMuon
AnalysisDataFormats CondTools ElectroWeakAnalysis HeavyFlavorAnalysis L1TriggerConfig RecoEgamma RecoTauTag SimPPS
CalibCalorimetry Configuration EventFilter HeterogeneousCore L1TriggerOffline RecoHI RecoTracker SimTracker
CalibMuon DPGAnalysis FWCore HiggsAnalysis MagneticField RecoJets RecoVertex SimTransport
CalibPPS DQM FastSimulation IOMC MuonAnalysis RecoLocalCalo SUSYBSMAnalysis TauAnalysis
CalibTracker DQMOffline Fireworks IOPool PerfTools RecoMET SimDataFormats TopQuarkAnalysis
Calibration DQMServices GeneratorInterface IORawData PhysicsTools RecoMTD SimG4CMS TrackingTools
CommonTools DataFormats Geometry IgTools QCDAnalysis RecoMuon SimG4Core Utilities
CondCore DetectorDescription HLTrigger JetMETCorrections RecoBTag RecoParticleFlow SimGeneral Validation
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Here's the checkdeps from the latest test of this PR: The small changes in other packages from this PR could be included in CMSSW without a problem. My concern is the several thousand lines of duplicated sim code. There's no reason for this to entire the official repository. |
|
@kpedro88 and @smuzaffar - I'd suggest to branch a separate IB for DD4hep migration. |
|
@ianna I remain unconvinced of the necessity of anything beyond a separate test repo (or equivalently a private CMSSW branch with the duplicated code in the |
|
@ianna, I would suggest schedule discussion at the next SIM meeting or offline. The ideal solution would be not a separate branch - for me no difference if it is a separate branch or a separate sub-package. Why two different DDs may be called from RunManager? because it is only one connection point. Geometry should be defined during initialisation, in run time, SIM does not make any call to DD. So, the task is only to initialise things properly for one or another DD. |
|
@kpedro88 - we have to duplicate the code to decouple current production version from a dev version to run fair comparison. |
|
@ianna I understand that duplicating the code helps you iterate faster in the testing and migration. I just don't see a reason that the duplicated code has to be included in the official CMSSW (until the time when it is ready to enter production and the changes are moved to the original code). |
|
Comparison is ready Comparison Summary:
|
|
A separate IB for DD4hep migration would appear to be a good compromise. |
|
This makes the eventual validation and switchover quite complicated. Better to be able to use both systems together.
On 21 May 2019, at 22:23, Ianna Osborne <notifications@github.com<mailto:notifications@github.com>> wrote:
@ianna<https://github.com/ianna> I understand that duplicating the code helps you iterate faster in the testing and migration. I just don't see a reason that the duplicated code has to be included in the official CMSSW (until the time when it is ready to enter production and the changes are moved to the original code).
What you suggest is similar to integration of other packages/versions such as ROOT, G4, etc. in a separate IB thread. It does make sense. It has never been agreed to support both DD and DD4hep. The plan has always been to migrate to DD4hep, validate it and drop DD.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub<#26823?email_source=notifications&email_token=ABGPFQ6EVYM64G5H6LR2KTTPWRK4FA5CNFSM4HNV5XE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODV5CKII#issuecomment-494544161>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABGPFQ3XOO2542XWWYLWAPDPWRK4FANCNFSM4HNV5XEQ>.
|
|
The difference is that 100s of CMSSW packages depend on ROOT, while 0 packages depend on SimG4Core/DD4hepGeometry. I am not just making a philosophical point here. I have done the entire GeantV integration and testing using a separate test repo https://github.com/kpedro88/SimGVCore. This affords the flexibility to try things and fix bugs without being tied to the CMSSW development cycle and maintenance requirements. Eventually, the final product can be brought over in a minimally intrusive way. |
|
It is a recommendation for what will be, in the end, the least burdensome way to proceed with testing migrated interfaces. I have no power to assign a cat A to do anything, nor do I think it would be necessary in this case. (What maintenance does a test repo require?) If there is some reason that the approach I suggest will not work or will cause undesirable difficulty, it has not been communicated in this conversation in a way that I can understand. |
|
+1 |
|
@ianna , SIM is using GEANT4 special branch for rolling tests of new Geant4 versions. It forbidden to make commits and PRs to this branch of CMSSW. This is a useful constrain to avoid diversity in software development. Concerning DD: Geant4 geometry is static, in sense that it is created once at initialisation and changed in run time of SIM step. There are geometry description itself, materials, regions, production cuts, and sensitive detectors. The geometry is complex. Other SIM data are trivial, moreover, these additional properties should be added after the geometry is built, except materials, which likely are filled already with geometry. We have ~300 materials, ~25 regions and production cuts, ~10 sensitive detectors. These data are 1-dimensional data structures. For example, production cut requires from 1 to 4 double for each object. Total memory for these simple objects is negligible. The requirement to the parser is to create this small data structures (shared between threads). The code to assign logical volumes to these data exist and is correct, no sense rewrite it, you need only take part from CompactView and add to the new equivalent class. Using template it is easy to switch between CompactView and new equivalent class. Only this place should be changed in SIM packages - it is basically few lines of code. |
|
@civanch - I'd like to remove |
|
@ianna , please do this, it is more correct to move this class to SimG4Core/Geometry, do not forget to change SimG4Core/Application/RunManager* lenks to headers. |
|
Most of the functionality of this PR except for the sim application itself has been integrated in the IB. The latter is under discussion and will be submitted as a separate PR. |
PR description:
Note: the code contains commented out fragments from original classes and it will be cleaned up when an equivalent functionality is provided. I'm submitting the code in order to migrate the developers area from 10_6 to 11.
PR validation:
The altered GEN_SIM step1 of a 10042.0 workflow runs fine(*) as follows. The events are empty due to an absence of sensitive volumes.
*) if biglibs are disabled.
if this PR is a backport please specify the original PR:
no back port is needed