Hi @calebho , this is great! I need to learn a bit more about how pip works and discuss it with the team. We will take a look at get back to you.

In the mean time, do you know why the tests are failing?

@lanctot Yeah I know why; will try to work on it tonight or tomorrow. It's because the wrong version of cmake is being invoked in CI

Good morning! Thank you for your contribution.

One goal for the library is to be easy to modify, to enable and speed up research on games. Thus, having an install package is definitely a plus, but it is also important to be able to modify the code and built it locally.

This CL is changing the full development process (It was just ./open_spiel/scripts/build_and_run_tests.sh before), in a way that is not that clear to me.

Is the iterative development process still possible with your new way to install a package? Or should we keep both installation procedures (i.e. building as before, and releasing a pip package)?

To rephrase, I am wondering: what happens when one want to modify the sources and rebuild? Is the process much different? Is it simply a question of rerunning python3 -m pip install . every time?

I looked at it quite extensively, and I think we may want to keep both workflows, as people may prefer one or the other (I also have not tried the pip workflow, so I do not know how good it is, especially when using pip install -e.). There are also subtle changes (replacing virtualenv with venv, or changes in the MacOS build), that are difficult to review and be sure it's ok.

As a side note, as we have no core developer on Mac, it would be really nice to have #84.

This CL is changing the build on MAC, and it's very hard for us to test any of this.

Apologies for the delay as I've been busy the past few days. Will address this sometime this week

Is the iterative development process still possible with your new way to install a package?

Yes. In order to build and run tests, you can simply invoke python setup.py test.

There are also subtle changes (replacing virtualenv with venv, or changes in the MacOS build), that are difficult to review and be sure it's ok.

Happy to revert the venv changes--I have a personal preference for standard library tools when there's no compelling reason to use something else.

I agree the Mac changes are difficult to test automatically. As you mentioned, #84 will help with this. I happen to have a Mac and a Linux box so I was able to test on both locally. I hope this addresses your concerns @jblespiau; let me know if you have any further questions

Given that we use a separate workflow within Google, this adds an additional layer to the development process that we cannot experiment ourselfe on our day-to-day work.

I would be tempted to prefer an option where we do not change the current way people can build, but in which we add, in addition to the current process, the possibility to build and deploy using pip. Later, if it appears it is superior, we can still drop the current way of developing.

I suppose that this would imply, not updating in place all the current install and testing, but adding a new path (e.g. install_with_pip.sh and a specific travis test that runs this).

Ditto; will work on changes

Build is failing because the wrong version of cmake is being used by the pip build

Thanks. I patched the PR, and tried to run it myself.
I ran ./open_spiel/scripts/pip_install_and_run_tests.sh and it worked as expected.

When I ran it a second time, I got an error however:

Building wheels for collected packages: pyspiel
  Running setup.py bdist_wheel for pyspiel ... error
  Complete output from command /usr/bin/python3 -u -c "import setuptools, tokenize;__file__='/tmp/pip-req-build-so02ymbg/setup.py';f=getattr(tokenize, 'open', open)(__file__);code=f.read().replace('\r\n', '\n');f.close();exec(compile(code, __file__, 'exec'))" bdist_wheel -d /tmp/pip-wheel-mn4voggi --python-tag cp37:
  running bdist_wheel
  running build
  running build_ext
  cmake version 3.13.4
  
  CMake suite maintained and supported by Kitware (kitware.com/cmake).
  CMake Error: The current CMakeCache.txt directory /tmp/pip-req-build-so02ymbg/build/temp.linux-x86_64-3.7/CMakeCache.txt is different than the directory /usr/local/google/home/jblespiau/tmp/open_spiel/copybara/build/temp.linux-x86_64-3.7 where CMakeCache.txt was created. This may result in binaries being created in the wrong place. If you are not sure, reedit the CMakeCache.txt
  CMake Error: The source "/tmp/pip-req-build-so02ymbg/open_spiel/CMakeLists.txt" does not match the source "/usr/local/google/home/jblespiau/tmp/open_spiel/copybara/open_spiel/CMakeLists.txt" used to generate cache.  Re-run cmake with a different source directory.
  Traceback (most recent call last):
    File "<string>", line 1, in <module>
    File "/tmp/pip-req-build-so02ymbg/setup.py", line 120, in <module>
      zip_safe=False,
    File "/usr/lib/python3/dist-packages/setuptools/__init__.py", line 145, in setup
      return distutils.core.setup(**attrs)
    File "/usr/lib/python3.7/distutils/core.py", line 148, in setup
      dist.run_commands()
    File "/usr/lib/python3.7/distutils/dist.py", line 966, in run_commands
      self.run_command(cmd)
    File "/usr/lib/python3.7/distutils/dist.py", line 985, in run_command
      cmd_obj.run()
    File "/usr/lib/python3/dist-packages/wheel/bdist_wheel.py", line 188, in run
      self.run_command('build')
    File "/usr/lib/python3.7/distutils/cmd.py", line 313, in run_command
      self.distribution.run_command(command)
    File "/usr/lib/python3.7/distutils/dist.py", line 985, in run_command
      cmd_obj.run()
    File "/usr/lib/python3.7/distutils/command/build.py", line 135, in run
      self.run_command(cmd_name)
    File "/usr/lib/python3.7/distutils/cmd.py", line 313, in run_command
      self.distribution.run_command(command)
    File "/usr/lib/python3.7/distutils/dist.py", line 985, in run_command
      cmd_obj.run()
    File "/tmp/pip-req-build-so02ymbg/setup.py", line 51, in run
      self.build_extension(ext)
    File "/tmp/pip-req-build-so02ymbg/setup.py", line 64, in build_extension
      ["cmake", ext.sourcedir] + cmake_args, cwd=self.build_temp)
    File "/usr/lib/python3.7/subprocess.py", line 363, in check_call
      raise CalledProcessError(retcode, cmd)
  subprocess.CalledProcessError: Command '['cmake', '/tmp/pip-req-build-so02ymbg/open_spiel', '-DPython_TARGET_VERSION=3.6', '-DCMAKE_CXX_COMPILER=g++', '-DCMAKE_LIBRARY_OUTPUT_DIRECTORY=/tmp/pip-req-build-so02ymbg/build/lib.linux-x86_64-3.7']' returned non-zero exit status 1.
  
  ----------------------------------------
  Failed building wheel for pyspiel
  Running setup.py clean for pyspiel
Failed to build pyspiel
Installing collected packages: pyspiel
  Running setup.py install for pyspiel ... -^[[done
Successfully installed pyspiel-0.0.1rc2
+ python3 setup.py test
running test
running egg_info
writing pyspiel.egg-info/PKG-INFO
writing dependency_links to pyspiel.egg-info/dependency_links.txt
writing requirements to pyspiel.egg-info/requires.txt
writing top-level names to pyspiel.egg-info/top_level.txt
reading manifest file 'pyspiel.egg-info/SOURCES.txt'
reading manifest template 'MANIFEST.in'
warning: no files found matching '*.hpp' under directory 'open_spiel'
writing manifest file 'pyspiel.egg-info/SOURCES.txt'
running build_ext
cmake version 3.13.4

I imagine it's because it has already been built, but it would be good to understand why this happens and how to fix it. I also need to find out the next steps to actually make a release/releases out of it.

I will not have time in the near future to help debugging this PR. As you say, it's currently not building on Travis, and it's indeed because of a CMake version. The version we currently require is indeed newer than the one supported by setuptools.

CMake suite maintained and supported by Kitware (kitware.com/cmake).
CMake Error at CMakeLists.txt:3 (cmake_minimum_required):
CMake 3.12 or higher is required.  You are running version 3.10.2

Unfortunately, we won't be able to include a change that does not pass the continuous integration tests.

It seems there exists addons to support CMake (e.g. https://pypi.org/project/cmake-setuptools/). I have no idea if they are easy to use or would solve the problem.

I looked around, and I have also find https://github.com/deepmind/tree/blob/master/setup.py which builds code, but this is using Bazel and not CMake, so It does not help much.

Sorry not to be able to help more.

Thanks for the feedback. I'll have time next week to revisit this

Okay I think this is in a working state now. Brief summary:

• Changed the testing mechanism from setup.py test to nox since the former is deprecated. To run tests, install nox (pip install nox) then run nox -s tests.
• Added three new matrix entries in CI which use this new testing mechanism instead of testing both the old and new mechanisms in the same job

Hi @calebho, I changed a few critical files today (related to pip) to allow us to put a requirement of tensorflow >= 1.15 in requirements.txt (which was necessary due to security vulnerabilities in TF <= 1.14).

I've noticed it has created conflict with this PR. Can you pull from master and resolve them?

I don't think what I did should be a big problem, but I'm not 100% sure. It was necessary to force an upgrade to pip 19 since it's required by TF 1.15. I also changed the Travis CI meta files and script which I see you had also changed, but I guess the merging should be straight-forward.

Let me know, and Happy New Year!

Oh, we also changed the C++ compiler to use clang instead of g++. So you might want to try installing your pip after the merge just to make sure it compiles properly as before (I had to make one change to CMakeLists.txt for different CXX compilation flags).

Here are the changes for the g++ -> clang: 7c6b935

Thanks for your contribution, your time, your patience and perseverance.

I should have responded last week was I started integrating this change (and got distracted doing it instead of communicating I was doing it^^).

This has been submitted internally, we should get it released soon. Thanks again.