Tournaments can run on Windows #721

drvinceknight · 2016-09-20T20:46:01Z

Started with #719 but has a bit more:

Have added the ability to keep everything in memory: currently this is the default and temporary files are not used at all but I want to think about that some more (might be more efficient to keep the temporary files as default unless we're on windows which we could just detect and in that case keep things in memory). I'm not overly exited with the fix in Fix for temporary files on windows. #713 as it risks keeping files around if anything is interrupted...
There are some tests for the above that I need to write ideally.
Have just skipped the parallel processing tests if on windows (have also skipped testing 2.7 and 3.4 on windows because life is short and they were throwing weird bugs). I feel happier to say that parallel processing is not supported on Windows than making hacky changes...

I believe this will pass all CI tests... Heading to bed: will work on some more tomorrow :)

This has not been used for a while now.

Todo: adjust docs.

This reverts commit af41e22.

Because life is short.

meatballs · 2016-09-21T07:17:25Z

I feel happier to say that parallel processing is not supported on Windows than making hacky changes...

Agreed - we could even wrap the parallel processing in a condition that checks the os

drvinceknight · 2016-09-21T07:43:32Z

we could even wrap the parallel processing in a condition that checks the os

Good call, will put something like that together now :) 👍

drvinceknight · 2016-09-21T18:04:57Z

I think this is in good shape for a review now:

Have added an in_memory variable to tournament.play. Default is False, if True it basically just uses an internal interactions dictionary.
Have added a axelrod.on_windows that on loading of the library detects if it's running on a Windows machine.
If running on a windows machine and no filename is provided then in_memory will automatically be set to True. This is because NamedTemporaryFiles are not very useful on Windows (see this known bug: https://bugs.python.org/issue14243).
I have added test skips, basically dropping support for parallel processing on Windows. There were other bugs that @Ducksual found with parallel processing: I have opened Fix parallel processing on windows #718 and suggest that that's fixed as a separate issue.

Have added some tests and also a short documentation tutorial about parallel processing.

I suggest that once this is OK'd and in:

We make a note on Fix parallel processing on windows #718 to say that once it's fixed we fix the docs which in two places would say that parallel processing is not supported on Windows.
We open an issue to refactor ResultSet so that it's as efficient as ResultSetFromFile. Currently it reads the dictionary more often than it needs to. This shouldn't be hard: everything is basically already in place in ResultSetFromFile, that just needs to be tweaked so that it worked with a dictionary.

meatballs · 2016-09-21T18:07:02Z

What about the wrapping of the parallel processing in a condition so that it only happens if not windows?

drvinceknight · 2016-09-21T18:07:57Z

What about the wrapping of the parallel processing in a condition so that it only happens if not windows?

Yup: I missed that (thought I had done it), appveyor just pointed it out. Just pushing that now :)

meatballs · 2016-09-21T18:08:48Z

axelrod/__init__.py

@@ -1,4 +1,7 @@
 from __future__ import absolute_import
+import sys
+
+on_windows = sys.platform.startswith("win")


Any reason you've gone with sys.platform rather than os.name?

Not an educated reason: would os.name be better? I have to check for nt or something like that right?

I've always used os.name == 'nt' based on answers at http://stackoverflow.com/questions/4553129/when-to-use-os-name-sys-platform-or-platform-system#11674977

Cool: I'll change that 👍

but I wondered if you knew something that I don't !!

Nope: I just got scared away by the nt :) Have changed this with 72238a3

meatballs · 2016-09-21T18:26:57Z

Big 👍 from me for this one. That was a painful day....

DavidCEllis · 2016-09-21T18:44:44Z

I don't know what you're talking about. Windows is easy! ;-)

In all seriousness I tried to be fairly careful to clean up the files but it would have been really nice if NamedTemporaryFile actually did something useful in Windows (or if TemporaryDirectory had existed before Python 3.2).

Multiprocessing on windows is a separate nightmare.

drvinceknight · 2016-09-21T18:55:00Z

In all seriousness I tried to be fairly careful to clean up the files but it would have been really nice if NamedTemporaryFile actually did something useful in Windows (or if TemporaryDirectory had existed before Python 3.2).

You had done a great job, and it's highly appreciated. You figured out all the bugs 👍

Multiprocessing on windows is a separate nightmare.

We'll get there eventually (hopefully)!

Either way hopefully this will all be in place so that you could contribute actual Game Theory next :)

DavidCEllis · 2016-09-21T18:58:51Z

I actually checked and NamedDirectory wouldn't have helped (at least in cleaning up the files on windows). As far as I can tell the implementation of NamedTemporaryFile on windows with the locking behaviour is the only way to guarantee the file will be deleted. Without locking something else could open the file and then Windows wouldn't allow it to be removed.

marcharper

Although this is good work, I'm not a fan of this kind of thing -- I doubt many windows users would use the library without a GUI and it makes the code more complex to maintain -- but I won't stand in the way.

Has anyone tested the library with bash on windows? That might be a better way to have some support on windows, and who knows, maybe multiprocessing works?

marcharper · 2016-09-21T18:52:03Z

appveyor.yml

@@ -1,10 +1,11 @@
 environment:
  matrix:
-    - PYTHON: "C:\\Python27"
-    - PYTHON: "C:\\Python34"
+    #- PYTHON: "C:\\Python27"


Can we delete these if they won't be used?

Have pushed that.

marcharper · 2016-09-21T18:53:48Z

axelrod/tests/integration/test_tournament.py

@@ -34,9 +33,9 @@ def test_full_tournament(self):
        tournament = axelrod.Tournament(name='test', players=strategies,
                                        game=self.game, turns=2,
                                        repetitions=2)
-        tmp_file = tempfile.NamedTemporaryFile()
+        filename = "test_outputs/test_tournament.csv"


Are we going to run into issues if the directory doesn't exist?

The directory should exist, I've put a README.md file in there so cloning the library should put it there right?

marcharper · 2016-09-21T18:57:04Z

axelrod/tournament.py

@@ -93,11 +102,15 @@ def play(self, build_results=True, filename=None,
            self.progress_bar = tqdm.tqdm(total=len(self.match_generator),
                                          desc="Playing matches")

-        self.setup_output_file(filename)
+        if on_windows and (filename is None):


So if the tournament takes up a lot of memory on windows, it won't work?

Yup: it it takes up a lot of memory then it will crash. It will work fine if a filename is provided though.

drvinceknight · 2016-09-21T19:06:43Z

I doubt many windows users would use the library without a GUI and it makes the code more complex to maintain

I know of 3 that have (with anaconda, Jupyter etc I actually would hope we would not close the door on windows users). I expect I'll have students doing so too. Important to get it working natively on windows even if it does make it harder for us.

Has anyone tested the library with bash on windows? That might be a better way to have some support on windows, and who knows, maybe multiprocessing works?

That's not really running on Windows though. I expect that if it runs on bash it might not pick up the on_windows flag? Apparently that's a full ubuntu thing? (I have no idea).

meatballs · 2016-09-21T19:12:23Z

I doubt many windows users would use the library without a GUI

Several of the students who came to the sprint were on windows - and I suspect that's often the case for young undergrads coming to university with their shiny new laptops!

DavidCEllis · 2016-09-21T19:23:19Z

Bash on windows is Ubuntu related - but still beta and a little more work to install than Anaconda. I'm not sure if all the Linux features would work on it or not but I'm installing it now so I can find out.

drvinceknight · 2016-09-21T19:28:17Z

Bash on windows is Ubuntu related - but still beta and a little more work to install than Anaconda. I'm not sure if all the Linux features would work on it or not but I'm installing it now so I can find out.

Thanks @Ducksual: that's helpful.

Either way I think that in the interest of EDIT ~~diversity~~ inclusivity we should work to offer native support. This PR is a first step there, we can always open more PRs to improve it and reduce the complexity :)

drvinceknight · 2016-09-21T19:40:48Z

I'm installing it now so I can find out.

@Ducksual it would be interesting to know what the following does on bash for windows...:

>>> import os
>>> os.name

DavidCEllis · 2016-09-21T19:48:05Z

>>> import os
>>> os.name
'posix'
>>>

drvinceknight · 2016-09-21T19:51:05Z

Neat :)

DavidCEllis · 2016-09-21T20:11:52Z

It's kind of neat but a little intimidating if you haven't used linux/bash before. After changing your machine to developer mode and going through the install you still don't have any gui support for linux.

drvinceknight · 2016-09-21T20:45:51Z

It's kind of neat but a little intimidating if you haven't used linux/bash before.

Oh absolutely. Bash for windows isn't really a solution. I was just curious :)

drvinceknight · 2016-09-21T20:49:11Z

Bash for windows isn't really a solution.

Although it might be for some people as it looks like you would be able to run parallel tournaments with it.

It's good to know.

marcharper · 2016-09-21T21:23:17Z

Thanks @Ducksual @drvinceknight @meatballs ! Great work everyone.

drvinceknight added 14 commits September 20, 2016 10:34

Write method to write interactions to dictionary.

012e801

Have initial concept working.

53999c9

Remove keep interactions option from tournament.

417baa2

This has not been used for a while now.

Remove temporary files

6b1e249

Adjustments so that tests pass.

f30beb5

Remove temporary files from tests.

877b989

Remove all temp files in tests.

fe617ee

Remove support for parallel processing on Windows.

4641166

Remove keep interactions for resultsset from file

af41e22

Todo: adjust docs.

Revert "Remove keep interactions for resultsset from file"

3e2fef1

This reverts commit af41e22.

Remove py2.7 and py3.4 from windows CI

552a1ca

Because life is short.

Remove temporary files from cache test.

b094f30

Put keep_interactions back in as an option.

60d8672

Add correct terminator for write summary.

e10adb0

drvinceknight added the in progress label Sep 20, 2016

drvinceknight added 7 commits September 21, 2016 08:53

Add on_windows property

a28ed1e

Make temp file default: switch to mem if win

f58c34b

Close output file on win when not build results.

3463038

Make doctest pass on Windows and not windows.

f73b272

Rename setup_output method.

0342065

Add tests.

ded3721

Add documentation about parallel processing.

8e20f9c

drvinceknight changed the title ~~WIP Do Not Merge (Windows fixes amongst other things)~~ Tournaments can run on Windows Sep 21, 2016

No parallel processing if on windows.

84c6c57

meatballs reviewed Sep 21, 2016

View reviewed changes

drvinceknight added the ready-for-review label Sep 21, 2016

meatballs added 1-positive-review and removed ready-for-review labels Sep 21, 2016

Change sys to os lib for win detection.

72238a3

marcharper reviewed Sep 21, 2016

View reviewed changes

Comment out unused python versions.

506a17e

marcharper merged commit 367a787 into master Sep 21, 2016

This was referenced Sep 22, 2016

Cannot run unit tests on Windows with Python 2.7 #322

Closed

Fix for temporary files on windows. #713

Closed

marcharper deleted the 719 branch October 13, 2016 14:04

drvinceknight mentioned this pull request Apr 16, 2017

refactor for fingerprint #970

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tournaments can run on Windows #721

Tournaments can run on Windows #721

drvinceknight commented Sep 20, 2016

meatballs commented Sep 21, 2016

drvinceknight commented Sep 21, 2016

drvinceknight commented Sep 21, 2016

meatballs commented Sep 21, 2016

drvinceknight commented Sep 21, 2016

meatballs Sep 21, 2016 •

edited

Loading

drvinceknight Sep 21, 2016

meatballs Sep 21, 2016

drvinceknight Sep 21, 2016

meatballs Sep 21, 2016

drvinceknight Sep 21, 2016

meatballs commented Sep 21, 2016

DavidCEllis commented Sep 21, 2016

drvinceknight commented Sep 21, 2016

DavidCEllis commented Sep 21, 2016

marcharper left a comment

marcharper Sep 21, 2016

drvinceknight Sep 21, 2016

drvinceknight Sep 21, 2016

marcharper Sep 21, 2016

drvinceknight Sep 21, 2016

marcharper Sep 21, 2016

drvinceknight Sep 21, 2016

drvinceknight commented Sep 21, 2016

meatballs commented Sep 21, 2016

DavidCEllis commented Sep 21, 2016

drvinceknight commented Sep 21, 2016 •

edited

Loading

drvinceknight commented Sep 21, 2016

DavidCEllis commented Sep 21, 2016 •

edited

Loading

drvinceknight commented Sep 21, 2016

DavidCEllis commented Sep 21, 2016

drvinceknight commented Sep 21, 2016

drvinceknight commented Sep 21, 2016 •

edited

Loading

marcharper commented Sep 21, 2016 •

edited

Loading

Tournaments can run on Windows #721

Tournaments can run on Windows #721

Conversation

drvinceknight commented Sep 20, 2016

meatballs commented Sep 21, 2016

drvinceknight commented Sep 21, 2016

drvinceknight commented Sep 21, 2016

meatballs commented Sep 21, 2016

drvinceknight commented Sep 21, 2016

meatballs Sep 21, 2016 • edited Loading

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

meatballs commented Sep 21, 2016

DavidCEllis commented Sep 21, 2016

drvinceknight commented Sep 21, 2016

DavidCEllis commented Sep 21, 2016

marcharper 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

drvinceknight commented Sep 21, 2016

meatballs commented Sep 21, 2016

DavidCEllis commented Sep 21, 2016

drvinceknight commented Sep 21, 2016 • edited Loading

drvinceknight commented Sep 21, 2016

DavidCEllis commented Sep 21, 2016 • edited Loading

drvinceknight commented Sep 21, 2016

DavidCEllis commented Sep 21, 2016

drvinceknight commented Sep 21, 2016

drvinceknight commented Sep 21, 2016 • edited Loading

marcharper commented Sep 21, 2016 • edited Loading

meatballs Sep 21, 2016 •

edited

Loading

drvinceknight commented Sep 21, 2016 •

edited

Loading

DavidCEllis commented Sep 21, 2016 •

edited

Loading

drvinceknight commented Sep 21, 2016 •

edited

Loading

marcharper commented Sep 21, 2016 •

edited

Loading