-
Notifications
You must be signed in to change notification settings - Fork 109
Add multithreading support to ddsim #1240
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
base: master
Are you sure you want to change the base?
Conversation
Test Results 9 files 9 suites 1h 40m 30s ⏱️ For more details on these failures, see this check. Results for commit d0ae8cd. ♻️ This comment has been updated with latest results. |
|
This is still a work in progress, but let me give some more background here as to the plan (the "more later" from the top).
It is likely that I will be doing some rewrites of the last commit (5bf711c) which is a bit too big for my taste and not as clear as one would like for non-squash merging. |
|
The status of this PR after #1417 is that we can now run MT gun simulations, like so: Main missing feature is ensuring |
| self._g4gun = None | ||
| self._g4gps = None |
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.
TODO: Geant4 gun and GPS event generators are not initialized correctly due to this removal.
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.
Despite modified implementation, this remain a TODO since only one thread reads the macroFile... (as of da36a3e). And Geant4GeneratorWrapper doesn't allow setting properties when shared = True.
|
The primary remaining difficulty I'm having here is with the event input files (I'm using HepMC3), which we create in: DD4hep/DDG4/python/DDSim/DD4hepSimulation.py Lines 444 to 446 in 6143d12
Simply making the GeneratorAction shared with To get around this, I went through setting up the necessary So, I'm open to input as to how best to have a single @MarkusFrankATcernch @andresailer Do you have any suggestions? |
|
Hi Wouter, This construct is not simple. Let's see what is necessary before the hacking starts... For Now what one probably wants is somehow a structure that this worker thread bound From what I see somehow the input stage needs then to be re-designed. In particular exchanging data between the actions building the queue of Having said all this, it looks necessary that the queue of
How to do something like this in a backwards compatible way is not so obvious to me and should be designed properly. |
|
So after coming back from the drawing board.... to an input stage which looks much rather like this (details are left out here): Here you have
In principle this is not too difficult, but still quite more difficult that you have imagined upfront. In addition:
|
|
While I agree that the diagram you outline (with an event queue) is optimal for throughput performance, I wonder if there is another less optimal but still functional approach (which may have some threads waiting unnecessarily for some of the time). What I had originally anticipated was a pattern where there are multiple Geant4InputActions sequences, one per thread as now, but the action that reads from disk would have a shared reader that is locked. This would mimic the approach of the Geant4OutputAction where each thread has an output event action, but they share and lock the output file. |
|
@wdconinc Where one actually puts the Geant4EventQueue is sort of user-gusto here, but it must be located before the If we invest here, we should do it properly....even if it costs some development. |
See #1440, which exposes the properties correctly now. |
|
@wdconinc @murnanedaniel |
The scope of this PR has always been to make multi-threaded running work with ddsim. I don't know what other ways of running DDG4 people are using, but I'm not familiar with them. |
|
Hi @wdconinc. Thanks for the quick response. I have been trying to reproduce your working setup with input hepmc3 files, but get this error, when setting to 8 threads: I removed the elif python handler test that was causing the recursion, leading to (I believe) the real error, which is this: For context, here is the full output log: full_output.log |
|
Okay, I made a couple of quick tweaks and it does seem to work, which is really promising. Firstly, the tweaks I made are here: murnanedaniel@cbbe86e. I'm not sure if you agree with them, or why they are not necessary in your setup? Secondly, there is one final error appearing after the simulation that doesn't appear in the main DD4hep. You can see it at the end of the successful log file: full_output_mt.log |
|
@wdconinc Just following up here, since we will soon be running a large set of simulations with your MT changes. I just want to ensure that the small error that appears, and the tweaks that I needed to make, are still consistent with the working version of ddsim as you see it. On that same point - have you validated that MT sim of hepmc3 inputs produces more or less identical outputs to DD4hep main? |
Remove elif leading to recursion and change shared check strategy


This PR adds multithreading support to ddsim, through the
-jflag. More later.BEGINRELEASENOTES
ENDRELEASENOTES