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

Fix loading old logit model, helper for converting old pickle. #5281

Merged
merged 10 commits into from
Feb 13, 2020

Conversation

trivialfis
Copy link
Member

@trivialfis trivialfis commented Feb 4, 2020

Continue #5277 . But PR against 1.0.0 branch. Fixes #5276 .

  • Remove saving metric in binary model.
  • Save a JSON string for objective.
  • Save a version string in binary model.
  • Distinguish model before 1.0 and after 1.0.
  • More tests for model compatibility.

@trivialfis
Copy link
Member Author

I will port the related changes to master. PR against release branch so we can run the tests.

@trivialfis
Copy link
Member Author

A batch of models with 0.90:

models.zip

tests/python/test_model_compatibility.py Outdated Show resolved Hide resolved
src/learner.cc Show resolved Hide resolved
@trivialfis
Copy link
Member Author

Phew ...

@trivialfis
Copy link
Member Author

@RAMitchell @hcho3 Please review. ;-)

@hcho3 hcho3 mentioned this pull request Feb 8, 2020
12 tasks
@trivialfis
Copy link
Member Author

trivialfis commented Feb 8, 2020

Wait, still got some test failures on my end.

@trivialfis
Copy link
Member Author

trivialfis commented Feb 8, 2020

@hcho3 I will generate a new batch of models including both 0.90 and 1.0.0. Can we upload it to some where, like Jenkins or maybe github has small data storage (like release meta data)?

@hcho3
Copy link
Collaborator

hcho3 commented Feb 8, 2020

We can put them in an S3 bucket. Jenkins can access all S3 buckets in the same AWS account. Can you send me the models?

@trivialfis
Copy link
Member Author

models.zip

@hcho3
Copy link
Collaborator

hcho3 commented Feb 10, 2020

@trivialfis The zip file is available at s3://xgboost-ci-jenkins-artifacts/xgboost_model_compatibility_test.zip. You can use boto3 package in Python to download the zip file:

import boto3

s3_bucket = boto3.resource('s3').Bucket('xgboost-ci-jenkins-artifacts')
s3_bucket.download_file('xgboost_model_compatibility_test.zip',
                        './xgboost_model_compatibility_test.zip')

Any Jenkins worker should be able to run this line.

@trivialfis
Copy link
Member Author

@hcho3 I updated the test to download the models, please review.

* Remove saving metrics.
* Add version as an attribute.
* Remove the size check in R test to relax the size constraint.
* Add missing R doc for passing linting. Run devtools.
* Cleanup old model IO logic.
* Test compatibility on CI.
Copy link
Collaborator

@hcho3 hcho3 left a comment

Choose a reason for hiding this comment

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

Thanks for adding model compatibility checks. The checks will ensure backward compatibility moving forward.

python-package/xgboost/core.py Outdated Show resolved Hide resolved
src/learner.cc Show resolved Hide resolved
src/learner.cc Show resolved Hide resolved
tests/python/test_basic_models.py Show resolved Hide resolved
tests/python/test_model_compatibility.py Outdated Show resolved Hide resolved
@trivialfis
Copy link
Member Author

A similar issue is also found in treelite. Will try to fix it. It may be worth documenting in release note that everyone who reads the binary model needs to adjust, and it's encouraged to try the experimental JSON support and provide feedbacks.

@trivialfis
Copy link
Member Author

This seems somehow breaks treelite.

@hcho3
Copy link
Collaborator

hcho3 commented Feb 11, 2020

Do you need help with fixing the build?

@trivialfis
Copy link
Member Author

I need to figure out a way to disable this auto include.

Copy link
Member

@RAMitchell RAMitchell left a comment

Choose a reason for hiding this comment

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

LGTM. Hope we don't have to maintain these fixes for long :)

doc/tutorials/saving_model.rst Outdated Show resolved Hide resolved
@mli
Copy link
Member

mli commented Feb 12, 2020

Codecov Report

Merging #5281 into release_1.0.0 will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@              Coverage Diff               @@
##           release_1.0.0    #5281   +/-   ##
==============================================
  Coverage          83.83%   83.83%           
==============================================
  Files                 11       11           
  Lines               2413     2413           
==============================================
  Hits                2023     2023           
  Misses               390      390           

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 5ca21f2...f482c9d. Read the comment docs.

@trivialfis trivialfis merged commit 213f4fa into dmlc:release_1.0.0 Feb 13, 2020
@trivialfis
Copy link
Member Author

Will port it to master branch.

trivialfis added a commit to trivialfis/xgboost that referenced this pull request Feb 13, 2020
trivialfis added a commit that referenced this pull request Feb 13, 2020
* Port test model compatibility.
* Port logit model fix.

#5248
#5281
@trivialfis trivialfis deleted the fix-obj-transform branch February 29, 2020 11:26
@hcho3 hcho3 mentioned this pull request May 25, 2020
@lock lock bot locked as resolved and limited conversation to collaborators Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants