Skip to content

Cvxpy#59518

Closed
teh wants to merge 4 commits intoNixOS:masterfrom
teh:cvxpy
Closed

Cvxpy#59518
teh wants to merge 4 commits intoNixOS:masterfrom
teh:cvxpy

Conversation

@teh
Copy link
Contributor

@teh teh commented Apr 14, 2019

Motivation for this change

Add cvxpy + dependencies

Finishing off the cvxpy packaging mentioned in #45047

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 nix-review --run "nix-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.

@teh teh requested a review from FRidh as a code owner April 14, 2019 20:23
@GrahamcOfBorg GrahamcOfBorg added 6.topic: python Python is a high-level, general-purpose programming language. 8.has: package (new) This PR adds a new package 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 14, 2019
Copy link
Member

@costrouc costrouc left a comment

Choose a reason for hiding this comment

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

Small fixes required but otherwise good. nix-review passed

Copy link
Member

Choose a reason for hiding this comment

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

Remove period from description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

remove period from description

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this? Can we not package that one separately?

Copy link
Contributor Author

@teh teh Apr 15, 2019

Choose a reason for hiding this comment

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

I could download the source separately but AFAICT the python module needs the osqp source, i.e. it's not just linking against a library [1].

[1]
https://github.com/oxfordcontrol/osqp-python/blob/master/setup.py#L95

Copy link
Contributor Author

@teh teh left a comment

Choose a reason for hiding this comment

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

thanks for the review!

@stites
Copy link
Member

stites commented Oct 8, 2019

Heads up, this PR can fail at test time, possibly due to bad seeds:

======================================================================
FAIL: test_ch8_mcsdp (test_examples.TestExamples)
----------------------------------------------------------------------
Traceback (most recent call last):                     
  File "/build/cvxopt-1.2.3/tests/test_examples.py", line 48, in test_ch8_mcsdp
    self.assertAlmostEqualLists(list(z[::n+1]),n*[1.0],5)
  File "/build/cvxopt-1.2.3/tests/test_examples.py", line 19, in assertAlmostEqualLists
    for u,v in zip(L1,L2): self.assertAlmostEqual(u,v,places)
AssertionError: 3.1962078445048556 != 1.0 within 5 places
----------------------------------------------------------------------
Ran 39 tests in 172.236s
FAILED (failures=1, skipped=6)

This is the second time I've run this build. The first time the value was ~1.2

Given the nature of the test, it is probably a bug in the software and it might be worth bumping this to the latest 1.0.25. This is not fixed in 1.0.25 and I'm not sure where this bug is coming from : (

@teh
Copy link
Contributor Author

teh commented Oct 9, 2019

That's pretty bad! cvxopt isn't part of this branch, it's been in master for a while. Not sure why it got triggered as build-from-source for you. I tried this (as a trusted user, e.g. root):

nix-build -A python3Packages.cvxopt --option repeat 10 --option substitute false  --no-out-link
these derivations will be built:
  /nix/store/wh6ikia2ck8xqwl2ljnifibrldsbpbvy-cvxopt-1.2.3.tar.gz.drv
  /nix/store/x9c5sc969yfwg2yjzi01m8w8x6dp2hrf-python3.7-cvxopt-1.2.3.drv
building '/nix/store/wh6ikia2ck8xqwl2ljnifibrldsbpbvy-cvxopt-1.2.3.tar.gz.drv'...
...
building '/nix/store/x9c5sc969yfwg2yjzi01m8w8x6dp2hrf-python3.7-cvxopt-1.2.3.drv' (round 1/11)...
...
building '/nix/store/x9c5sc969yfwg2yjzi01m8w8x6dp2hrf-python3.7-cvxopt-1.2.3.drv' (round 11/11)...
...
OK (skipped=6)
/nix/store/47x71dgcpf7h9214wr8hkjmz1pma3sk7-python3.7-cvxopt-1.2.3

And can't reproduce. Can you share your build command?

@stites
Copy link
Member

stites commented Oct 10, 2019

Oops! cvxopt works fine on unstable, actually. The problem was that I was pulling cvxopt into a shell from this PR (which is broken) and I was too rushed to tell the differentiate between the suffixes. Unfortunately, I am still getting a test error with cvxpy : /

======================================================================
ERROR: test_psd_nsd_parameters (cvxpy.tests.test_expressions.TestExpressions)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/build/cvxpy-1.0.21/cvxpy/tests/test_expressions.py", line 265, in test_psd_nsd_parameters
    p.value = a2
  File "/build/cvxpy-1.0.21/cvxpy/expressions/constants/parameter.py", line 60, in value
    self._value = self._validate_value(val)
  File "/build/cvxpy-1.0.21/cvxpy/expressions/leaf.py", line 428, in _validate_value
    "%s value must be %s." % (self.__class__.__name__, attr_str)
ValueError: Parameter value must be positive semidefinite.

----------------------------------------------------------------------
Ran 1882 tests in 12.856s

FAILED (SKIP=2, errors=1)
Error in AA type 1, iter: 39, info: 0, norm 4.22e+04
Error in AA type 1, iter: 39, info: 0, norm 4.22e+04
Error in AA type 1, iter: 10, info: 0, norm 1.24e+04
builder for '/nix/store/i9y43yq923acwvxl3xamp1pczagw9mc4-python3.6-cvxpy-1.0.21.drv' failed with exit code 1                                
cannot build derivation '/nix/store/mlbl4p4a5vy99dans7zkhsiw85j6szwj-python3-3.6.8-env.drv': 1 dependencies couldn't be built
error: build of '/nix/store/mlbl4p4a5vy99dans7zkhsiw85j6szwj-python3-3.6.8-env.drv' failed                                                  

I pinned my nix-shell to your PR. Here's a compressed shell file:

# shell.nix
{ ... }:

let
  hostpkgs = import <nixpkgs> {};
  srcDef = builtins.fromJSON ''{
    "url": "https://github.com/NixOS/nixpkgs",
    "rev": "0115285de38b27ed9c3cf900ec3abff0a7a37632",
    "date": "2019-04-19T11:38:25+01:00",
    "sha256": "05yb5ww7wl68v4fwq0fg6k6sdf49ykfr7yckhlyx2a1glzpdzs1h",
    "fetchSubmodules": false
  }'';
in

with (import (hostpkgs.fetchFromGitHub {
  owner = "NixOS";
  repo = "nixpkgs";
  inherit (srcDef) rev sha256;
}) { });

mkShell {
  buildInputs = [
    (python36.withPackages (ps: with ps; [
      cvxpy
    ]))
  ];
}

With the nixos commit being this PR: 0115285

I also copied your default.nix, placed it explicitly in the withPackages section and bumped the sha256+version to 1.0.25, but still got the same error. Nothing changes on python 3.7.

@teh
Copy link
Contributor Author

teh commented Oct 10, 2019

I did a rebase against master and several rebuilds / tests and still can't reproduce. One thing that's a bit confusing is that you got numerical errors first on cvxopt and now cvxpy.

The only similar incident I can remember recently was related to an openblas bug. What's your CPU architecture (/proc/cpuinfo)? I'm on Intel(R) Core(TM) i7-5557U CPU @ 3.10GHz

@stites
Copy link
Member

stites commented Jan 13, 2020

Quick followup -- I just built this on an Intel i7-8665U (8) @ 4.800GHz successfully, but got the above error when building on Intel i9-7900X (20) @ 4.500GHz. Both were built on NixOS, as well as x64 architectures.

Let me know if I can help debug this further. I'm not quite sure what to do next.

@drewrisinger
Copy link
Contributor

I think this was superseded by #78898, which was just merged. Feel free to close @jonringer @FRidh @costrouc @teh

@FRidh FRidh closed this Feb 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: python Python is a high-level, general-purpose programming language. 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants