Clean up game outcome handling #495

Wesmania · 2019-09-14T20:03:26Z

This moves the logic determining game's outcome for players into a separate class. Methods for choosing an outcome from multiple reports are merged into one. TODO: discuss if the method chosen is appropriate.

The PR is based on top of #494, so it has some extra commits.

cleborys · 2019-09-19T20:52:47Z

server/games/game.py

@@ -462,7 +406,7 @@ def host_left_lobby() -> bool:
                    await self.mark_invalid(ValidityState.MUTUAL_DRAW)
                    return

-                if len(self._results) == 0:
+                if not self._results:


When is this True? Does this maybe rely on collections.abc.Mapping.__len__ being nonzero or GameResults.__len__ being nonzero? Or just self._results not being None? I am uncertain how trucity/casting to bool is handled for user-defined class if there is no __bool__.

According to Python docs, in absence of __bool__ truthiness is decided by __len__ being non-zero. So, it's perfectly fine to idiomatically check for non-emptiness.

Good to know, thanks!

cleborys · 2019-09-20T17:33:47Z

server/games/game.py

-        self._logger.info("Conflicting scores (%s) reported for game %s", scores, self)
-        score, _votes = max(scores.items(), key=lambda kv: kv[::-1])
-        return score
+        return self._results.score(army, self.id)

    def get_army_result(self, player):


Maybe this should be called get_player_outcome?

It does return a score (a 'score' value from the game) instead of an outcome ('victory', 'defeat' etc.).

Sorry, I meant get_army_result, which takes a Player as argument and returns a GameOutcome (not get_army_score)

I didn't want to touch Game class public methods, though I could rename that if you want.

It just looked off to me, so I'm all for it. I can do it in my later PR if you don't want to :)

Brutus5000 · 2019-09-20T19:33:22Z

If you want to clean up game outcomes please write into the db table player_stats into a new column an enum of type VICTORY, DRAW, DEFEAT. That would save me froma lot of trouble in the future

cleborys · 2019-09-21T13:23:48Z

server/games/game_results.py

+                return False
+        return True
+
+    def outcome(self, army: int) -> GameOutcome:


Maybe get_outcome, making it a verb?

It returns a value without side effects, so I think it's up to personal taste.

Askaholic · 2019-09-21T22:13:38Z

server/games/game_results.py

+
+        if len(outcomes) == 1:
+            return outcomes.pop()
+        else:


Can drop the else

The else clause is a one-liner, so I thought it's clearer to keep it.

Now that it's not a one liner you should probably get rid of the else. It's just an unnecessary level of indentation

Askaholic · 2019-09-21T22:21:13Z

server/games/game_results.py

+    async def from_db(cls, database, game_id):
+        results = cls()
+        async with database.acquire() as conn:
+            rows = await conn.execute(


Could rewrite this with sqlalchemy models

Sure. I kept it to make it easier to verify that the code is equivalent, we can change that in a separate commit.

server/games/game_results.py

Askaholic · 2019-09-21T23:42:15Z

If you want to clean up game outcomes please write into the db table player_stats into a new column an enum of type VICTORY, DRAW, DEFEAT. That would save me froma lot of trouble in the future

Yea, we should really add that. Otherwise the only way of knowing how the server rated a game, is to run the exact version of the rating code that was deployed at the time against the same game results. And as we know, that rating code is flawed at the moment

Askaholic

We need at least a few test cases for persist_results, and also a test for this code path:
https://coveralls.io/builds/25850926/source?filename=server%2Fgames%2Fgame.py#L771.

I think one solution to the unknown game results could be to simply make the game unranked if we are in a situation where the winner can't be determined.

server/games/game_results.py

Askaholic · 2019-10-20T21:05:17Z

tests/unit_tests/test_game.py

@@ -405,6 +406,8 @@ def custom_game(event_loop, database, game_service, game_stats_service):
            assert new_rating != Rating(*player.ratings[RatingType.GLOBAL])


+# FIXME - discuss correct get_army_result behaviour


So this test now fails because the first result (with one reported victory and one reported defeat) now is reported as an unknown outcome? It might be a good idea to unrank the game due to UNKNOWN_RESULT in this case as well.

I'll change the test to do that.

Askaholic · 2019-10-20T21:13:16Z

server/games/game_results.py

+        Pick and return most frequently reported score for an army. If multiple
+        scores are most frequent, pick the largest one.
+        """
+        if army not in self:


I think some pieces of the code actually expect this to raise a KeyError. Have a look at:

server/server/games/game.py

Line 515 in 6b9aec7

except KeyError:

and

server/server/games/game.py

Line 897 in 6b9aec7

except KeyError:

It even mentions it in the docstring

server/server/games/game.py

Line 841 in 6b9aec7

:raise KeyError

We need some tests for persist_results because I don't think it ever writes -1 into the database any more. As a matter of fact that code path is not covered by the tests: https://coveralls.io/builds/25850926/source?filename=server%2Fgames%2Fgame.py#L449
which is probably why I missed this when I rewrote that function in the first place

I think I won't restore these KeyErrors and will just fix these catch blocks - we have good neutral values (0, and GameOutcome.UNKNOWN) for when an army isn't there. It also makes sense in that it's not an exceptional condition - we might have not received any results for a given army yet, so we need a sensible value for an army with no results.

Yea I agree with that approach. Like I mentioned, the code used to write -1 for unknown scores, although I'm not convinced that this is unambiguous since negative scores are possible. Is it possible to have a score of 0?

Wesmania

I'll add some unit tests tomorrow, as you asked.

server/games/game_results.py

Wesmania · 2019-10-27T21:33:56Z

tests/unit_tests/test_game.py

@@ -405,6 +406,8 @@ def custom_game(event_loop, database, game_service, game_stats_service):
            assert new_rating != Rating(*player.ratings[RatingType.GLOBAL])


+# FIXME - discuss correct get_army_result behaviour


I'll change the test to do that.

Wesmania · 2019-10-27T21:47:04Z

server/games/game_results.py

+        Pick and return most frequently reported score for an army. If multiple
+        scores are most frequent, pick the largest one.
+        """
+        if army not in self:


I think I won't restore these KeyErrors and will just fix these catch blocks - we have good neutral values (0, and GameOutcome.UNKNOWN) for when an army isn't there. It also makes sense in that it's not an exceptional condition - we might have not received any results for a given army yet, so we need a sensible value for an army with no results.

server/games/game_results.py

Askaholic · 2019-11-23T08:00:07Z

I think this PR is very close to being ready, but I'd still like to see a regression test that verifies unknown results are reported as score -1 in the database. This is mostly so that the next time someone changes the code and accidentally modifies that behaviour, at least they will know.

Also I think you have a few FIXME' comments that can be removed now, as well as a now extraneous else statement.

This slims down the Game class a bit and lets us see more clearly how we're correlating game results. Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

Wesmania · 2019-11-24T17:00:40Z

Now that I look at the old code, I'm somewhat confused. Originally -1 was entered when get_army_score threw a KeyError, but I can't see how that could happen in old code. It seems to me we never hit that '-1' branch, and that I should modify the new get_army_score to just return 0 when there are no results.

Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

We'll keep the strict method, but we'll also add logging to gauge how often we get conflicting results. Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

* Move game outcome logic to a separate class This slims down the Game class a bit and lets us see more clearly how we're correlating game results. Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com> * Use only player id as game result reporter Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com> * Use one method for deciding player's game outcome Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com> * Remove unused exception catch Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com> * Agree on game outcome calculating method We'll keep the strict method, but we'll also add logging to gauge how often we get conflicting results. Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com> * Remove obsolete FIXMEs Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com> * Add tests for player scores, consistently use 0 as default Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

cleborys reviewed Sep 19, 2019

View reviewed changes

cleborys reviewed Sep 20, 2019

View reviewed changes

cleborys reviewed Sep 21, 2019

View reviewed changes

cleborys mentioned this pull request Sep 21, 2019

Refactor Game.compute_rating #496

Merged

Askaholic reviewed Sep 21, 2019

View reviewed changes

Wesmania force-pushed the fix-game-outcomes branch from 1577fd2 to 53cb44a Compare September 21, 2019 23:35

Askaholic requested changes Oct 20, 2019

View reviewed changes

Wesmania commented Oct 27, 2019

View reviewed changes

Wesmania force-pushed the fix-game-outcomes branch from 53cb44a to 8507a70 Compare October 28, 2019 21:44

Askaholic reviewed Nov 23, 2019

View reviewed changes

server/games/game_results.py Outdated Show resolved Hide resolved

Igor Kotrasinski added 3 commits November 24, 2019 00:50

Move game outcome logic to a separate class

5ff130a

This slims down the Game class a bit and lets us see more clearly how we're correlating game results. Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

Use only player id as game result reporter

e75e21c

Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

Use one method for deciding player's game outcome

c27bd3a

Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

Wesmania force-pushed the fix-game-outcomes branch from 8507a70 to 726b697 Compare November 23, 2019 23:50

Wesmania force-pushed the fix-game-outcomes branch from 726b697 to 27f8009 Compare November 24, 2019 19:19

Askaholic mentioned this pull request Nov 26, 2019

Add column for game outcome FAForever/db#208

Merged

Igor Kotrasinski added 4 commits November 27, 2019 00:11

Remove unused exception catch

f3e68d6

Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

Agree on game outcome calculating method

be8eff3

We'll keep the strict method, but we'll also add logging to gauge how often we get conflicting results. Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

Remove obsolete FIXMEs

44b5b6a

Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

Add tests for player scores, consistently use 0 as default

7a1b59e

Signed-off-by: Igor Kotrasinski <i.kotrasinsk@gmail.com>

Wesmania force-pushed the fix-game-outcomes branch from e9788ad to 7a1b59e Compare November 26, 2019 23:12

Askaholic merged commit e6ac71f into FAForever:develop Dec 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean up game outcome handling #495

Clean up game outcome handling #495

Wesmania commented Sep 14, 2019

cleborys Sep 19, 2019

Wesmania Sep 21, 2019

cleborys Sep 21, 2019

cleborys Sep 20, 2019

Wesmania Sep 21, 2019

cleborys Sep 21, 2019 •

edited

Loading

Wesmania Sep 22, 2019

cleborys Sep 22, 2019

Brutus5000 commented Sep 20, 2019

cleborys Sep 21, 2019

Wesmania Sep 21, 2019

Askaholic Sep 21, 2019

Wesmania Sep 22, 2019

Askaholic Nov 23, 2019

Askaholic Sep 21, 2019

Wesmania Sep 22, 2019

Askaholic commented Sep 21, 2019

Askaholic left a comment

Askaholic Oct 20, 2019

Wesmania Oct 27, 2019

Askaholic Oct 20, 2019

Wesmania Oct 27, 2019

Askaholic Oct 28, 2019

Wesmania left a comment

Wesmania Oct 27, 2019

Wesmania Oct 27, 2019

Askaholic commented Nov 23, 2019

Wesmania commented Nov 24, 2019

		@@ -405,6 +406,8 @@ def custom_game(event_loop, database, game_service, game_stats_service):
		assert new_rating != Rating(*player.ratings[RatingType.GLOBAL])


		# FIXME - discuss correct get_army_result behaviour

Clean up game outcome handling #495

Clean up game outcome handling #495

Conversation

Wesmania commented Sep 14, 2019

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

cleborys Sep 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Brutus5000 commented Sep 20, 2019

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

Askaholic commented Sep 21, 2019

Askaholic 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

Wesmania left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Askaholic commented Nov 23, 2019

Wesmania commented Nov 24, 2019

cleborys Sep 21, 2019 •

edited

Loading