Change namespace and add adapter

[gaffney2010]

Sorry for a big change without fully discussing first. I wanted to show how I was thinking about this, so I'm hoping to use this as a strawman for discussion, and am willing to go a different direction.

The main changes here are to move namespace like discussed in #1175 and to build an adapter, as I did in ipd_adapter.py. This is with an eye towards the 5.0.0 change.

The adapter is currently a trivial pass-through except for Player which needs some work because of the way we inherit the Player class for different strategies. The idea behind the adapter is to keep the existing public API while allowing us to change the API internally. If for example, we want BasePlayer/IpdPlayer to take History on play instead of opponent, we make that change and can have Player continue to just take opponent; the Player class would keep track of history and pass it to the IpdPlayer it holds internally. This allows us to make incremental, piecewise changes to the API. I think the API changes we'll need to make for 5.0.0 are too numerous to fully anticipate or change all at once, and not being able to change little-by-little can be paralyzing; at the same time we don't want to be repeatedly changing the API. It was suggested that we keep a separate branch, but I think it can be too cumbersome to maintain both branches as we make changes to the rest of the code.

For Player, Game, Match, and Tournament, I made base versions that are just unimplemented API prototypes and left the inherited Ipd version to implement. This may be incorrect, because we may find that we need Base/Ipd of things like Action and History too. On the other hand, we may find that we only need a single version of Match and Tournament, with no inheriting. I did these few to establish a pattern, but we're able to change this however is needed. I am trying to suggest that we attempt a minimal viable product, from which we can start adding other games. I think things likes Moran processes or fingerprinting can be generalized, but those can be done later.

This isn't ready to be submitted. I still need to write unittests for the adapter, but was hoping for some initial feedback in the meantime. As well I left a shell script, which will help me re-merge on top of other outstanding changes that we'll want to submit first; I'll delete this later.

[drvinceknight]

The main changes here are to move namespace like discussed in #1175 and to build an adapter, as I did in ipd_adapter.py. This is with an eye towards the 5.0.0 change.

The adapter is currently a trivial pass-through except for Player which needs some work because of the way we inherit the Player class for different strategies. The idea behind the adapter is to keep the existing public API while allowing us to change the API internally. If for example, we want BasePlayer/IpdPlayer to take History on play instead of opponent, we make that change and can have Player continue to just take opponent; the Player class would keep track of history and pass it to the IpdPlayer it holds internally. This allows us to make incremental, piecewise changes to the API. I think the API changes we'll need to make for 5.0.0 are too numerous to fully anticipate or change all at once, and not being able to change little-by-little can be paralyzing; at the same time we don't want to be repeatedly changing the API. It was suggested that we keep a separate branch, but I think it can be too cumbersome to maintain both branches as we make changes to the rest of the code.

I understand this point of view but the library is used (for example 90 students in my final year game theory class use it) and API changes would lead to problems. Probably leading to supporting people which I don't have the time to do more than I already to. So I'm quite firm on this, API changes with a view to 5.0.0 should be on a separate branch (which we can continuously rebase to master if we want). Especially if they're going to be incremental and we're not 100% sure of them at the time of making them.

Given that, in terms of feedback on this, there are 194 file changes. Any particular one you'd like someone to look at?

[gaffney2010]

API changes would lead to problems

What I'm advocating for here is to make it so that the API would not change; I agree that we don't want to affect the users of the library. I'm making a class called IpdPlayer, then changing Player to a class with all the same methods as IpdPlayer, which also holds an instance of IpdPlayer, and forwards calls. This would allow us to change the API on IpdPlayer piecemeal, while keeping the API on Player the same. As we change the API on IpdPlayer, we will need to add some translation in the Player class logic, but the public API will never change.

Given that, in terms of feedback on this, there are 194 file changes. Any particular one you'd like someone to look at?

This looks like more than it is, because it's mostly just moving to a new folder. ipd_adapter.py has the adapter logic that I'm trying to describe.

Thanks!!

[drvinceknight]

Ok I'll take a look when I get time to work through it all.

[gaffney2010]

there are 194 file changes

I didn't realize that this would be so difficult to parse in git; I thought there would be a way to diff against the old file. I was really just trying to show my idea for splitting Player (etc.) into BasePlayer/IpdPlayer. But I can move the files back to their old location (with BasePlayer/IpdPlayer side-by-side in /axelrod/), and it will be easier to make sense of.

[marcharper]

My 2 cents and a question

• Since Vince and students are heavy active users, we should prioritize not breaking their use cases, or Vince can fix a version

• In principle, it's ok with me if we "mock the existing API" while we make background changes, then remove the API mocking layer on the 5.0 release. This is a decent compromise IMO to make changes to the master branch without having master and a 5.0 branch get too out of sync, but let's be sure to document it all somewhere, maybe in an upcoming API changes file (for others and for us when we actually get around to removing the API layers). What we should actually do depends on the answer to the question below.

Question: What parts of the API shouldn't be changed until 5.0, i.e. need to be mocked? For example, #1288 moves play(...) away from Player and into the Match class. That might break some of Vince's course materials (of course we could mock it as well). Are the students basically just using the library or are they making changes to any of the "internals"?

[gaffney2010]

Since Vince and students are heavy active users, we should prioritize not breaking their use cases

I agree. I wouldn't want to change the API for any users. I'll add tests to make sure the components I changed names of still work with their old name for all the public API; these tests would fail if we ever accidentally broke the API with a future change. Maybe a potential concern: If anyone imports axelrod.xyz, this would break because it would have to be axelrod.ipd.xyz. (I'm not sure how to prevent this, but I think it's possible.) Anything that just does "import axelrod as axl" would still work, because I kept all the same imports in init.

In principle ok with me if we "mock the existing API" while we make background changes, then remove the API mocking layer on the 5.0 release. This is a decent compromise IMO to make changes to the master branch without having master and a 5.0 branch get too out of sync

👍

Question: What parts of the API shouldn't be changed until 5.0, i.e. need to be mocked? For example, #1288 moves play(...) away from Player and into the Match class.

For changes I make, I would try to err on the side of caution, and make everything backward compatible.

[drvinceknight]

Since Vince and students are heavy active users, we should prioritize not breaking their use cases

I agree. I wouldn't want to change the API for any users. I'll add tests to make sure the components I changed names of still work with their old name for all the public API; these tests would fail if we ever accidentally broke the API with a future change. Maybe a potential concern: If anyone imports axelrod.xyz, this would break because it would have to be axelrod.ipd.xyz. (I'm not sure how to prevent this, but I think it's possible.) Anything that just does "import axelrod as axl" would still work, because I kept all the same imports in init.

In principle ok with me if we "mock the existing API" while we make background changes, then remove the API mocking layer on the 5.0 release. This is a decent compromise IMO to make changes to the master branch without having master and a 5.0 branch get too out of sync

👍

For changes I make, I would try to err on the side of caution, and make everything backward compatible.

This all sounds good, I'd misunderstood what you wrote @gaffney2010 (my mistake, sorry). I'm completely happy to mock the API. Essentially, from my pov as long as doctests don't break I'm probably happy :)

[drvinceknight]

Maybe a potential concern: If anyone imports axelrod.xyz, this would break because it would have to be axelrod.ipd.xyz. (I'm not sure how to prevent this, but I think it's possible.) Anything that just does "import axelrod as axl" would still work, because I kept all the same imports in init.

If and when these specific things occur (I believe from an API pov it's mainly the interaction utils that might be used) let's chat about them EDIT and use the doctests as guidance for discussion. Apologies again for misunderstanding your original comment.

[gaffney2010]

I'm going to close this for now. I will finish adding the tests, and reopen.

I will put off changing the namespace until later which will make this easier to review, and avoid changing any import behavior.

Thanks both for the preliminary review of the idea.

[drvinceknight]

Would the idea be (for now from the pov of keeping the current API) to change this to:

Suggested change

	from axelrod.ipd.tournament import IpdTournament
	import axelrod.ipd.tournament.IpdTournament as Tournament

Or perhaps it would be nicer to have:

axelrod.ipd.tournament.IpdTournament

as

axelrod.ipd.tournament.Tournament

As the namespace takes care of the tournament? So theoretically we would also have:

axelrod.<other_game>.tournament.Tournament

At some point?

[drvinceknight]

And then similarly for the IpdMatch? Perhaps just have axelrod.ipd.match.Match? Which makes things easier now but I believe also later as when we have multiple game types a Match is always the name of the object needed and the namespace takes care of behaviour?

[marcharper]

I'm not sure we need a new Match for each game -- maybe a generic Match class could work? Same for Tournaments -- maybe these classes don't need to know about the Game and the Players, they just need to be handed Players and a compatible Game.

[drvinceknight]

That would be great!

[drvinceknight]

I'm going to close this for now. I will finish adding the tests, and reopen.

I will put off changing the namespace until later which will make this easier to review, and avoid changing any import behavior.

Thanks both for the preliminary review of the idea.

Cool, left one comment above regarding naming/namespace. Happy to discuss that here or elsewhere.