Passing default num_threads to booster.predict not working

[yinweisu]

Description

Passing num_threads=-1, or 0 or None to booster.predict method will not take effect. Instead, it will continue to use num_threads specified by the train method. Currently, the workaround I found is passing a huge value, i.e. 999.

One can pass the number of cores as an argument themselves of course. However, user should be able to provide a default value and lgb should auto use all the cores

Reproducible example

import lightgbm as lgb
import numpy as np

params = {'num_threads': 1}
data = np.random.rand(50_000_000, 10)
label = np.random.randint(2, size=50_000_000)
train_data = lgb.Dataset(data, label=label)
test_data = np.random.rand(50_000_000, 10)
num_round = 10

model = lgb.train(params, train_data, num_round)
model.predict(test_data, num_threads=0) # will still use one thread

Environment info

LightGBM version or commit hash: 3.2.1

Command(s) you used to install LightGBM

pip install lightgbm

Additional Comments

[StrikerRUS]

@yinweisu Thanks for raising this issue!
I can reproduce it on my Windows machine. Indeed, switching from any particular number of threads used in training to the default number of threads in OpenMP (0) is not working. Setting number of threads in prediction to any fixed value works fine, though.

[yinweisu]

@StrikerRUS Thanks for confirming the issue!
Yes, so far the workaround would be passing a large value. However, I think this is not a good practice ;)

[yinweisu]

Some updates about this issue: I noticed that setting num_threads equal to a very large number, i.e. 999, would cause mem leak

[StrikerRUS]

Hmm, I guess this can be a separate bug. How did you notice memory leaks? We'd really appreciate a new issue with small reproducible example like one you've used in your original comment here.

[shiyu1994]

The problem is rooted here,

LightGBM/src/c_api.cpp

Lines 1807 to 1821 in a77260f

	int LGBM_BoosterPredictForFile(BoosterHandle handle,
	const char* data_filename,
	int data_has_header,
	int predict_type,
	int start_iteration,
	int num_iteration,
	const char* parameter,
	const char* result_filename) {
	API_BEGIN();
	auto param = Config::Str2Map(parameter);
	Config config;
	config.Set(param);
	if (config.num_threads > 0) {
	omp_set_num_threads(config.num_threads);
	}

when num_threads <= 0, it is being ignored and the current setting of number of threads in OpenMP is kept.

Just wondering is there a way to find the original default number of threads used by OpenMP, except query the environment variable OMP_NUM_THREADS? If there is, then we can fix this bug by resetting the default number of threads when num_threads <= 0 in configuration parameters.

[hzy46]

I did some investigation on this topic. OpenMP has the following 4 ways to determine the number of the thread it uses:

1. num_threads in the pragma omp parallel num_threads(...)
2. omp_set_num_threads(...)
3. The environment variable OMP_NUM_THREADS
4. Compiler's default setting (usually equal to the number of the CPU cores)

The priority of these ways is 1 > 2 > 3 > 4.

As for this issue, the main cause is: We first call omp_set_num_threads(...) to set the thread number during training. Then, omp_set_num_threads will not be called during inferencing. So it will follow the previous setting.

It can be also reproduced by: 1. set num_threads=3 for training 2. ctrl+c to stop the training 3. set num_threads=0 for training (The code will still use 3 as the thread number).

I submit a PR #4704 which records the default thread setting and reset the thread number back when num_threads <= 0. It has a small side-effect: If there's any change on environment variable OMP_NUM_THREADS after the first run, the change will not be respected. I'm not sure whether we should take it into consideration.

[shiyu1994]

@hzy46 Great, I think this is a satisfactory solution. Thanks! Let's set aside the change of OMP_NUM_THREADS between two runs outside this PR.

[StrikerRUS]

It has a small side-effect: If there's any change on environment variable OMP_NUM_THREADS after the first run, the change will not be respected.

I believe we should at least document this clearly. scikit-learn suggests this way of changing number of threads in their docs. So I think users might expect the same (default) behavior from LightGBM as well.
https://scikit-learn.org/stable/computing/parallelism.html?highlight=omp_num_threads#openmp-based-parallelism

[StrikerRUS]

Cross-linking the following issue here: #4705.

[david-cortes]

There's actually more issues coming from this OMP usage. Ideally, the lightgbm functions should never set the number of global threads through omp_set_num_threads, since that has bad side effects extending outside of lightgbm (for example, if one uses openblas built with openmp through numpy, it means that the threads for matrix multiplications in numpy will follow what lightgbm set as its threads).

The official scikit-learn docs say that, for scikit-learn estimators, OMP-based parallelization should follow the environment variable for getting the number of threads, but I think such recommendation is counterintuive and unergonomic (i.e. most users would expect to be able to control the number of threads from n_jobs) and this wouldn't be the only package to ignore such recommendation (and I hope scikit-learn will also change such flawed logic in the future).

It would be better to have the number of threads set for each OMP pragma separately:

#pragma omp parallel for num_thread(my_num_threads)

(this would require changing many function signatures though)

However, that will not ignore invalid values such as 0 - these have to be manually checked beforehand and set to a minimum of 1 or so, or it could alternatively be made throw an exception or a warning in such cases. In the python package, additionally, I think it would be expected that negative numbers of threads follow the joblib formula:

nthreads_final = available_threads + 1 + neg_nthreads

(with an additional check that the number that's obtained from that is still valid)

From a design POV, it'd additional be helpful to be able to set the number of threads in a fitted model object.

(And BTW while you're at it, please don't use const int num_threads in the pragma as that has issues with some compilers and will trigger CRAN errors)

[shiyu1994]

@david-cortes Thanks. I agree. Calling omp_set_num_threads within lightgbm will unexpectedly change the number of threads used in other parts of the program with OpenMP. However removing this usage requires more effort. And maybe we can merge #4704 when it is ready and leave the removing of omp_set_num_threads in later PRs.

[StrikerRUS]

I just checked and can confirm that #4704 fixed bug from the original comment. But #4705 remains open.

[github-actions]

This issue 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.