-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
dlib: use cuda and blas properly #279927
dlib: use cuda and blas properly #279927
Conversation
Result of 15 packages built:
|
I have fixed these things in #273665, but I haven't had the time to act on the feedback. |
Ah thanks for the feedback, don't know how I completely missed that you had already basically already put in this PR! I'll have a look into putting in the fixes CUDA granularity for dlib/default.nix tonight. I've just been testing this on c++ dlib library so can't comment on the python stuff much unfortunately. |
So, I've been looking into this a bit more and trying to split |
Was banging my head against a wall trying to get it work without cudatoolkit, then realized your pr suggestions @SomeoneSerge in #273665 did it so I've just implemented those here. Running the |
Result of 1 package marked as broken and skipped:
6 packages failed to build:
8 packages built:
|
Result of 1 package marked as broken and skipped:
14 packages built:
|
This is the error i'm getting for openturns I assume the other packages are similar, I'll have a proper look tomorrow.
|
cudaPackages.libcublas |
Yeah the lib is there on line 67, I think I need to be more explicit about where it is to cmake. |
patches = [ ] ++ lib.optional | ||
cudaSupport | ||
useNewCMakePatch; |
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.
nit: mv patches
up to after src nit2:
optionalsover
optional`
be5d69f
to
1864fdc
Compare
still broken for
and path definitely had the header file it wanted
face-recogniser is failing with this error
I wonder if it's somehow inheriting a different cuda dependency from somewhere else because when I run the same c++ cuda call in my testing flake it's fine? I'll have a proper investigation tomorrow. |
1864fdc
to
0c4e1dc
Compare
After doing a bit more research today I've concluded that Working fine
Broken by fix when run with {config.cudaSupport = true;}
Broken before this pr with and without {config.cudaSupport = true;}
The error Seeing that Would appreciate your input @SomeoneSerge, thanks! |
This just means you need to deploy a newer nvidia driver or build with older |
Result of 1 package marked as broken and skipped:
14 packages failed to build:
|
I noticed this before also, for some reason when I run nixpkgs-review everything would fail to build whereas when I just used the new fix as an overlay in my test flake it was fine (I suspect I have a misunderstood something about how nixpkgs works). I'll have more of a look into this after I fix the |
Result of 1 package marked as broken and skipped:
14 packages built:
|
@SomeoneSerge am I right in reading into this issue that accessing the GPU in tests is not currently supported in nixpkgs ? Because |
0c4e1dc
to
e1ca7b8
Compare
c1f76c9
to
315a44d
Compare
Result of 1 package marked as broken and skipped:
14 packages built:
|
Result of 1 package marked as broken and skipped:
14 packages built:
|
Provided disabling tests for |
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.
am I right in reading into this #225912 that accessing the GPU in tests is not currently supported in nixpkgs
Yes, since we want to be able to build derivations on machines that have no GPUs as well, we disable the GPU-related checks during the build. There's an idea to declare them in e.g. dedicated passthru derivations marked with requiredSystemFeatures
so that a selection of builders could be set up to expose GPUs in the sandbox
Provided disabling tests for python311Packages.face-recognition is acceptable, I think this is ready for review/merge.
Yes. It's preferable to keep as many tests as possible on, and to do pythonImportsCheck
. Some packages might fail even that if they eager-load the driver...
symlinkJoin
Is expensive and discouraged, but I think you could only get rid of it once we had patched dlib to support FindCUDAToolkit.cmake
I'll probably have one more look a bit later, but overall looks good!
You also mentioned here that it might be worth splitting cuda packages up into |
Yes, cf the linked issue
No, it a necessity created by upstream build scripts (e.g. expecting all cuda libraries in the same merged directory) |
Sounds good, I'll add those changes tonight. |
315a44d
to
42d5908
Compare
Result of 1 package marked as broken and skipped:
14 packages built:
|
@@ -36,6 +36,9 @@ buildPythonPackage rec { | |||
pytestCheckHook | |||
]; | |||
|
|||
# Disable tests | |||
doCheck = false; |
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.
Last nit! Can you make this doCheck = !cudaSupport
please
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.
Oh, I thought this was in dlib
. Well, dlib.cudaSupport
would've made sense still, but maybe out of scope
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 can still add it, I probably should put a reference to the issue as well, might help someone else who comes across it.
Result of 1 package marked as broken and skipped:
14 packages built:
|
f21e71d
to
d2fe369
Compare
…lt blas and added lapack to make it easier to override, removed fftw because dlib hasn't supported it since 2015 Signed-off-by: Robbie Buxton <[email protected]>
d2fe369
to
de87abd
Compare
Result of 1 package marked as broken and skipped:
14 packages built:
|
Result of 1 package marked as broken and skipped:
14 packages built:
|
@@ -36,6 +38,9 @@ buildPythonPackage rec { | |||
pytestCheckHook | |||
]; | |||
|
|||
# Disables tests when running with cuda due to https://github.com/NixOS/nixpkgs/issues/225912 | |||
doCheck = !config.cudaSupport; |
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.
Note for future: I think it's more flexible to expose a passthru in dlib
and test that intead of the global config
, because one might override dlib = prev.dlib.override { cudaSupport = true; }
specifically. But this is good enough as it is IMO
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.
Hey thanks a lot @RobbieBuxton, nice job! I'm honestly surprised it just worked w/o symlinkJoin
. And thanks @hacker1024 for all the preceding effort!
changed dlib to actually use cuda properly and switched back to blas and added lapack to make it easier to override, removed fftw because dlib hasn't supported it since 2015.
Fixes this Issue / closes #279924
Not building with CUDA
When you set
cudaSupport = True
, although it sets the CMAKE flag correctly the resulting package isn't actually built correctly as cmake can't find cuda so it unsets the flag. This is due to the incorrect usage ofpkgs.stdenv.mkDerivation
instead ofpkgs.cudaPackages.backendStdenv.mkDerivation
when building the cuda related packages. The correct usage can be seen in the OpenCV package. It also tries to build with CUDA by default which is wrong.dlib takes fftw as a build input
Dlib hasn't supported fftw since 2015 however there are leftover references to it in cmake which might lead people to think it does support fftw. fftw should be removed.
Taking in openblas rather than blas and adding lapack
As mentioned in this pr it makes more sense to have
blas
as an input intodlib
rather thatopenblas
so that people can override it with other BLAS implementations e.gmkl
more easily if they want to. The package was also missing lapack which should be added.Description of changes
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.