Skip to content

Conversation

@ezhdi
Copy link
Contributor

@ezhdi ezhdi commented Aug 1, 2021

Extend the existing XGBoost integration (dvc.xgb) to include model saving functionality.

Closes #109

ezhdi added 2 commits August 1, 2021 22:48
Add save model
Test model saving
@daavoo daavoo self-requested a review August 2, 2021 06:45
Copy link
Contributor

@daavoo daavoo left a comment

Choose a reason for hiding this comment

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

Hi @ezhdi ! Thanks for the contribution.

It looks like there are some problems with the code-check workflow:

https://github.com/iterative/dvclive/pull/131/checks?check_run_id=3217954053

You can set up pre-commit in your local environment to check those problems (and potentially fix some automatically):

pip install pre-commit
pre-commit install
pre-commit run --all-files

(See more in https://github.com/iterative/dvclive/blob/master/CONTRIBUTING.md#development-environment)

It also looks like the code added in the test is not correct:

>       preds = xgb.predict(iris_data)
E       AttributeError: module 'xgboost' has no attribute 'predict'

I think you should change the line to use the model returned from xgb.train:

    model = xgb.train(
        train_params,
        iris_data,
        callbacks=[DvcLiveCallback("eval_data", model_file="model_xgb.json")],
        num_boost_round=5,
        evals=[(iris_data, "eval_data")],
    )
    . . .
    preds = model.predict(iris_data)

Regardless, I think we should test the model_file in a separate test function, similar to how it is tested in test_keras.

@daavoo daavoo self-assigned this Aug 2, 2021
@ezhdi
Copy link
Contributor Author

ezhdi commented Aug 2, 2021

thanks. fixed

@codecov-commenter
Copy link

codecov-commenter commented Aug 2, 2021

Codecov Report

Merging #131 (8c85adb) into master (b24fda7) will increase coverage by 0.11%.
The diff coverage is 100.00%.

❗ Current head 8c85adb differs from pull request most recent head ebe9183. Consider uploading reports for the commit ebe9183 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #131      +/-   ##
==========================================
+ Coverage   88.69%   88.81%   +0.11%     
==========================================
  Files          11       11              
  Lines         292      295       +3     
==========================================
+ Hits          259      262       +3     
  Misses         33       33              
Impacted Files Coverage Δ
dvclive/metrics.py 94.39% <100.00%> (ø)
dvclive/version.py 72.22% <100.00%> (ø)
dvclive/xgb.py 100.00% <100.00%> (ø)

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 b24fda7...ebe9183. Read the comment docs.

@daavoo daavoo merged commit 12b2f99 into treeverse:master Aug 2, 2021
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.

Save model in XGBoost

3 participants