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

Implementing Rust runtime API #189

Closed
higumachan opened this issue Jul 21, 2020 · 17 comments
Closed

Implementing Rust runtime API #189

higumachan opened this issue Jul 21, 2020 · 17 comments

Comments

@higumachan
Copy link
Contributor

Hi.
I'm thinking of creating a Runtime API for Rust, should I create it as a separate repository or in the runtime/rust directory?

@hcho3
Copy link
Collaborator

hcho3 commented Jul 21, 2020

@higumachan Hi, thank you so much for reaching out. I'd love to have a Rust runtime. Unfortunately, the current C API is not yet stable and is about to break soon. Here is why:

The current runtime API has turned out to be rather deficient, in that it can only handle model and data with 32-bit floating point values. LightGBM and scikit-learn use 64-bit floating-point values, so in some corner cases Treelite produces wrong results. Related: #155 #153 #115 #98 #95. To make Treelite work, I will have to re-write the runtime API to handle multiple data types.

I have been occupied with other priorities so far (I'm a maintainer of XGBoost). Let me make time and design a new runtime API by end of August. #130 was my previous attempt and it failed due to a poor API design.

@higumachan
Copy link
Contributor Author

@hcho3
Thank you for response.
OK,I will wait for the C Runtime API to stable and implement the Rust runtime.

@hcho3
Copy link
Collaborator

hcho3 commented Sep 4, 2020

@higumachan Update: I'm currently working to enable 64-bit floating-point types in #196. The pull request has grown really large, so I plan to break it up and have others review it. Would you be interested in reviewing the new runtime API?

@higumachan
Copy link
Contributor Author

@hcho3 Sorry, I'm late in replying.
I'm interested in reviewing the new API.
I would like to review it if I can get it assigned with some PRs.

@hcho3
Copy link
Collaborator

hcho3 commented Sep 9, 2020

@higumachan Great! Let me assign you some PRs that will be relevant to the new Rust runtime.

@hcho3
Copy link
Collaborator

hcho3 commented Sep 10, 2020

@higumachan I assign you #199 to review. Thanks for offering to review.

@hcho3
Copy link
Collaborator

hcho3 commented Oct 9, 2020

@higumachan Hi, all the refactored code has now been merged into the mainline branch. You can now start building on it.

@dovahcrow
Copy link
Contributor

Hi @higumachan @hcho3 I recently come to a project where I need to use lightgbm in Rust. I built https://github.com/dovahcrow/lightgbm-rs and then found this treelite project promising. If any help is needed to build a treelite Rust API binding, I definitely would like to help!

I also find there already exists a treelite rust binding on crates.io https://crates.io/crates/treelite, however, there's no document or source code available. I think it would better to involve the author @zhchang here as well.

@zhchang
Copy link

zhchang commented Jan 8, 2021 via email

@dovahcrow
Copy link
Contributor

dovahcrow commented Jan 9, 2021

I had the runtime partially implemented at https://github.com/dovahcrow/treerite.

The CI is failing due to the cmake 3.14 requirement - the rust github action had 3.13. Are there any ways to relax the cmake version requirement?

@hcho3
Copy link
Collaborator

hcho3 commented Jan 9, 2021

@dovahcrow I filed #246 to relax the CMake requirement to 3.13. Can you try it out?

@dovahcrow
Copy link
Contributor

@hcho3 unfortunately, it failed:

--- stderr
  CMake Error at cmake/ExternalLibs.cmake:8 (FetchContent_MakeAvailable):
    Unknown CMake command "FetchContent_MakeAvailable".
  Call Stack (most recent call first):
    CMakeLists.txt:42 (include)

@hcho3
Copy link
Collaborator

hcho3 commented Jan 9, 2021

@dovahcrow I did not realize that FetchContent_MakeAvailable was unavailable in CMake 3.13. I revised the pull request to make it compatible with CMake 3.13. I tested it on my machine with CMake 3.13. Can you try it now?

@dovahcrow
Copy link
Contributor

@hcho3 thanks! I can confirm cmake works now. However, I got other CI errors originated from my side - I'm working on that.

@dovahcrow
Copy link
Contributor

@hcho3 Good news, with #246 the CI passed. https://github.com/dovahcrow/treerite/actions/runs/473692895.

I noticed that the shared library generated by treelite sometimes depends on glibc sometimes not. Previously the CI failed due to the model binary built on my machine has a higher glibc dependency than the CI environment.

@dovahcrow
Copy link
Contributor

dovahcrow commented Jan 13, 2021

The crate is published here, which binds to the just merged 1.0.0 release. You can use it by adding treerite = "0.1" in your Cargo.toml. Sorry, I forgot to change the readme on crates.io.

@hcho3
Copy link
Collaborator

hcho3 commented Mar 12, 2021

Closed by #247

@hcho3 hcho3 closed this as completed Mar 12, 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

No branches or pull requests

4 participants