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

pythonPackages.osqp: remove mkl dependency #84124

Merged
merged 1 commit into from Apr 3, 2020
Merged

pythonPackages.osqp: remove mkl dependency #84124

merged 1 commit into from Apr 3, 2020

Conversation

drewrisinger
Copy link
Contributor

Motivation for this change

Removing mkl allows this to be built in Hydra (mkl is unfree-ish license), and it seems the mkl dependency is somewhat optional given downstream packages build cleanly when removing mkl.
Also add explicit numpy/scipy dependency.
Cleanup commit slightly for formatting.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • 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 nixpkgs-review --run "nixpkgs-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)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@drewrisinger
pythonPackages.osqp: remove mkl
beedfa5 
Also add scipy.
Cleanup commit slightly for formatting.
Removing mkl allows this to be built in Hydra (mkl = unfree),
and it seems the mkl dependency is somewhat optional given downstream
packages build cleanly when removing mkl.
@drewrisinger
Copy link
Contributor Author

@GrahamcOfBorg build python37Packages.osqp python37Packages.cvxpy python38Packages.osqp python38Packages.cvxpy

@bhipple
Copy link
Contributor

bhipple commented Apr 2, 2020

CC @matthewbauer, who is looking at making this type of thing parameterizable in #83888

@drewrisinger
Copy link
Contributor Author

drewrisinger commented Apr 2, 2020 via email •
edited

@matthewbauer
Copy link
Member

This case is a little different because osqp doesn't use MKL for BLAS, but MKL PARDISO (https://software.intel.com/en-us/mkl-developer-reference-fortran-intel-mkl-pardiso-parallel-direct-sparse-solver-interface) which is unique to MKL.

Anyway, it looks like this library just needs MKL to test the MKL functionality. Since osqp just uses dlopen, you can always enable MKL support through LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$(nix-build '<nixpkgs>' -A mkl)/lib.

@drewrisinger
Copy link
Contributor Author

This case is a little different because osqp doesn't use MKL for BLAS, but MKL PARDISO (https://software.intel.com/en-us/mkl-developer-reference-fortran-intel-mkl-pardiso-parallel-direct-sparse-solver-interface) which is unique to MKL.

Anyway, it looks like this library just needs MKL to test the MKL functionality. Since osqp just uses dlopen, you can always enable MKL support through LD_LIBRARY_PATH=$LD_LIBRARY_PATH:$(nix-build '<nixpkgs>' -A mkl)/lib.

Ack, thanks for the good tips. I wasn't familiar with what MKL PARDISO did. pythonPackages.osqp does just use MKL PARDISO for testing PARDISO functionality AFAICT, it probably also enables some runtime functionality but I view that as a downstream thing. I prefer for build purposes to have no unfree dependencies so as much as possible can be built/cached by Hydra.

@drewrisinger
Copy link
Contributor Author

@GrahamcOfBorg build python37Packages.qiskit-aer python37Packages.qiskit-ignis
@GrahamcOfBorg build python38Packages.qiskit-aer python38Packages.qiskit-ignis

@drewrisinger
Copy link
Contributor Author

Result of nixpkgs-review pr 84124

9 package built:
python27Packages.osqp python37Packages.cvxpy python37Packages.osqp python37Packages.qiskit-aer python37Packages.qiskit-ignis python38Packages.cvxpy python38Packages.osqp python38Packages.qiskit-aer python38Packages.qiskit-ignis

@drewrisinger drewrisinger mentioned this pull request Apr 3, 2020
Merged
10 tasks
bhipple
bhipple approved these changes Apr 3, 2020
View reviewed changes
Copy link
Contributor

@bhipple bhipple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Result of nixpkgs-review pr 84124 1

9 package built: - python27Packages.osqp - python37Packages.cvxpy - python37Packages.osqp - python37Packages.qiskit-aer - python37Packages.qiskit-ignis - python38Packages.cvxpy - python38Packages.osqp - python38Packages.qiskit-aer - python38Packages.qiskit-ignis

@bhipple bhipple merged commit c5313eb into NixOS:master Apr 3, 2020
@bhipple
Copy link
Contributor

bhipple commented Apr 3, 2020

Can you send a 20.03 backport PR as well?

@drewrisinger drewrisinger deleted the dr-pr-py-osqp-remove-mkl branch April 3, 2020 22:09
@drewrisinger
Copy link
Contributor Author

drewrisinger commented Apr 3, 2020 •
edited

@bhipple python3Packages.osqp isn't in release-20.03.

@bhipple
Copy link
Contributor

bhipple commented Apr 3, 2020

Ah ok, my bad! For some reason I thought we'd had this packaged for longer, but I was thinking of another related pkg.

@drewrisinger drewrisinger mentioned this pull request Apr 3, 2020
Merged
10 tasks
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

@matthewbauer matthewbauer matthewbauer approved these changes

@bhipple bhipple bhipple approved these changes

@jonringer jonringer Awaiting requested review from jonringer

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@drewrisinger @bhipple @matthewbauer @FRidh