-
Notifications
You must be signed in to change notification settings - Fork 41
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
base: master
Are you sure you want to change the base?
Conversation
@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...). |
There was a problem hiding this 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://github.com/davechallis/xgboost | |||
branch = master | |||
url = https://github.com/dmlc/xgboost |
There was a problem hiding this comment.
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...).
xgboost-sys/build.rs
Outdated
.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"); |
There was a problem hiding this comment.
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...).
There was a problem hiding this comment.
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.
Signed-off-by: yihong0618 <zouzou0208@gmail.com>
intel/m1 compatibility
This one took me a bit, but I think it's fixed by 0307a92 |
Update version
…v2.0.3; Add train code in train iteration; Passed all examples.
Fix the NaN recall in xgboost training
Updated to use c++17
update for 2021 edition
Needs detailed review: