From d2bc0f0f08d2b9e1040cc440dfce1a88281ff7c3 Mon Sep 17 00:00:00 2001 From: Philip Hyunsu Cho Date: Fri, 6 May 2022 22:49:38 -0700 Subject: [PATCH] Allow loading old models from RDS (#7864) --- .../tests/testthat/test_model_compatibility.R | 17 ++++++++--------- src/learner.cc | 8 +++++++- src/tree/updater_quantile_hist.h | 18 +----------------- 3 files changed, 16 insertions(+), 27 deletions(-) diff --git a/R-package/tests/testthat/test_model_compatibility.R b/R-package/tests/testthat/test_model_compatibility.R index 0f13bdc73..6a35ca4b3 100644 --- a/R-package/tests/testthat/test_model_compatibility.R +++ b/R-package/tests/testthat/test_model_compatibility.R @@ -77,6 +77,7 @@ test_that("Models from previous versions of XGBoost can be loaded", { model_xgb_ver <- m[2] name <- m[3] is_rds <- endsWith(model_file, '.rds') + is_json <- endsWith(model_file, '.json') cpp_warning <- capture.output({ # Expect an R warning when a model is loaded from RDS and it was generated by version < 1.1.x @@ -95,15 +96,13 @@ test_that("Models from previous versions of XGBoost can be loaded", { run_booster_check(booster, name) } }) - if (compareVersion(model_xgb_ver, '1.0.0.0') < 0) { - # Expect a C++ warning when a model was generated in version < 1.0.x - m <- grepl(paste0('.*Loading model from XGBoost < 1\\.0\\.0, consider saving it again for ', - 'improved compatibility.*'), cpp_warning, perl = TRUE) - expect_true(length(m) > 0 && all(m)) - } else if (is_rds && model_xgb_ver == '1.1.1.1') { - # Expect a C++ warning when a model is loaded from RDS and it was generated by version 1.1.x - m <- grepl(paste0('.*Attempted to load internal configuration for a model file that was ', - 'generated by a previous version of XGBoost.*'), cpp_warning, perl = TRUE) + cpp_warning <- paste0(cpp_warning, collapse = ' ') + if (is_rds && compareVersion(model_xgb_ver, '1.1.1.1') >= 0) { + # Expect a C++ warning when a model is loaded from RDS and it was generated by old XGBoost` + m <- grepl(paste0('.*If you are loading a serialized model ', + '\\(like pickle in Python, RDS in R\\).*', + 'for more details about differences between ', + 'saving model and serializing.*'), cpp_warning, perl = TRUE) expect_true(length(m) > 0 && all(m)) } }) diff --git a/src/learner.cc b/src/learner.cc index 20b342b7b..568cfc680 100644 --- a/src/learner.cc +++ b/src/learner.cc @@ -406,8 +406,14 @@ class LearnerConfiguration : public Learner { } void LoadConfig(Json const& in) override { + // If configuration is loaded, ensure that the model came from the same version CHECK(IsA(in)); - Version::Load(in); + auto origin_version = Version::Load(in); + + if (!Version::Same(origin_version)) { + LOG(WARNING) << ModelMsg(); + return; // skip configuration if version is not matched + } auto const& learner_parameters = get(in["learner"]); FromJson(learner_parameters.at("learner_train_param"), &tparam_); diff --git a/src/tree/updater_quantile_hist.h b/src/tree/updater_quantile_hist.h index a0cf4906e..18b4d6682 100644 --- a/src/tree/updater_quantile_hist.h +++ b/src/tree/updater_quantile_hist.h @@ -249,23 +249,7 @@ class QuantileHistMaker: public TreeUpdater { void LoadConfig(Json const& in) override { auto const& config = get(in); FromJson(config.at("train_param"), &this->param_); - try { - FromJson(config.at("cpu_hist_train_param"), &this->hist_maker_param_); - } catch (std::out_of_range&) { - // XGBoost model is from 1.1.x, so 'cpu_hist_train_param' is missing. - // We add this compatibility check because it's just recently that we (developers) began - // persuade R users away from using saveRDS() for model serialization. Hopefully, one day, - // everyone will be using xgb.save(). - LOG(WARNING) - << "Attempted to load internal configuration for a model file that was generated " - << "by a previous version of XGBoost. A likely cause for this warning is that the model " - << "was saved with saveRDS() in R or pickle.dump() in Python. We strongly ADVISE AGAINST " - << "using saveRDS() or pickle.dump() so that the model remains accessible in current and " - << "upcoming XGBoost releases. Please use xgb.save() instead to preserve models for the " - << "long term. For more details and explanation, see " - << "https://xgboost.readthedocs.io/en/latest/tutorials/saving_model.html"; - this->hist_maker_param_.UpdateAllowUnknown(Args{}); - } + FromJson(config.at("cpu_hist_train_param"), &this->hist_maker_param_); } void SaveConfig(Json* p_out) const override { auto& out = *p_out;