-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
[R-package] Temporary workaround for not modifying global OMP config #5134
Conversation
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.
Thanks for your proposal, but I don't support introducing a new required dependency to the R package for this purpose.
Instead of this fix that benefits only the R package, and even then only one part of it (i.e. not other multi-threaded operations like prediction and Dataset
construction), if you're interested in seeing LightGBM's behavior here change, please work with @guolinke and @shiyu1994 on #4705 to implement something at the C++ level that all users of LightGBM will benefit from.
If you mean #4705 (comment)
I believe that was a recommendation for how LightGBM's C++ code would manage getting and setting the the number of threads, not a recommendation that every wrapper of that code do this itself. |
I agree that such changes should be made in C++ so that both R and Python packages can leverage the workaround. I can help to do that. |
Thanks @shiyu1994 . For the benefit of everyone reading this PR, I just want to clarify...when I refer to wrappers of LightGBM's C++ code, I don't only mean the R and Python package in this repo. I also mean SynapseML (formerly MMLSpark), the Java interface generated with SWIG, Based on this discussion, I'm going to close this PR. @david-cortes thanks very much for documenting #4705 and for helping this project try to address it! |
This pull request has been automatically locked since there has not been any recent activity since it was closed. To start a new related discussion, open a new issue at https://github.com/microsoft/LightGBM/issues including a reference to this. |
ref #4705
This is a temporary workaround to avoid modifying the global OpenMP user configuration for the whole R process when running
lgb.train
. This PR should be reverted once issue #4705 is fixed properly.