pysc2: init at 1.2 #30552

ghost · 2017-10-18T21:16:35Z

Motivation for this change

Things done

Tested using sandboxing (nix.useSandbox on NixOS, or option build-use-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/)
Fits CONTRIBUTING.md.

Mic92 · 2017-10-18T21:19:47Z

pkgs/development/python-modules/pysc2/fix-setup-for-py3.patch

+
+if (sys.version_info[0] == 2
+        or (sys.version_info[0] == 3 and sys.version_info[1] < 4)):
+    requires.append('enum34')


Can you also send this upstream?

i would like to eventually, but i have to do some legwork on the google contributors agreement before that's possible.

Mic92 · 2017-10-18T21:21:19Z

pkgs/development/python-modules/absl-py/default.nix

+  name = "absl-py-${version}";
+
+  src = fetchurl {
+    url = "mirror://pypi/a/absl-py/${name}.tar.gz";


Please use here in and in the other cases fetchPypi instead of fetchurl.

FRidh

Will you maintain these packages?

FRidh · 2017-10-19T12:33:27Z

pkgs/development/python-modules/absl-py/default.nix

+    sha256 = "017wc85i7l3vpjzw3shgb7k7n0jfid88g09dlf1kgdy4ll0sjfax";
+  };
+
+  buildInputs = [ six ];


propagatedBuildInputs if six is in install_requires of setup.py.

FRidh · 2017-10-19T12:33:41Z

pkgs/development/python-modules/mpyq/default.nix

+
+  buildInputs = [ argparse ];
+
+  doCheck = false;


Why? Leave a comment.

FRidh · 2017-10-19T12:34:33Z

pkgs/development/python-modules/mpyq/default.nix

+    sha256 = "01q0xh2fy3zzsrfr45d2ypj4whs7s060cy1rnprg6sg55fbgbaih";
+  };
+
+  buildInputs = [ argparse ];


argparse has been removed from our package set. Patch the source to remove the dependency on argparse.

ghost · 2017-10-19T21:41:12Z

@FRidh yes, i intend on maintaining these packages. i've added myself to the maintainers list and marked myself as each package's maintainer accordingly.

Mic92 · 2017-10-23T18:39:05Z

pkgs/top-level/python-packages.nix

@@ -257,6 +259,8 @@ in {
    pythonPackages = self;
  };

+  pysc2 = callPackage ../development/python-modules/pysc2 { };


Is this supposed to be used as a library or a command line application?

Both. The package comes with scripts for running a machine learning agent as a player in a Starcraft 2 scenario, but it is also a library for creating such an agent.

FRidh · 2017-10-23T18:52:53Z

pkgs/development/python-modules/pysc2/default.nix

+      wrapProgram $f \
+        --set SC2PATH "${sc2-headless}"
+    done
+  '';


It won't be able to find sc2 when you use this package as a library and you try to import this package. If this should be supported (and considering you wrote it can be used as a library I think that's the case), then instead of wrapping the executables the path to sc2 should be hardcoded, e.g. here https://github.com/deepmind/pysc2/blob/c8ad5e8fdc0fff5549c575abea9acbf11d903fe0/pysc2/run_configs/platforms.py#L122

Thanks for that. When I looked at the library aspect of the package it did not occur to me that someone could want to launch the environment in their own code instead of invoking one of the bin scripts with their agent as an argument. I've updated it to apply a patch that enables us to substitute the correct path and also cleans up their LD_LIBRARY_PATH manipulations since they aren't necessary in nix.

ghost · 2017-11-13T15:59:48Z

@FRidh Is there anything else I should do to improve this PR further?

FRidh

A commit per expression/package is needed.

FRidh · 2017-11-13T17:01:49Z

pkgs/development/python-modules/absl-py/default.nix

+, lib
+, fetchPypi
+, six
+, pytest


FRidh · 2017-11-13T17:01:54Z

pkgs/development/python-modules/mpyq/default.nix

+{ buildPythonPackage
+, lib
+, fetchPypi
+, argparse


FRidh · 2017-11-13T17:02:01Z

pkgs/development/python-modules/portpicker/default.nix

+{ buildPythonPackage
+, lib
+, fetchPypi
+, argparse


FRidh · 2017-11-13T17:03:05Z

pkgs/development/python-modules/pysc2/default.nix

+    absl-py
+    enum34
+    future
+    mock


Does it need mock during runtime? Its quite unlikely. Remember, install_requires goes here. tests_require goes to checkInputs

Their setup.py lists mock as an install_requires instead of a tests_requires [0]. They use it in a file that is part of their lib package. I suspect that it is a test that was just misplaced but it is where it is.

[0] https://github.com/deepmind/pysc2/blob/master/setup.py#L64
[1] https://github.com/deepmind/pysc2/blob/master/pysc2/lib/stopwatch_test.py

ghost · 2017-11-14T19:59:06Z

Thank you @FRidh for taking a close look at this PR. I've clearly bitten off more than I can chew but I'm glad to put in the work to get this ready to merge. I've implemented all of the changes you've requested except for the one involving mock. I've confirmed that the module that uses mock is available to the user at runtime and so it has to be included as an install dependency.

FRidh · 2017-11-14T20:24:58Z

Thanks!

ghost requested a review from FRidh as a code owner October 18, 2017 21:16

Mic92 reviewed Oct 18, 2017

View reviewed changes

ghost force-pushed the pysc2-init branch from b2889f5 to 35f35f5 Compare October 18, 2017 21:28

FRidh reviewed Oct 19, 2017

View reviewed changes

FRidh added the 6.topic: python label Oct 19, 2017

ghost force-pushed the pysc2-init branch from 35f35f5 to 7c35c3c Compare October 19, 2017 21:40

ghost requested a review from edolstra as a code owner October 19, 2017 21:40

ghost force-pushed the pysc2-init branch from 5d3b833 to eef09ad Compare October 23, 2017 16:36

Mic92 reviewed Oct 23, 2017

View reviewed changes

FRidh reviewed Oct 23, 2017

View reviewed changes

ghost force-pushed the pysc2-init branch from eef09ad to a21cf23 Compare October 23, 2017 20:47

ghost requested a review from peti as a code owner October 26, 2017 16:00

ghost force-pushed the pysc2-init branch from b1e3d32 to 7b8ae6d Compare October 26, 2017 16:01

GrahamcOfBorg added 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10 labels Oct 26, 2017

FRidh requested changes Nov 13, 2017

View reviewed changes

ghost force-pushed the pysc2-init branch from 7b8ae6d to 2e81984 Compare November 14, 2017 19:35

ghost requested a review from nbp as a code owner November 14, 2017 19:35

Dan Haraj added 5 commits November 14, 2017 14:37

absl-py: init at 0.1.3

08508f5

mpyq: init at 0.2.5

1327d09

portpicker: init at 1.2.0

2201774

s2clientprotocol: init at 3.19.1.58600.0

ced67b1

sc2-headless: add pysc2 minigame map pack

bdce7d3

pysc2: init at 1.2

6a70e7f

ghost force-pushed the pysc2-init branch from 2e81984 to 6a70e7f Compare November 14, 2017 19:38

FRidh merged commit 0cedba1 into NixOS:master Nov 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pysc2: init at 1.2 #30552

pysc2: init at 1.2 #30552

ghost commented Oct 18, 2017

Mic92 Oct 18, 2017

ghost Oct 18, 2017

Mic92 Oct 18, 2017

ghost Oct 18, 2017

FRidh left a comment

FRidh Oct 19, 2017

FRidh Oct 19, 2017

FRidh Oct 19, 2017

ghost commented Oct 19, 2017

Mic92 Oct 23, 2017

ghost Oct 23, 2017 •

edited by ghost

Loading

FRidh Oct 23, 2017

ghost Oct 23, 2017

ghost commented Nov 13, 2017

FRidh left a comment

FRidh Nov 13, 2017

FRidh Nov 13, 2017

FRidh Nov 13, 2017

FRidh Nov 13, 2017

ghost Nov 13, 2017

ghost commented Nov 14, 2017 •

edited by ghost

Loading

FRidh commented Nov 14, 2017

pysc2: init at 1.2 #30552

pysc2: init at 1.2 #30552

Conversation

ghost commented Oct 18, 2017

Motivation for this change

Things done

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FRidh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Oct 19, 2017

Choose a reason for hiding this comment

ghost Oct 23, 2017 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 13, 2017

FRidh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Nov 14, 2017 • edited by ghost Loading

FRidh commented Nov 14, 2017

ghost Oct 23, 2017 •

edited by ghost

Loading

ghost commented Nov 14, 2017 •

edited by ghost

Loading