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.

Sign up for GitHub

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

Jump to bottom

dlib: improve AVX configuration #56786

Merged
merged 2 commits into from
Mar 30, 2019
Merged

dlib: improve AVX configuration #56786

merged 2 commits into from
Mar 30, 2019

Conversation

Ma27
Copy link
Member

@Ma27 Ma27 commented Mar 3, 2019 •
edited
Loading

Motivation for this change

This change aims to improve the overall situation of how AVX is used to compile DLib and its Python bindings. This PR consists of the following commits:

  • 9732c44: Adds avxSupport (true by default) to the dlib builds. If older hardware is used, DLib can be compiled without AVX.

  • 6fec5aa: Bump face_recognition to 1.2.3 and disable the check phase for now to avoid breakage on Hydra builders without AVX support. After changes, maintainers should run checkPhase locally in a nix-shell.

Please have a look at the full commit messages for more information.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Ma27 Ma27 requested a review from FRidh as a code owner March 3, 2019 14:05
@GrahamcOfBorg GrahamcOfBorg added 6.topic: python 8.has: documentation This PR adds or changes documentation labels Mar 3, 2019
@FRidh
Copy link
Member

FRidh commented Mar 3, 2019

I don't think it's worth bothering with a having an option, since one can use overrideAttrs to modify configureFlags.

@Ma27
Copy link
Member Author

Ma27 commented Mar 3, 2019

I don't think it's worth bothering with a having an option, since one can use overrideAttrs to modify configureFlags.

I'd prefer an explicit option though: this problem seems to be fairly common (even Hydra builders are affected), if somebody experiences such a problem, he can simply turn on the build option without knowing about details like which CMake flag to set or how to add further flags to setup.py when building python packages with Nix. IMHO we are responsible for this as maintainers.

Ma27 added 2 commits March 3, 2019 15:35
@Ma27
dlib: add flag to disable AVX support
9732c44 
Especially older hardware doesn't support AVX instructions. DLib is
still functional there, but significantly slower[1].

By setting `avxInstructions` to false, DLib will be compiled without
this feature.

[1] http://dlib.net/compile.html
@Ma27
pythonPackages.face_recognition: 1.2.2 -> 1.2.3
6fec5aa 
There's no git tag for 1.2.3, hence we need to pin to the corresponding
revision because we build from a git source.

After recent breakage on Hydra[1], the tests were disabled. Although
some build machines don't support AVX, we shouldn't use a DLib without
AVX as the builder's result is also used on modern machines with AVX
support. Before merging changes, maintainers should run the check phase
locally in a `nix-shell`.

[1] https://hydra.nixos.org/build/89533530
@Ma27 Ma27 force-pushed the dlib-avx-fixes branch from 450e9a9 to 6fec5aa Compare March 3, 2019 14:36
@Ma27 Ma27 mentioned this pull request Mar 5, 2019
Closed
@Ma27
Copy link
Member Author

Ma27 commented Mar 7, 2019

@FRidh anything else to add from your side?
@volth thanks! I'll look into this that afternoon :)

@Ma27
Copy link
Member Author

Ma27 commented Mar 9, 2019

@FRidh anything else to add? I'd love to see this merged :)

@Ma27
Copy link
Member Author

Ma27 commented Mar 16, 2019

Ping @FRidh, I'd love to get this merged. It's not perfect, but let's merge this for now and wait for better platform feature detection. Unless I hear anything else from you, I'd merge and backport next Friday.

@Ma27
Copy link
Member Author

Ma27 commented Mar 30, 2019

Merging for now. Until we have a proper hardware feature detection implemented, this should be the best solution for now as it ensures that the package is buildable on all Hydra builders and you don't need to build it locally if i.e. a KVM is used to build dlib by Hydra.

I didn't get any further objections during the last two weeks, so this should be fine. As soon as a better detection mechanism is implemented, I'll fix dlib accordingly.

@Ma27 Ma27 merged commit 4cd107f into NixOS:master Mar 30, 2019
@Ma27 Ma27 deleted the dlib-avx-fixes branch March 30, 2019 18:20
@Ma27
Copy link
Member Author

Ma27 commented Mar 30, 2019

To ensure that nobody faces this problem on 19.03, I backported this as 6f55b04 and f32f452.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FRidh FRidh FRidh left review comments

Assignees
No one assigned
Labels
6.topic: python 8.has: documentation This PR adds or changes documentation 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Ma27 @FRidh @GrahamcOfBorg