-
Notifications
You must be signed in to change notification settings - Fork 1
Add automatic allocation of copies of the share library on demand for multiple copies of players. #77
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
Conversation
… multiple copies of players.
Awesome, this would be great to fully get working as it would make things a lot simpler for the revisiting tournament too :) |
src/axelrod_fortran/player.py
Outdated
self.player_indices = defaultdict(set) | ||
self.player_next = defaultdict(set) | ||
# Generate a random prefix for tempfile generation | ||
self.prefix = str(uuid.uuid4()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't remembery why, but I seem to recall it's safer to use uuid4().hex here instead of str
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the docs here it looks like str(...)
gives the hex value (perhaps previously it was different?). I just need a unique value so we can change to hex
if there are any concerns.
src/axelrod_fortran/player.py
Outdated
self.library_path = self.find_shared_library(shared_library_name) | ||
|
||
def find_shared_library(self, shared_library_name): | ||
## This finds only the relative path to the library, unfortunately. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if the absolute
method of the Pathlib library might be able to help us out here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll look into it. I think the issue is that the library loader doesn't return the absolute path for some reason (though it clearly knows it) so we may need to either look for the file somehow (in a cross platform way) or instruct the user to symlink the file (not ideal).
Maybe if we have an install script for the shared library here we can capture the path at installation time and write to a config file.
Ok, I've got a thread-safe implementation now and a proper tournament runs (see example below). There's also a function to find the absolute path on Linux (which works locally) and various documentation leads me to believe that the absolute path is returned by However, the travis build still can't find the library, and the local tests never finish for me, using "python -m pytest" locally. I'm not as familiar with
|
Nice work @marcharper! I've taken a quick look at running things locally. Here are my steps: I have not done anything at all in To double check things I uninstalled whatever version of the
Then I installed a develop version of the library to be able to test (this did not actually seem necessary, there might be something that takes care of this in the other files in the repo):
Then I ran pytest just be typing:
From the stacktrace I believe some things are working but I'm not entirely sure, I've attached the output to this comment: test.log I'm not sure if that's helpful, when I get a moment I can look in to things a bit more. |
tests/test_player.py
Outdated
@@ -26,6 +28,7 @@ def test_init(): | |||
assert type(player.shared_library) is CDLL | |||
assert "libstrategies.so" in str(player.shared_library) | |||
|
|||
|
|||
def test_init_with_shared(): | |||
player = Player("k42r", shared_library_name="libstrategies.so") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that passing the shared_library_name
argument here will throw an error as it's no longer in the signature for player.init
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. I'll use Vince's example above to get the tests run locally and fix them up.
pytest still doesn't work locally for me but it seems to be some shenanigans with various library versions that I can't seem to sort. In any case it looks like we're in business! |
Nice work @marcharper! Looks good to me, when I get a moment I'll give it a whirl on the reproducing paper repo, should clear up the analysis code nicely :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one minor typo and a suggestion/debate topic from me. Looking really good!
src/axelrod_fortran/player.py
Outdated
@@ -106,6 +113,25 @@ def strategy(self, opponent): | |||
my_last_move) | |||
return actions[original_action] | |||
|
|||
def _release_shared_library(self): | |||
# While this looks like we're checking that the shared library file | |||
# isn't deleted, the exception is actually thrown in the manager |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: in should be if
# Copy the library file to a new (temp) location. | ||
temp_directory = tempfile.gettempdir() | ||
copy_number = len(self.filenames) | ||
new_filename = os.path.join( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we consider using the pathlib libary here? It's a personal preference, but I think it makes this sort of path manipulation cleaner.
def __del__(self): | ||
"""Cleanup temp files on object deletion.""" | ||
for filename in self.filenames: | ||
if os.path.exists(filename): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
again, I think using pathlib would make this a little cleaner
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for pathlib (big fan), if it's a pain, we could merge and refactor?
def load_library(filename): | ||
"""Loads a shared library.""" | ||
lib = None | ||
if os.path.exists(filename): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for consistency, this could now be if Path(filename).exists():
self.prefix, | ||
str(copy_number), | ||
self.shared_library_name) | ||
new_filename = str(Path(temp_directory) / filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
any reason this isn't new_filename = str(Path(temp_directory, filename))
?
if path.exists(): | ||
if self.verbose: | ||
print("deleting", str(path)) | ||
os.remove(str(path)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this could now be path.unlink()
from ctypes import cdll | ||
from ctypes.util import find_library | ||
from multiprocessing.managers import BaseManager | ||
import os |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I think this should now become redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
This PR adds a class to manage multiple copies of the shared library so that multiple copies of a given player can be used without the user having to maintain multiple copies of the shared library themselves. It does this by literally copying the shared library file and reloading it. (Loading the same file repeatedly doesn't work.)
It works as is and I was able to run the Moran Process for several copies of players, but there are two issues: