-
Notifications
You must be signed in to change notification settings - Fork 58
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
threadpoolexecutor for distributing MPI #230
threadpoolexecutor for distributing MPI #230
Conversation
if one runs |
@tylerbarna That's a good question and I didn't had this case in my mind. When |
looking through some stackoverflow pages, concurrent.futures might already handle this (see here), but I've seen some conflicting info. I've also seen some things along the following lines that explicitly use concurrent.futures.Queue():
but I haven't experimented much with it. There's also multiprocessing,queue |
@tylerbarna I have indeed used the |
y
Conflicting info regarding whether or not submitting more jobs than the max number of workers handles the queueing automatically |
@tylerbarna Okay. We can try to implement along these lines. Maybe, defaulting to 1 process per configuration if no. of config > no. of process mentioned. Or making it strict policy for the no of processes to be an integer multiple of the no of configurations, if analysis needs to be in parallel. Also try to implement a queue based execution, as what you have asked here #230 (comment) @mcoughlin @tsunhopang Any comments regarding these? |
yeah, the former option seems like it would be best. There can be situations where we might have a larger number of jobs that we want to fit than number of cores we can allocate to one job in a cluster, but being able to still parallelize using the cores we have is good |
@sahiljhawar I have no particularly strong preference, whatever works for folks. |
|
b4ac52a
to
ca98d7d
Compare
could u add a small test for this executable? E.g. Run on At2017gfo with two models |
@tsunhopang yeah, I am trying to do that but there seems to be some issues with config file |
@tsunhopang this is working now. can you review? |
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.
Looks good to me
The test and code actually works but sometimes it is cancelled/exits due to Github runners limited resources. |
3b0b3b0
into
nuclear-multimessenger-astronomy:main
This PR more closely follows the idea given in #215, repeatedely calling
light_curve_analysis
within the script. This parallelisation is free from all the issues faced in #229. Multiple methods are implemented to leverage the parallelisation based on user preference.List of commands with explanation:
multi_config_analysis --config config.yaml --parallel --process 20
: This runs all the configurations in parallel by dividing 20 processes equally amongst the configs. Everything runs in parallel.multi_config_analysis --config config.yaml --process 20
: This runs all the configurations in serial but each configuration use 20 processes one after another.multi_config_analysis --config config.yaml --parallel
: If--process-per-config
is given in yaml (if given, should be given individually to all the configs), then those many numbers of processes are assigned to each configuration. Everything runs in parallel.Things to note:
--process
is strictly required if--process-per-config
is not given--process-per-config
takes precedence over--process
injection.log
now works as it is expected to; logs from concurrent runs do not leak.