-
-
Notifications
You must be signed in to change notification settings - Fork 307
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
lib/gis: Add a helper function to determine the number of threads for OpenMP #3929
Conversation
This PR failed on four tests that ran
In the test, the args in SimpleModule still go through the parser I modified in this PR. Although OpenMP is not supported, this Python module ( Original: grass/temporal/t.rast.what/testsuite/test_what.py Lines 275 to 301 in 0938951
My questions are:
@HuidaeCho @wenzeslaus @marisn Do you have any suggestions? |
Perhaps the function should be called in the particular C tools, that would avoid the problem with Python tools. Also, there is the NPROCS variable, the function should take into account: |
I checked and found that every C module parallelized by OpenMP has the keyword
The parser runs after options are defined, so the function can also handle an environment variable. |
That's probably not a good strategy, the parallel keyword was meant for any parallelization, not just openmp, we would have to have a new keyword, but I am not sure using a keyword is good idea anyway.
|
That I think is a good approach. Other values require this approach too. RGB colors are one example, but even the current string provided to nprocs needs to be converted to integer. |
I left the parser unchanged and just added a helper function instead. A C module can call this function if it is needed.
Though this PR does not use the keyword now, I think new keywords should be considered. As you said, parallelizations could be done by different libraries. New keywords to indicate which libraries/methods are used can help with documentation and maintenance. |
Agreed. I think this will work well when we add description for each keyword, an extended version of what we have for topics (topic keywords). |
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.
Should the function just return number of threads as an integer?
Can you please provide a breakdown how this will behave with the default value for nprocs and the possibility to change that using g.gisenv or settings in GUI? (Previously mentioned by @petrasovaa)
You are right. I make it return the value now, and it won't change the answer of the nprocs option.
Let's take
I put the helper function in |
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 still lean towards not checking for over-subscription at all, but I'm fine the the PR except couple comments about comments and messages.
I'm totally fine without over-subscription checking. I removed it in the new commit. |
@HuidaeCho Given how you were involved in this PR, please review or remove yourself from the review. Otherwise, this is ready to merge. |
He has addressed all my comments and the PR looks good. Sorry for the delay! |
… OpenMP (OSGeo#3929) * determine the number of threads for OpenMP in opt parser * add OpenMP library paths * solve segfault when nprocs option is not specified in the command * only change nprocs for C module * update the description of the function, which does not return any value * add helper function, left parser unchanged * return threads rather than change the answer of nprocs * modify code based on review comments * remove threads > logic cores check, update based on review comments
… OpenMP (OSGeo#3929) * determine the number of threads for OpenMP in opt parser * add OpenMP library paths * solve segfault when nprocs option is not specified in the command * only change nprocs for C module * update the description of the function, which does not return any value * add helper function, left parser unchanged * return threads rather than change the answer of nprocs * modify code based on review comments * remove threads > logic cores check, update based on review comments
Based on the discussion in #3917, this PR adds a function to
make the parserdetermine the number of threads, mainly for the standard optionG_OPT_M_NPROCS
.