Create template classes for model representations to support multiple data types (Part of #196) #198

hcho3 · 2020-09-10T06:49:46Z

Extracted from #196 for easier reviewing.
(The CI tests in this PR won't pass due to missing parts. See #196 to look up the test results.)

The ModelImpl class (created in Refactor struct Model -> class Model + class ModelImpl (Part of #196) #201) becomes the template class ModelImpl<ThresholdType, LeafOutputType>.
Implement template classes ModelImpl<ThresholdType, LeafOutputType> and TreeImpl<ThresholdType, LeafOutputType> that contain the details of the tree ensemble model. The template classes are parameterized by the types of the thresholds and leaf outputs. Currently the following combinations are allowed:

Threshold type	Leaf output type
float32	float32
float32	uint32
float64	float64
float64	uint32

Revise the zero-copy serialization protocol, to prepend the type information (threshold_type, leaf_output_type) to the serialized types so that the recipient will choose the correct ModelImpl<ThresholdType, LeafOutputType> to deserialize to.
A run-time type dispatching system using the enum type TypeInfo. Users are able to dispatch a correct version of ModelImpl<ThresholdType, LeafOutputType> by specifying a pair of TypeInfo values. We also implement a set of convenient functions, such as InferTypeInfoOf<T> that converts the template arg T into TypeInfo enum.

@canonizer @levsnv @jakirkham @wphicks Can you review this PR?

wphicks

Just one grammatical suggestion. Otherwise, everything looks great!

include/treelite/tree.h

jakirkham · 2020-09-10T19:46:46Z

There's a few things being passed as pointers. Would it make sense to pass them by reference instead? Or would there be issues with that?

include/treelite/tree_impl.h

hcho3 · 2020-09-10T19:59:35Z

@jakirkham

Would it make sense to pass them by reference instead? Or would there be issues with that?

Cpplint and Google C++ style guide warn against using non-const reference parameters in functions.

jakirkham · 2020-09-10T20:52:37Z

Interesting, I'm curious why Google thinks a potentially null pointer is safer than a reference 😄

hcho3 · 2020-09-11T01:44:16Z

@jakirkham The idea is to explicitly mark "output" parameters. FooBar(&a) clearly shows that a is modified, whereas FooBar(a) isn't clear whether a is being modified or not.

include/treelite/tree.h

canonizer · 2020-09-12T01:02:25Z

include/treelite/typeinfo.h

+    return "float64";
+  default:
+    throw std::runtime_error("Unrecognized type");
+    return "";


Is return required after throw?

Without it, some old compilers like GCC 5 complains about a missing return value.

canonizer · 2020-09-12T01:08:44Z

include/treelite/typeinfo.h

+ * \return Whatever that's returned by the dispatcher
+ */
+template <template<class> class Dispatcher, typename ...Args>
+inline auto DispatchWithTypeInfo(TypeInfo type, Args&& ...args) {


It's hard to understand what the intended usage of DispatchWithTypeInfo() / DispatchWithModelTypes() is. Could you add an example (or reference a place in code) demonstrating it?

Here is the idea. Let's say we are given a TypeInfo variable type at runtime. We want to call Dispatcher<T>::Dispatch() method where the template arg T corresponds to type, and Dispatcher<T> is a class that contains a custom function that we'd like to run.

For a concrete example, see https://github.com/dmlc/treelite/pull/198/files#diff-409f38fcc1a201a94e70922bbf7c44a6R777-R788 and https://github.com/dmlc/treelite/pull/196/files#diff-65416f46630c396d5db3df0ac92a0a02R130-R148.

include/treelite/typeinfo.h

canonizer · 2020-09-12T01:11:43Z

include/treelite/typeinfo.h

+  } else if (std::is_same<T, double>::value) {
+    return TypeInfo::kFloat64;
+  } else {
+    throw std::runtime_error(std::string("Unrecognized Value type") + typeid(T).name());


Is runtime type information already used in treelite?

Do you mean the built-in RTTI? Yes, since we use dynamic_cast<> to downcast Model* into ModelImpl<>*.

include/treelite/tree.h

include/treelite/tree_impl.h

… data types

Co-authored-by: William Hicks <[email protected]>

hcho3 · 2020-09-18T22:45:38Z

@canonizer Can I get another round of review for this PR?

canonizer

Approved, provided that the remaining comments are addressed.

include/treelite/tree.h

canonizer · 2020-09-23T21:18:23Z

include/treelite/tree.h

  Model(const Model&) = delete;
  Model& operator=(const Model&) = delete;
  Model(Model&&) = default;
  Model& operator=(Model&&) = default;

+  template <typename ThresholdType, typename LeafOutputType>
+  inline static std::unique_ptr<Model> Create();


Consider moving this function into ModelImpl, as it depends on ThresholdType and LeafOutputType.

It is actually not possible to move the function, since it needs access the base class's members threshold_type_ and leaf_output_type_. The members were moved to the base class per your previous suggestion.

canonizer · 2020-09-23T21:33:03Z

include/treelite/tree.h

+    return leaf_output_type_;
+  }
+  template <typename Func>
+  inline auto Dispatch(Func func);


Are these Dispatch() functions used anywhere?

Yes, it's used in the C code generator (excluded from this PR). See #196 to search for actual uses.

Here is an example: https://github.com/dmlc/treelite/pull/196/files#diff-e85efd30a12be68c2d66cb6a7b674d48R124

… data types (Part of #196) (#198) * The ModelImpl class (created in #201) becomes the template class ModelImpl<ThresholdType, LeafOutputType>. * Implement template classes ModelImpl<ThresholdType, LeafOutputType> and TreeImpl<ThresholdType, LeafOutputType> that contain the details of the tree ensemble model. The template classes are parameterized by the types of the thresholds and leaf outputs. Currently the following combinations are allowed: | Threshold type | Leaf output type | |----------------|------------------| | float32 | float32 | | float32 | uint32 | | float64 | float64 | | float64 | uint32 | |----------------|------------------| * Revise the zero-copy serialization protocol, to prepend the type information (threshold_type, leaf_output_type) to the serialized types so that the recipient will choose the correct ModelImpl<ThresholdType, LeafOutputType> to deserialize to. * A run-time type dispatching system using the enum type TypeInfo. Users are able to dispatch a correct version of ModelImpl<ThresholdType, LeafOutputType> by specifying a pair of TypeInfo values. We also implement a set of convenient functions, such as InferTypeInfoOf<T> that converts the template arg T into TypeInfo enum. Co-authored-by: William Hicks <[email protected]> Co-authored-by: Andy Adinets <[email protected]>

Addresses #95 and #111. Follow-up to #198, #199, #201 Trying again, since #130 failed. This time, I made the Model class to be polymorphic. This way, the amount of pointer indirection is minimized. Summary: Model is an opaque container that wraps the polymorphic handle ModelImpl<ThresholdType, LeafOutputType>. The handle in turn stores the list of trees Tree<ThresholdType, LeafOutputType>. To unbox the Model container and obtain ModelImpl<ThresholdType, LeafOutputType>, use Model::Dispatch(<lambda expression>). Also, upgrade to C++14 to access the generic lambda feature, which proved to be very useful in the dispatching logic for the polymorphic Model class. * Turn the Model and Tree classes into template classes * Revise the string templates so that correct data types are used in the generated C code * Rewrite the model builder class * Revise the zero-copy serializer * Create an abstract matrix class that supports multiple data types (float32, float64 for now). * Move the DMatrix class to the runtime. * Extend the DMatrix class so that it can hold float32 and float64. * Redesign the C runtime API using the DMatrix class. * Ensure accuracy of scikit-learn models. To achieve the best results, use float32 for the input matrix and float64 for the split thresholds and leaf outputs. * Revise the JVM runtime.

hcho3 force-pushed the new_model_rep branch from ae0fd7e to ab00d7b Compare September 10, 2020 07:00

hcho3 changed the base branch from mainline to multi_type_refactor_breakup September 10, 2020 07:01

wphicks requested changes Sep 10, 2020

View reviewed changes

include/treelite/tree.h Outdated Show resolved Hide resolved

JohnZed mentioned this pull request Sep 10, 2020

[BUG] LightGBM 3 not working with FIL rapidsai/cuml#2795

Closed

jakirkham reviewed Sep 10, 2020

View reviewed changes

include/treelite/tree_impl.h Show resolved Hide resolved

canonizer reviewed Sep 12, 2020

View reviewed changes

This was referenced Sep 14, 2020

Refactor struct Model -> class Model + class ModelImpl (Part of #196) #201

Merged

New prediction runtime C API, to support multiple data types (Part of #196) #199

Merged

hcho3 and others added 3 commits September 18, 2020 15:20

Create template classes for model representations to support multiple…

7029bb2

… data types

Update include/treelite/tree.h

bf7d71e

Co-authored-by: William Hicks <[email protected]>

Address review comments from @canonizer

09e908b

hcho3 force-pushed the new_model_rep branch from e42c3c4 to 09e908b Compare September 18, 2020 22:28

canonizer approved these changes Sep 23, 2020

View reviewed changes

Address more comments from @canonizer

1306930

hcho3 force-pushed the new_model_rep branch from 1e1c6e0 to 1306930 Compare September 23, 2020 22:33

hcho3 merged commit 269dd2e into dmlc:multi_type_refactor_breakup Sep 25, 2020

hcho3 deleted the new_model_rep branch September 25, 2020 08:04

This was referenced Sep 25, 2020

Add support for multiple data types (remainder of #196) #203

Merged

Add support for multiple data types (remainder of #196) #204

Closed

hcho3 mentioned this pull request Sep 25, 2020

Add support for multiple data types (will break up into smaller pull requests) #196

Closed

8 tasks

wphicks mentioned this pull request Sep 25, 2020

Update linting rules to allow non-const reference parameters #205

Closed

This was referenced Oct 9, 2020

core Errors when predict online from the xgboost model with java api #163

Closed

Option for 64bit input data #95

Closed

hcho3 mentioned this pull request Oct 24, 2020

Different score after compiling a lightgbm model #115

Closed

hcho3 mentioned this pull request Dec 17, 2020

Add back Clone() method to the Tree class #235

Merged

hcho3 mentioned this pull request Dec 29, 2020

Enforce invariant: Treelite codegen doesn't support uint32 leaf output #240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create template classes for model representations to support multiple data types (Part of #196) #198

Create template classes for model representations to support multiple data types (Part of #196) #198

hcho3 commented Sep 10, 2020 •

edited

Loading

wphicks left a comment

jakirkham commented Sep 10, 2020

hcho3 commented Sep 10, 2020 •

edited

Loading

jakirkham commented Sep 10, 2020

hcho3 commented Sep 11, 2020

canonizer Sep 12, 2020

hcho3 Sep 12, 2020

canonizer Sep 12, 2020

hcho3 Sep 12, 2020

canonizer Sep 12, 2020

hcho3 Sep 12, 2020

hcho3 commented Sep 18, 2020

canonizer left a comment

canonizer Sep 23, 2020

hcho3 Sep 23, 2020

canonizer Sep 23, 2020

hcho3 Sep 23, 2020 •

edited

Loading

hcho3 Sep 23, 2020

Create template classes for model representations to support multiple data types (Part of #196) #198

Create template classes for model representations to support multiple data types (Part of #196) #198

Conversation

hcho3 commented Sep 10, 2020 • edited Loading

wphicks left a comment

Choose a reason for hiding this comment

jakirkham commented Sep 10, 2020

hcho3 commented Sep 10, 2020 • edited Loading

jakirkham commented Sep 10, 2020

hcho3 commented Sep 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hcho3 commented Sep 18, 2020

canonizer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hcho3 Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hcho3 commented Sep 10, 2020 •

edited

Loading

hcho3 commented Sep 10, 2020 •

edited

Loading

hcho3 Sep 23, 2020 •

edited

Loading