Skip to content

pythonPackages.osqp: remove mkl dependency #84124

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
Merged
merged 1 commit into from
Apr 3, 2020
Merged

pythonPackages.osqp: remove mkl dependency #84124

merged 1 commit into from
Apr 3, 2020
+8 −10

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.

Sorry, something went wrong.

@drewrisinger
pythonPackages.osqp: remove mkl

Verified

This commit was signed with the committer’s verified signature.
Mic92 Jörg Thalheim
GPG key ID: 003F2096411B5F92
Verified
Learn about vigilant mode
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

@ofborg ofborg bot added 6.topic: python 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Apr 2, 2020
@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
Loading

@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
Loading

@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

@matthewbauer matthewbauer

@bhipple bhipple

@jonringer jonringer

Assignees
No one assigned
Labels
6.topic: python 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

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