Skip to content
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

Keep number of threads in a global variable separate from global OMP config #6152

Closed
wants to merge 1 commit into from

Conversation

david-cortes
Copy link
Contributor

ref #4705
ref #6133

This PR changes the recently introduced workaround for getting the number of threads to be a global variable specific to lightgbm instead of modifying the global openmp configuration.

This is a quick workaround (compared to using a variable local to a function call) and doesn't solve the problem for good - for example, if one calls fit in multiple threads, the configs will overwrite each other, but at least they won't conflict with those of other software.

Ideally this new global variable should be thread-local but I see that lightgbm is also compiled with MSVC and I'm not sure if omp's thread-local pragmas are supported for that compiler so I left them out.

Copy link
Collaborator

@jameslamb jameslamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I generally support this pattern, but leaving a blocking review because I'd like the opportunity to test this.

I have also been working on a similar approach (+ a more permanent solution to LightGBM's various multi-threading issues you've linked) over in my fork, just haven't had time to move it forward as I've been traveling for the last two weeks.

@@ -17,20 +17,17 @@
#include <stdexcept>
#include <vector>

static int lgb_global_omp_threads = omp_get_max_threads();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been testing an approach like this over on my fork (jameslamb#55) and I don't think just having static int in a header here will work.

I found that when I did that, everything that included openmp_wrapper.h got its own copy of that integer, and that therefore updates like this:

OMP_SET_NUM_THREADS(config_.num_threads);

did not propagate through everywhere.

@jameslamb
Copy link
Collaborator

Closing this, as it was superseded by #6226.

Sorry to do that, but I think it was necessary given the time pressure associated with needing to do a new release to CRAN before they archived {lightgbm} (#6221).

Thanks as always for your help with LightGBM, and for documenting these issues with thread control in #4705 and other discussions.

@jameslamb jameslamb closed this Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants