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

A batch of updates to get to xgboost 2.0.3 #21

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

Conversation

montanalow
Copy link

  • builds with xgboost v2.0.3
  • expose cuda support as a feature
  • some additional OS X (openmp) compatibility for homebrew
  • move upstream to dmlc/xgboost
  • fix cargo dependencies for git submodules
  • update cargo dependencies
  • update DMatrix uri loading format for 2.0 compat
  • fix cargo clippy lints
    • static lifetimes are no longer required
  • fix dmatrix slicing out of bounds segfault
  • cargo fmt (rustfmt.toml to minimize changes where possible)

Needs detailed review:

@davechallis
Copy link
Owner

@montanalow Thanks for this, looks good on quick read through! I'll spend a bit of time reviewing in detail shortly (just dealing with a bit of a backlog after xmas...).

Copy link
Owner

@davechallis davechallis left a comment

Choose a reason for hiding this comment

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

Code changes mostly look good I think.

I'm having some trouble building xgboost-sys after the changes though, so still investigating that. I get the errors:

error[E0412]: cannot find type `_Tp` in this scope
   --> /Users/dsc/src/rust-xgboost/xgboost-sys/target/debug/build/xgboost-sys-95c0101c1ac52cb0/out/bindings.rs:452:27
    |
452 |     pub static std_value: _Tp;
    |                           ^^^ not found in this scope

For more information about this error, try `rustc --explain E0412`.
error: could not compile `xgboost-sys` (lib) due to previous error

I'll carry on trying to find the cause though, possible I'm using an outdated llvm install.

@@ -1,4 +1,3 @@
[submodule "xgboost-sys/xgboost"]
path = xgboost-sys/xgboost
url = https:/davechallis/xgboost
branch = master
url = https:/dmlc/xgboost
Copy link
Owner

Choose a reason for hiding this comment

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

iirc, this was originally pointed at a fork to deal with some build issues with the original library involving unused packages. Hopefully it's fixed now though (I'm hazy on the details, but was something to do with the python bindings perhaps...).

.define("CMAKE_C_COMPILER", "/opt/homebrew/opt/llvm/bin/clang")
.define("CMAKE_CXX_COMPILER", "/opt/homebrew/opt/llvm/bin/clang++")
.define("OPENMP_LIBRARIES", "/opt/homebrew/opt/llvm/lib")
.define("OPENMP_INCLUDES", "/opt/homebrew/opt/llvm/include");
Copy link
Owner

Choose a reason for hiding this comment

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

It would be good to leave this as configurable (to allow for non-homebrew LLVM installs), and to allow for homebrew installs to different locations.

Homebrew defaults to /usr/local for intel macs, and /opt/homebrew for silicon: https://docs.brew.sh/FAQ#why-should-i-install-homebrew-in-the-default-location, so bound to be some variation here.

I had to comment out the above to compile, since I'm on an intel mac (hoping for an m1 someday...).

Copy link
Author

Choose a reason for hiding this comment

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

I added a check for the m1 directory existence before adding these extras in 1da1c03

Let me know if this is still causing an issue for intel. I don't have a laptop I can test this one on.

@montanalow
Copy link
Author

Code changes mostly look good I think.

I'm having some trouble building xgboost-sys after the changes though, so still investigating that. I get the errors:

error[E0412]: cannot find type `_Tp` in this scope
   --> /Users/dsc/src/rust-xgboost/xgboost-sys/target/debug/build/xgboost-sys-95c0101c1ac52cb0/out/bindings.rs:452:27
    |
452 |     pub static std_value: _Tp;
    |                           ^^^ not found in this scope

For more information about this error, try `rustc --explain E0412`.
error: could not compile `xgboost-sys` (lib) due to previous error

I'll carry on trying to find the cause though, possible I'm using an outdated llvm install.

This one took me a bit, but I think it's fixed by 0307a92

montanalow and others added 4 commits April 29, 2024 17:59
Update version
…v2.0.3; Add train code in train iteration; Passed all examples.
Fix the NaN recall in xgboost training
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.

6 participants