-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
haskellPackages.libtorch-ffi: Fix the link to libtorch #367998
haskellPackages.libtorch-ffi: Fix the link to libtorch #367998
Conversation
e250f25
to
3107945
Compare
3107945
to
9dceca0
Compare
Can we make changes like this PR in maintainers/scripts/haskell? |
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'm not familiar with haskell packages infra really, but the first line of the file says: /* hackage-packages.nix is auto-generated by hackage2nix -- DO NOT EDIT MANUALLY! */
. I presume there should be some other place for applying overrides?
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.
Maybe this is the place? pkgs/development/haskell-modules/replacements-by-name/
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.
Maybe pkgs/development/haskell-modules/configuration-nix.nix
is the better tool, I see that libGL
related patches and configurations are applied there
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.
Thank you for your review! I'll check the place.
configureFlags = [ | ||
"--extra-include-dirs=${lib.getDev pkgs.libtorch-bin}/include/torch/csrc/api/include" | ||
] ++ lib.optionals pkgs.config.cudaSupport [ "-f cuda" ]; | ||
}) ({ | ||
c10 = pkgs.libtorch-bin; | ||
torch_cpu = pkgs.libtorch-bin; | ||
torch = pkgs.libtorch-bin; | ||
} // lib.optionalAttrs (pkgs.config.cudaSupport) { | ||
torch_cuda = pkgs.libtorch-bin; | ||
}); | ||
|
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.
Any particular reason to choose the wheel package over the real build from python3Packages.torch
?
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.
AFAIK python3Packages.torch
uses pre-cxx11 ABI. It needs the extra option for compiling and linking.
So it uses the libtorch.
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.
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.
AFAIK python3Packages.torch uses pre-cxx11 ABI. It needs the extra option for compiling and linking.
So it uses the libtorch.
Huh. I wonder if we should introduce another attribute for cxx11-abi source-built torch, or just make cxx11-abi the default. "There are no stupid questions", but: could the latter cause symbol collisions with other native extensions? The new attribute, otoh, could be named libtorch
to contrast it with libtorch-bin
. We definitely should do one of the two, but not in this PR
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.
For other native extensions of pytorch, we need to add a cmake option not to use cxx11-abi, otherwise symbol conflicts will occur.
gcc and clang use cxx11-abi by default.
I think it's good that cxx11-abi libtorch is the default, because users don't have to specify any options.
I don't think libtorch users are also native extension users, so I'm not sure if the two options are necessary.
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.
For other native extensions of pytorch
Well yes we use extensions like torchaudio with the same variant of pytorch we compile it against, incl. the same version of the cuda stack. But I rather meant, other extensions like opencv4, zeromq, etc, etc that use libstdc++.
Bottom line: would be nice for us at some point to investigate moving hasktorch from libtorch-bin to source-build torch
Indeed, hackage-packages.nix may not be edited manually. You can re do your PR based on the |
9dceca0
to
10fafcc
Compare
@SomeoneSerge @sternenseemann Thank you for great help!
|
10fafcc
to
ca8b8a8
Compare
I assume 0.2.1.1 fixes some issue 0.2.0.1 still exhibits? I think the manual upgrades will be obsolete soon, we'll have to bump hackage again soon for unrelated reasons. |
The 0.2.0.1 has an issue of cabal file and no support for Apple GPU. |
It might be better to remove the version and wait for the update. For now, the test will fail though. |
ca8b8a8
to
b2fb84e
Compare
It removed the versions of hasktorch and libtorch-ffi. |
@@ -388,6 +388,10 @@ self: super: ({ | |||
http-reverse-proxy = dontCheck super.http-reverse-proxy; | |||
servant-auth-server = dontCheck super.servant-auth-server; | |||
|
|||
sysinfo = overrideCabal (orig: { | |||
doCheck = true; |
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.
There are doCheck / dontCheck helpers in haskell.lib.
Also is this a bitflip? Don’t you want to disable the test?
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 think this should be sysinfo = dontCheck super.sysinfo
here, as it used to be doCheck = !…isDarwin
.
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.
@maralorn @sternenseemann
It was wrong. Thanks for pointing that out. I've fixed it.
…properly on the sandbox of macOS.
b2fb84e
to
0145fac
Compare
@junjihashimoto In #371032, |
@sternenseemann |
I will make it possible to build hasktorch at the nightly stage before the LTS is released. |
Yes, I saw that! I've made a note to remove the broken flag when we have that version. |
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.