-
Notifications
You must be signed in to change notification settings - Fork 734
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
[PyTorch] Update to 2.4.0, add distributed #1510
base: master
Are you sure you want to change the base?
Conversation
This reverts commit 4b99f14.
@HGuillemet Let's just merge this and fix anything broken with the upgrade to PyTorch 2.4? |
I'm about to add a commit for 2.4.0 upgrade |
Ok, we can do that too |
Before merging, you must decide what to do with bytedeco/javacpp#766 |
Also to decide: whether adding the cuda dependency is a good thing or not. |
An optional dependency on the presets for CUDA that only torch_cuda needs, sure, that's fine. |
.put(new Info().javaText("import org.bytedeco.cuda.cusolver.*;")) | ||
.put(new Info().javaText("import org.bytedeco.cuda.cudnn.*;")) | ||
// .put(new Info().javaText("import org.bytedeco.cuda.nccl.*;")) // Not on Windows | ||
.put(new Info().javaText("import org.bytedeco.pytorch.chrono.*;")) |
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.
Instead of doing that, please add helper.class, chrono.class and cudnn.class to the @Platform(inherit=...
list.
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.
Done for cuda, but what helper.class
and chrono.class
are you talking about ?
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.
Right, JavaCPP assumes the helper classes to be in the same package as the generated classes, so let's remove that package too?
As for chrono, see bytedeco/javacpp#766 (comment)
Consider my comment on top post: the dependency will be mandatory when using CUDA. Presently, most users use their system cuda I guess, they will now have to use cuda presets. |
Sure, that's fine, it's not a huge dependency |
This reverts commit 6151980.
@saudet please restart the jobs that failed |
@@ -26,8 +26,7 @@ runs: | |||
|
|||
brew uninstall --force --ignore-dependencies gcc gcc@7 gcc@8 gcc@9 gcc@10 gcc@11 gcc@12 gcc@13 gcc@14 little-cms2 maven openblas r | |||
brew install boost ccache swig autoconf-archive automake cmake libomp libtool libusb ant nasm xz pkg-config sdl2 gpg1 bison flex perl ragel binutils gradle gmp isl libmpc mpfr geoip pcre ssdeep yajl | |||
brew link --force libomp | |||
|
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.
Why do you need to remove this line?
.put(new Info().enumerate().friendly()) | ||
.put(new Info().javaText("import org.bytedeco.pytorch.*;")) | ||
.put(new Info().javaText("import org.bytedeco.pytorch.cuda.functions.*;")) | ||
.put(new Info().javaText("import org.bytedeco.pytorch.Error;")) | ||
.put(new Info().javaText("import org.bytedeco.pytorch.global.torch.DeviceType;")) | ||
.put(new Info().javaText("import org.bytedeco.pytorch.global.torch.ScalarType;")) | ||
.put(new Info().javaText("import org.bytedeco.pytorch.global.torch.MemoryFormat;")) |
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.
Why do we need those imports here?
"<cublas.h>", // pytorch includes cublas_v2, which is not compatible with cublas included from inherited cudnn presets | ||
"<driver_functions.h>" // causes #warning | ||
}, | ||
library = "jnitorch" |
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.
Please don't do that, leave it as jnitorch_cuda
inherit = { torch.class, chrono.class }, | ||
value = { | ||
@Platform( | ||
library = "jnitorch" |
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.
Please don't do that, leave it as jnigloo
chmod +w ../lib/libiomp5.dylib | ||
install_name_tool -id @rpath/libiomp5.dylib ../lib/libiomp5.dylib | ||
install_name_tool -change @rpath/libomp.dylib @rpath/libiomp5.dylib ../lib/libtorch_cpu.dylib | ||
cp $(brew ls libomp|grep libomp.dylib) ../lib |
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.
Please revert that. This is necessary to make it work with MKL
Included in this PR:
intrusive_ptr
, enabling transparent usage like forshared_ptr
weak_ptr
, enabling transparent usage (could be moved to JavaCPP ?)org.bytedeco.javacpp.pathsFirst
and their system cuda has same version than the cuda presets).functions
packages to main packageRemains to be done: