-
Notifications
You must be signed in to change notification settings - Fork 100
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
Add multithreading support to ddsim #1240
base: master
Are you sure you want to change the base?
Conversation
Test Results 4 files 4 suites 47m 27s ⏱️ For more details on these failures, see this check. Results for commit 5bf711c. |
@@ -447,8 +393,6 @@ def run(self): | |||
self._buildInputStage(geant4, actionList, output_level=self.output.inputStage, | |||
have_mctruth=self._enablePrimaryHandler()) | |||
|
|||
# ================================================================================================ |
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.
Can you limit to necessary changes only, no gratuitous beautyfications please?
I am not sure if things are re-arranged a lot or diff
gets confused.
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.
I was trying to :-) The main reason for rearranging is that the worker actions needed to be separated from the master actions, as previously they were intermingled. That required some necessary rearranging.
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.
One way to help diff out could be to define helper functions in the order where things are in the file right now, and then use those helper functions in some new code that simply calls them (like __setupWorker
). E.g. the UI setup can be put in a function at its current place in the file.
# ================================================================================= | ||
return 1 | ||
|
||
def __setupWorker(geant4, sim): |
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.
As is this is still an instance method.
def __setupWorker(geant4, sim): | |
def __setupWorker(self, geant4) |
and accordingly below? Or why would this not work?
Then you either need to move out of the class or add the @staticmethod
decorator.
And is the return value required by the interface?
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.
I can (and should) likely make this work with @staticmethod
. The return value appears to be unused. I based the structure on what is done in SiDSim_MT.py
, to the extent it provides any prior art.
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.
(I was unable to make this work with an instance method since I suspect that by the time __setupWorker
is called, the DD4hepSimulation object is not available to the thread that calls __setupWorker
.)
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.
But according to the indentation, this still is an instance method? Just because the first argument isn't self
doesn't change that, does it?
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. |
This PR adds multithreading support to ddsim, through the
-j
flag. More later.BEGINRELEASENOTES
ENDRELEASENOTES