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

[R] Check warnings explicitly for model compatibility tests #6114

Merged
merged 2 commits into from
Sep 15, 2020

Conversation

hcho3
Copy link
Collaborator

@hcho3 hcho3 commented Sep 13, 2020

  • Suppress warnings in test_model_compatibility.R, as they clutter test results.
  • Furthermore, check that the correct types of warnings are thrown in test_model_compatibility.R.

@vspinu Can you review?

@codecov-commenter
Copy link

codecov-commenter commented Sep 13, 2020

Codecov Report

Merging #6114 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6114   +/-   ##
=======================================
  Coverage   78.52%   78.52%           
=======================================
  Files          12       12           
  Lines        3069     3069           
=======================================
  Hits         2410     2410           
  Misses        659      659           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1453bee...2c0ba92. Read the comment docs.

Copy link
Member

@trivialfis trivialfis left a comment

Choose a reason for hiding this comment

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

I think we can remove that warning in longer term. Just as an example, if I unify the single_precision_histogram for tree methods, the RDS breaks.

@hcho3
Copy link
Collaborator Author

hcho3 commented Sep 14, 2020

Yes, the warning is meant to encourage users to move away from saveRDS() and use xgb.save() instead. The habit is hard to break, so we produce copious warnings in the user's terminal. Here, we suppress the warnings when running the unit tests.

Copy link
Contributor

@vspinu vspinu left a comment

Choose a reason for hiding this comment

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

I still see some internal warnings. Maybe those could be gotten rid of as well?

⠏ |   0       | update trees in an existing model[10:46:30] WARNING: ../..//amalgamation/../src/gbm/gbtree.cc:81: DANGER AHEAD: You have manually specified `updater` parameter. The `tree_method` parameter will be ignored. Incorrect sequence of updaters will produce undefined behavior. For common uses, we recommend using`tree_method` parameter instead.
⠋ |   1       | update trees in an existing model[10:46:30] WARNING: ../..//amalgamation/../src/gbm/gbtree.cc:81: DANGER AHEAD: You have manually specified `updater` parameter. The `tree_method` parameter will be ignored. Incorrect sequence of updaters will produce undefined behavior. For common uses, we recommend using`tree_method` parameter instead.
[10:46:30] WARNING: ../..//amalgamation/../src/gbm/gbtree.cc:81: DANGER AHEAD: You have manually specified `updater` parameter. The `tree_method` parameter will be ignored. Incorrect sequence of updaters will produce undefined behavior. For common uses, we recommend using`tree_method` parameter instead.
[10:46:30] WARNING: ../..//amalgamation/../src/gbm/gbtree.cc:81: DANGER AHEAD: You have manually specified `updater` parameter. The `tree_method` parameter will be ignored. Incorrect sequence of updaters will produce undefined behavior. For common uses, we recommend using`tree_method` parameter instead.
⠇ |   9       | update trees in an existing model[10:46:30] WARNING: ../..//amalgamation/../src/gbm/gbtree.cc:81: DANGER AHEAD: You have manually specified `updater` parameter. The `tree_method` parameter will be ignored. Incorrect sequence of updaters will produce undefined behavior. For common uses, we recommend using`tree_method` parameter instead.
[10:46:30] WARNING: ../..//amalgamation/../src/gbm/gbtree.cc:81: DANGER AHEAD: You have manually specified `updater` parameter. The `tree_method` parameter will be ignored. Incorrect sequence of updaters will produce undefined behavior. For common uses, we recommend using`tree_method` parameter instead.
✔ |  22       | update trees in an existing model [0.3 s]

Also there is a download message:

⠏ |   0       | Models from previous versions of XGBoost can be loadedtrying URL 'https://xgboost-ci-jenkins-artifacts.s3-us-west-2.amazonaws.com/xgboost_r_model_compatibility_test.zip'
Content type 'application/zip' length 63026 bytes (61 KB)
==================================================
downloaded 61 KB

It can be disabled by quiet = TRUE argument of download.file, (optionally followed with on.exit(unlink(zip_file, force = TRUE)) in order to avoid it showing in git status).

booster <- xgb.load(model_file)
cpp_warning <- capture.output({
# Expect an R warning when a model is loaded from RDS and it was generated by version < 1.1.x
if (is_rds && compareVersion(model_xgb_ver, '1.1.1.1') < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a bit confused by this logic. So for ver >= 1.1.1.1, predict and run_booster_check no longer throw warnings when the model is loaded from RDS? Wouldn't you want to discourage it regardless and throw warning?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's correct. When RDS is used, XGBoost package does not actually know that RDS is used. So the warning is only generated when a material change in the model representation is detected. There was a major change in 1.1.1.1, and a minor one in 1.2.0.1. The major change is covered by the R warning ("The model had been generated by XGBoost version 1.0.0 or earlier and was loaded from a RDS file") and the minor change is covered by the C++ warning ("Attempted to load internal configuration for a model file that was generated by a previous version of XGBoost").

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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') {
Copy link
Contributor

Choose a reason for hiding this comment

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

Version is hardcoded here, so for new models this if-else won't be triggered at all?

Copy link
Collaborator Author

@hcho3 hcho3 Sep 14, 2020

Choose a reason for hiding this comment

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

That's also correct, since no C++ warning is currently generated for the models that were generated by version 1.2.0.1.

@hcho3 hcho3 merged commit 9e955fb into dmlc:master Sep 15, 2020
@hcho3 hcho3 deleted the check_r_warning branch September 21, 2020 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants