De-duplicate GPU parameters.

[trivialfis]

• Only define gpu_id and n_gpus in Learner TrainParam
• Disable all GPU usage when GPU related parameters are not specified.

[trivialfis]

@RAMitchell It turns out there are some numeric difference between CPU and CUDA transform weren't caught by the tests before. After disabling GPU this test won't pass:

xgboost/tests/python-gpu/test_gpu_prediction.py

Line 13 in be0f346

def test_predict(self):

I tried to diff the outputs from two different settings. The difference mostly come from GetGradient, starts at 1e-6 and accumulates up. To pass the test rtol needs to be changed to 1e-3 from 1e-5. Here is the mismatch rate with 5000x10 data set:

E               AssertionError: 
E               Not equal to tolerance rtol=0.0001, atol=0
E               
E               Mismatch: 0.04%
E                x: array([-0.108287,  0.163954,  0.096447, ..., -0.173721, -0.115342,
E                      -0.577273], dtype=float32)
E                y: array([-0.108287,  0.163954,  0.096447, ..., -0.173721, -0.115342,
E                      -0.577273], dtype=float32)

[trivialfis]

Another issue is, I'm currently hacking the configuration in Learner TrainParam, honestly I don't fully understand what's happening in there.

[trivialfis]

One last thing to do is either delay the configuration of Objective to after data is known, or resize the label checking result vector every time GetGradient is called.

[RAMitchell]

So in summary this PR provides the GPUSet::Global() method, where any algorithm can simply fetch the global configuration of GPUs instead of manually reading parameters and configuring this. This is nice because removes quite a bit of code from other algorithms while still allowing them to manually specify this if necessary.

This PR solves the problem where GPUs were being used when the user expects only CPU algorithms. This was happening in the objective functions due to the default parameter of n_gpus=1. This is solved by pushing the configuration into the learner, which has more global knowledge about whether CPU or GPU algorithms should be used.

The danger here is mostly in the very difficult to understand configurations happening in the learner, but this is no different to existing code e.g. where we manually configure updater parameters based on tree_method. It would be good to make sure the behaviour is expected upon serialisation/deserialisation of the model.

I like the way you have named functions in the learner to more explicitly describe the configuration that is happening. I think this area of code can be improved a lot in future.

One feature that would be nice to have is to log information (at some verbosity level) about GPUs selected upon initialisation of the global singleton.

Looks good to me, although I'm not sure if it should go in 0.9. This could be considered a bug fix.

@canonizer can I get a review from you as well please.

[trivialfis]

it would be good to make sure the behaviour is expected upon serialisation/deserialisation of the model.

Good point, will try to make a test later.

One feature that would be nice to have is to log information

Will add a LOG(INFO) in GPUSet::Init.

[trivialfis]

@RAMitchell Actually I don't want to merge this PR. I remember there were users who train different models on different GPUs. Making a global variable will break it. I need to think of something that doesn't break the functionality but also keep our implementation clean.

[trivialfis]

Suggestions are welcomed.

[trivialfis]

@hcho3 @RAMitchell I have a local branch that passes a pointer to const LearnerTrainParam to Predictor, Metric, TreeUpdater, GBM and LinearUpdater via Create method in respective class. The change is quite massive and I would like to hear about your opinions before proceeding.

The benefit of passing LearnerTrainParam is we can eliminate the duplication of gpu_id, n_gpus without creating a global variable. As another benefit, nthread is also passed around, creating an opportunity of eliminating global OpenMP variable.

The downside is the restructuring is massive, and all Create methods need to accept an addition parameter.

[trivialfis]

Also, this is a blocker for JSon.

[hcho3]

Current structure of parameter handling causes duplication, as learning parameters are duplicated into objectives, metrics, and updaters. So your proposal has merits. Let me think over this and get back to you.

[trivialfis]

@hcho3 It also fixes the bug that XGBoost choosing GPU aggressively due to lack of global configuration. Currently if the user don't supply n_gpus=0 parameter XGBoost will run metrics and objectives on GPU by default.

[canonizer]

Could you use #define NGPUS 1, without (), as there are no parameters?

[hcho3]

Could you use #define NGPUS 1, without (), as there are no parameters?

@trivialfis Second this comment.

[trivialfis]

@hcho3 Could you please help taking a look in Jenkins' cache? It seems clang-tidy is running on an old copy of this PR: https://xgboost-ci.net/blue/organizations/jenkins/xgboost/detail/PR-4454/10/pipeline

In latest commit gpu_predictor.cu:376:3 is not a constructor.

[RAMitchell]

I like your solution. Agree with the others that a const shared pointer is much better than a raw pointer.

[trivialfis]

@RAMitchell @canonizer Could you please explain why shared pointer is better?

From my point of view, objects like Predictor do not own LearnerTrainParam, so its "borrowing" a pointer from Learner. That's like when you want to access the internal data of std::vector by calling data(), or std::string by calling c_str(), you get a raw (const) pointer. With the returned raw pointer you are only borrowing its content, but should not manage the content of the pointer (deallocate) because the ownership is still in std::string/std::vector.

Passing shared_ptr means the ownership is shared. It's a semantic issue. See
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-smart
https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rr-ptr
and related topics.

I'm open to suggestions, but I need more context to understand why shared pointer is a better choice.

[trivialfis]

@hcho3

Could you please help taking a look in Jenkins' cache?

Never mind, it works. Thanks.

[trivialfis]

Summary

Since this PR does a lots of things, here is a summary for @hcho3 when the release after 0.90 hit.

This PR unifies the two parameters gpu_id and n_gpus as part of LearnerTrainParam. Other than code maintenance, the unification will fix the behaviour that XGBoost choosing GPU aggressively. Previously XGBoost will run metrics and objectives on single GPU by default due to lack of global configuration. After this PR, Learner will be able to choose between CPU/GPU based on training algorithm and predictor. Also, clearing out the duplication will get us one step closer to JSON RFC. In future we might be able to use nthreads from this parameters set to eliminate the use of OpenMP's global variable.

To implement such configuration, I added an extra parameter learner_param_ const* in every factory Create method and stored a LearnTrainParam const * in base class. The only one instance of LearnerTrainParam is stored in Learner class (not LearnerImpl). Since during deallocation, Learner is the last one to go (after Predictor, Metric ...) so there's no need for extra memory management for this parameter. I chose raw pointer over smart pointer, which is still in discussion with @canonizer and @RAMitchell .

Also, this PR fixes pickling issue when model is trained on GPU but later loaded on CPU-only devices.

Added tests for Learner should show the correctness of configuration.

[trivialfis]

closes #4494
closes #4361

[hcho3]

@trivialfis Thanks for fixing the pickling issue

[pseudotensor]

@hcho3 Yes, this works great. Please merge! :)

[hcho3]

@pseudotensor Thanks for trying it out. I will review this PR and approve it today.

[hcho3]

LGTM overall. I have some comments about style.

[pseudotensor]

ModuleNotFoundError: No module named 'xgboost.training'
That's just from testing, not me.

[hcho3]

@pseudotensor Can we have a repro script?

[hcho3]

@trivialfis Currently, Python tests on Win64 are not idempotent, as they attempt to install and remove XGBoost package into the system Python environment. So running more than one jobs in the same machine will cause problems (one job installs XGBoost and another removes it). For now, I limited the number of jobs to 1 for Windows workers (and restarted tests), but I'd like to make Win64 tests idempotent in a follow-up PR.

[pseudotensor]

@pseudotensor Can we have a repro script?

That's just from the CI, seems fixed.

[trivialfis]

@pseudotensor sounds like the reason described by @hcho3 .

[hcho3]

Thanks for clarification. I will follow up with a PR to stabilize Windows CI