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.

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

[WIP] pythonPackages.nevergrad: init at 0.3.2 #80424

Closed
wants to merge 1 commit into from
Closed

[WIP] pythonPackages.nevergrad: init at 0.3.2 #80424

wants to merge 1 commit into from

Conversation

juliendehos
Copy link
Contributor

Motivation for this change

Add Nevergrad (a Python gradient-free optimization framework).

Things done

Tested on NixOS and on Nix+Debian10, using the following shell.nix:

with import (fetchTarball "https://github.com/juliendehos/nixpkgs/archive/6afeb1dd1ff5c79b82d0e0ef697fe9cffc434706.tar.gz") {};
(python3.withPackages ( ps: with ps; [ nevergrad ])).env

and with the following example (from Nevergrad docs):

import nevergrad as ng

def square(x):
    return sum((x - .5)**2)

optimizer = ng.optimizers.OnePlusOne(parametrization=2, budget=100)
recommendation = optimizer.minimize(square)
print(recommendation)
  • 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.

@juliendehos
Copy link
Contributor Author

Thanks for your comments. I've updated the PR and squashed the commits.

drewrisinger
drewrisinger suggested changes Mar 25, 2020
View reviewed changes
Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

Looks pretty good other than small changes. Haven't tried building.

pkgs/development/python-modules/nevergrad/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/nevergrad/default.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/nevergrad/default.nix Outdated Show resolved Hide resolved
drewrisinger
drewrisinger reviewed Mar 26, 2020
View reviewed changes
pkgs/development/python-modules/nevergrad/default.nix

src = fetchFromGitHub {
owner = "facebookresearch";
repo = "nevergrad";
Copy link
Contributor

Choose a reason for hiding this comment

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

You could do repo = pname here but also fine as is.

drewrisinger
drewrisinger suggested changes Mar 26, 2020
View reviewed changes
Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

Build fails (test failures) with nixpkgs-review pr 80424 for python37Packages.nevergrad
Full build log: https://pastebin.com/tvB9ibfc
python38Packages.nevergrad also fails, but due to broken python38Packages.pytorch.

@juliendehos
Copy link
Contributor Author

I have the same problem with nix-review but I can't reproduce the problem "manually" in a nix-shell.
There is a new release of nevergrad (0.4.0) so I tried to package it but it now depends on a pre-built binary package (fcmaes) and I don't know how to package that correctly.
So I'm afraid I have to give up with this package.

@drewrisinger
Copy link
Contributor

I have the same problem with nix-review but I can't reproduce the problem "manually" in a nix-shell.

I usually try to build it with e.g. nix-build -A python37Packages.nevergrad. That usually works for me. I've never had luck running the python phases manually from nix-shell.

@drewrisinger
Copy link
Contributor

drewrisinger commented Mar 30, 2020 •
edited

There is a new release of nevergrad (0.4.0) so I tried to package it but it now depends on a pre-built binary package (fcmaes) and I don't know how to package that correctly.
So I'm afraid I have to give up with this package.

Re fcmaes: I took a brief glance at it GitHub link. You could either package it straight from github (which is a bit harder with installation, see below), or pull it from pypi via fetchPypi (probably easier, looks like it has the pre-built C++ libraries included).

If you choose to install from GitHub, I would just use a regular python package, with the following additions:

  • to buildInputs/nativeBuildInputs: add cmake.
  • to preBuild: run the install.sh script in ./_fcmaes
  • set dontUseCmakeConfigure = true at top level of nix script.

I think that should cover it from a quick glance.

@juliendehos
Copy link
Contributor Author

Thanks for the help.
I tried nix-build too but that didn't reproduce the problem either (the tests passed).

I tried to package fcmaes a few days ago and asked for help here : https://discourse.nixos.org/t/build-a-python-package-which-uses-a-c-library/6097/2
It looks like the pre-built lib need to be patched for mkl but I'm not sure how to do that.
Btw, I've just noticed the project seems to include the source code of the lib, so maybe we can build it ourself. I'll try to do that.

@juliendehos juliendehos mentioned this pull request Mar 30, 2020
Closed
10 tasks
@juliendehos
Copy link
Contributor Author

@drewrisinger I have a package for fcmaes that seems to work, thanks to your help. I'll try to package the latest version of nevergrad and send another PR then.

@drewrisinger
Copy link
Contributor

If it's just those two (nevergrad, fcmaes) as new packages, you could merge them into this one PR and submit together. that allows them to be tested together. could also include the bump to 0.4.0

drewrisinger
drewrisinger reviewed Mar 30, 2020
View reviewed changes
Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

Comments re pytestCheckHook.

pkgs/development/python-modules/nevergrad/default.nix
Comment on lines +36 to +37
checkInputs = [ pytest ];
checkPhase = "pytest";
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
checkInputs = [ pytest ];
checkPhase = "pytest";
checkInputs = [ pytestCheckHook];

I suggest using pytestCheckHook, it's slightly cleaner and gives more flexibility in future if you need to disable tests, etc. Not a hard requirement.

pkgs/development/python-modules/nevergrad/default.nix
, matplotlib
, opencv4
, pandas
, pytest
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
, pytest
, pytestCheckHook

@juliendehos
Copy link
Contributor Author

juliendehos commented Mar 30, 2020 •
edited

I've already opened a PR for fcmaes. So I will rebase and update this PR when possible (and switch to pytestCheckHook, as suggested).

@veprbl veprbl changed the title pythonPackages.nevergrad: init at 0.3.2 [WIP] pythonPackages.nevergrad: init at 0.3.2 May 1, 2020
@juliendehos juliendehos deleted the nevergrad branch September 1, 2020 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jonringer jonringer jonringer left review comments

@drewrisinger drewrisinger drewrisinger requested changes

@FRidh FRidh Awaiting requested review from FRidh

Assignees
No one assigned
Labels
6.topic: python 8.has: package (new) 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
3 participants
@juliendehos @drewrisinger @jonringer