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

Refactor Rust crate avoid build #2161

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

jlloh
Copy link

@jlloh jlloh commented Sep 8, 2022

Originally had this MR to fix the build for Mac M1. #2160 .

After testing this in my environment, I realised that with the current implementation:

  1. The current implementation does not allow the crate to be easily published to crates.io due to dependencies on the base repo as it compiles the shared library from source
    • This repo is actually pretty big, and if caching this for CI, it is >1GB and affects build speed pretty significantly
  2. Looking at how onnxruntime's rust bindings and pytorch's rust bindings download the shared library (in this case libcatboost.so/dylib) over building it
  3. Taking inspiration from that, I modified catboost-sys to download the libcatboost shared library from github releases (and did a soft link to ../../libs/model_interface to allow bindgen to generate the necessary rust bindings. I managed to publish the crate here.
  4. After that, I subsequently changed the dependencies for the actual crate itself and published it to here.
    • Note that I had to call the crate catboost-rs instead of catboost because there's an existing empty crate already using that name, preventing me from using it

Note that for now in build.rs I check for apple + aarch64 before downloading the dylib, as that's the machine I have. I don't have a non-M1 mac to test if that works fine.

Would like to get feedback on whether this is a reasonable approach, and can be merged into master.

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en.

@jlloh jlloh changed the title Refactor crate avoid build Refactor Rust crate avoid build Sep 8, 2022
@jlloh
Copy link
Author

jlloh commented Oct 15, 2022

Minor update: As I haven't heard back from the maintainers, and it seems like overkill to fork the whole repo and maintain it, I have opted to split out the code to it's own repository, similar to onnxruntime-rs, since all I need are the files in catboost/libs/model_interface for catboost-sys.

Instead, to build catboost-sys, the user is responsible for cloning the catboost repo himself and set an environment variable CATBOOST_DIR.

The crate is published here.

Leaving the MR open for now in case the maintainers ever see it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants