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.

Sign up for GitHub

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

Jump to bottom

Upgrade Treelite module #3316

Merged
merged 10 commits into from
Jan 7, 2021
Merged

Upgrade Treelite module #3316

merged 10 commits into from
Jan 7, 2021

Conversation

hcho3
Copy link
Contributor

@hcho3 hcho3 commented Dec 17, 2020 •
edited
Loading

Depends on dmlc/treelite#235, dmlc/treelite#236, dmlc/treelite#237, dmlc/treelite#240, dmlc/treelite#241, dmlc/treelite#242, dmlc/treelite#243

Depends on rapidsai/integration#201

Closes #2160
Closes #2795
Closes #2817

TODOs

@hcho3 hcho3 added 2 - In Progress Currenty a work in progress 5 - Merge After Dependencies Depends on another PR: do not merge out of order Build or Dep Issues related to building the code or dependencies CUDA / C++ CUDA issue Cython / Python Cython or Python issue non-breaking Non-breaking change Tech Debt Issues related to debt and removed 5 - Merge After Dependencies Depends on another PR: do not merge out of order labels Dec 17, 2020
@hcho3 hcho3 added the BigConflictCauser Large refactors or similar that will touch many files label Dec 18, 2020
@hcho3 hcho3 mentioned this pull request Dec 29, 2020
Merged
@hcho3 hcho3 mentioned this pull request Dec 30, 2020
Closed
@hcho3
Copy link
Contributor Author

hcho3 commented Dec 30, 2020

I submitted conda-forge/treelite-feedstock#18 to upload the 1.0.0rc1 version of Treelite to Conda-forge.

@hcho3 hcho3 changed the title [WIP] Upgrade Treelite module Upgrade Treelite module Dec 30, 2020
@hcho3 hcho3 added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currenty a work in progress labels Dec 30, 2020
@hcho3 hcho3 marked this pull request as ready for review December 30, 2020 07:27
@hcho3 hcho3 requested review from a team as code owners December 30, 2020 07:27
@hcho3 hcho3 mentioned this pull request Dec 30, 2020
Merged
@hcho3 hcho3 added the conda conda issue label Dec 30, 2020
ajschmidt8
ajschmidt8 approved these changes Jan 5, 2021
View reviewed changes
Copy link
Member

@ajschmidt8 ajschmidt8 left a comment

Choose a reason for hiding this comment

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

approving. treelite has been upgraded in the integration repo PR below

rapidsai/integration#201

@hcho3
Copy link
Contributor Author

hcho3 commented Jan 5, 2021

Rerun tests

@hcho3 hcho3 added the improvement Improvement / enhancement to an existing function label Jan 5, 2021
hcho3
hcho3 commented Jan 6, 2021
View reviewed changes
cpp/src/fil/fil.cu Outdated
Comment on lines 799 to 808
// Display appropriate warnings when float64 values are being casted into
// float32, as FIL only supports inferencing with float32 for the time being
if (std::is_same<T, double>::value || std::is_same<L, double>::value) {
CUML_LOG_WARN(
"The tree ensemble model contains float64 values. Since FIL "
"currently only supports inferencing with float32 values, "
"all thresholds and leaf values will be converted from "
"float64 to float32. This may lead to inaccurate "
"prediction.");
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to reviewer: We finally have a robust method for warning about float64 use.

@codecov-io
Copy link

Codecov Report

Merging #3316 (86fa70b) into branch-0.18 (ae7e444) will decrease coverage by 0.01%.
The diff coverage is 66.66%.

Impacted file tree graph

@@               Coverage Diff               @@
##           branch-0.18    #3316      +/-   ##
===============================================
- Coverage        71.48%   71.47%   -0.02%     
===============================================
  Files              207      207              
  Lines            16750    16753       +3     
===============================================
  Hits             11974    11974              
- Misses            4776     4779       +3
Impacted Files Coverage Δ
python/cuml/fil/fil.pyx 91.87% <60.00%> (-1.88%) ⬇️
python/cuml/benchmark/algorithms.py 94.38% <100.00%> (ø)
python/cuml/ensemble/randomforest_shared.pyx 100.00% <100.00%> (ø)
...l/_thirdparty/sklearn/preprocessing/_imputation.py 62.50% <0.00%> (+0.40%) ⬆️

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 ae7e444...86fa70b. Read the comment docs.

wphicks
wphicks requested changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

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

Looks good! One question and one wording tweak. Otherwise, this is good to go.

cpp/src/decisiontree/decisiontree_impl.cuh Show resolved Hide resolved
cpp/src/fil/fil.cu Outdated Show resolved Hide resolved
@wphicks wphicks mentioned this pull request Jan 6, 2021
Merged
wphicks
wphicks approved these changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@wphicks wphicks left a comment

Choose a reason for hiding this comment

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

LGTM!

JohnZed
JohnZed approved these changes Jan 6, 2021
View reviewed changes
Copy link
Contributor

@JohnZed JohnZed left a comment

Choose a reason for hiding this comment

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

(just did a quick pass since Will and AJ went into more detail)

@ajschmidt8
Copy link
Member

rerun tests

@ajschmidt8
Copy link
Member

rerunning tests. looks like the latest failure was a test timeout and not related to your most recent warning message change commit. hopefully this run passes.

adding the 6 - Okay to Auto-Merge label since we have all the codeowner sign offs and this is holding up the container builds and other cuml PRs. it should merge automatically once the CI checks pass again.

@rapids-bot rapids-bot bot merged commit b8e71ca into rapidsai:branch-0.18 Jan 7, 2021
@hcho3 hcho3 deleted the upgrade_treelite branch January 7, 2021 03:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wphicks wphicks wphicks approved these changes

@ajschmidt8 ajschmidt8 ajschmidt8 approved these changes

@JohnZed JohnZed JohnZed approved these changes

Assignees
No one assigned
Labels
3 - Ready for Review Ready for review by team BigConflictCauser Large refactors or similar that will touch many files Build or Dep Issues related to building the code or dependencies conda conda issue CUDA / C++ CUDA issue Cython / Python Cython or Python issue improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Tech Debt Issues related to debt
Projects
None yet
Milestone
No milestone
5 participants
@hcho3 @codecov-io @ajschmidt8 @JohnZed @wphicks