-
Notifications
You must be signed in to change notification settings - Fork 139
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
Can we remove nprocs
from ice_in
?
#945
Comments
I've considered this a number of times, but have put off a change. I guess there are a few issues. First, is there ever a case where nprocs (MPI_COMM_SIZE) is not the number of process you want to run on? Could the CICE model be part of a multi-model system where somehow the MPI communicator doesn't match the number of processors the ice model should run on? Second, this feature provides a check. Even in a standalone mode, the size of the communicator is going to depend on how the model was launched (via mpiexec or similar). Is it helpful to have an internal check that the number of processors specified by the user in namelist actually matches the number of processors in the communicator, especially since block size and many other things will depend on the number of processors in play? Does the fact that MPI and OpenMP are supported together affect the usefulness of this feature? Having said all that, I'm open to changing this. In that case, I assume you're setting the block size and the max blocks will accommodate a range of processor counts. And if the max blocks is too big, you just live with it (or maybe max blocks is set internally?). Maybe I'll suggest adding support for setting nprocs=-1, which would mean ignore it and use get_num_procs instead. I think that provides backwards compatibility and support for cases where it may be useful, but allows users to rely on get_num_procs if they want. |
There's many possibilities - but the code aborts if nprocs != get_num_procs, so any possibility that uses nprocs from the namelist as different to the system number of processors would need code modifications anyway. We think we can improve the automated max_blocks estimate too, but it requires calling p.s. Your suggestion makes sense - it's hard to know, we are guessing about what some possible use cases are. p.p.s We discovered setting max_blocks too big gives memory problems, and the automated setting of max_blocks wasn't working because
|
The max_blocks setting is also problematic. I've looked at that too. The current estimate when max_blocks=-1 is not correct for some decompositions. It would be nice for the code to set it automatically for the grid, pe count, and decomposition properly. My assessment is the same, it would require some serious code changes to compute the max_blocks correctly. But it's possible. Maybe I'll look at that again. In my experience, setting max_blocks too large doesn't affect performance, but, of coarse, it does use memory. I will implement the "nprocs=-1" option. Part of that will be to move the "nprocs=get_num_procs()" to before nprocs is used, will make sure it'll work for CESMCOUPLED too. I'll look at the max_block calc too again. |
I've done some work on it already, I can make a PR and you can review / edit ? |
Sure, would love to see what you've got. I've got a sandbox going to. Trying to see if I can get the max_blocks computed on the fly such that each task has the appropriate value. I think I'm close with a reasonable set of modifications. Feel free to create a PR or point me to your branch. Thanks! |
#949 makes nprocs optional. I will close this issue now. |
In this line, all that happens is we check
nprocs
from the namelist matches the results fromget_num_procs()
:CICE/cicecore/cicedyn/infrastructure/ice_domain.F90
Line 252 in 29c7bcf
Can we just assign
nprocs = get_num_procs()
?We would have to move it a little bit earlier in the code, before:
CICE/cicecore/cicedyn/infrastructure/ice_domain.F90
Line 207 in 29c7bcf
This would make model configuration a little easier (i.e. one less configuration to have to match up in multiple places).
The text was updated successfully, but these errors were encountered: