Skip to content
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

Add config option to specify json float serialization precision #905

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
rev: v4.0.1
rev: v4.2.0
hooks:
- id: trailing-whitespace
- id: end-of-file-fixer
- id: check-docstring-first
- repo: https://github.com/PyCQA/flake8
rev: 3.9.2
rev: 4.0.1
hooks:
- id: flake8
- repo: https://github.com/pre-commit/mirrors-autopep8
rev: v1.5.7
rev: v1.6.0
hooks:
- id: autopep8
- repo: https://github.com/PyCQA/isort
rev: 5.9.3
rev: 5.10.1
hooks:
- id: isort
5 changes: 5 additions & 0 deletions server/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ def __init__(self):
# How many previous queue sizes to consider
self.QUEUE_POP_TIME_MOVING_AVG_SIZE = 5

# Whether floats should be rounded before json encoding
self.JSON_ROUND_FLOATS = True
# The maximum number of decimal places to use for float serialization
self.JSON_ROUND_FLOATS_PRECISION = 2

self._defaults = {
key: value for key, value in vars(self).items() if key.isupper()
}
Expand Down
22 changes: 21 additions & 1 deletion server/protocol/protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,30 @@
from asyncio import StreamReader, StreamWriter

import server.metrics as metrics
from server.config import config

from ..asyncio_extensions import synchronizedmethod

json_encoder = json.JSONEncoder(separators=(",", ":"))

class CustomJSONEncoder(json.JSONEncoder):
# taken from https://stackoverflow.com/a/53798633
def encode(self, o):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this way the best out of all the ones proposed so far. Have you measured the performance difference for this one? I imagine it would be noticeable especially for dicts since it makes a copy of the entire data structure, but maybe it's not too bad.

I think the other way that would have merit would be to make a wrapper type around float and use default to format it. That would require us to explicitly set the float rounding everywhere, but it would also give more control if we wanted to round rating to 3 or 4 places but timestamps only to 2.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh sorry, I see this is the 'round floats' approach you mentioned in your other comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't quite understand what do you mean by a wrapper, because if it requires us to call this wrapper every time we do some math, then why don't just use round?
Rewriting representation of the float won't help, the C code will operate with value and encode all the digits

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something similar to this, but using our own class instead of Decimal: https://stackoverflow.com/a/3885198

class PFloat:
    def __init__(self, value: float, precision: int):
        self.value = value
        self.precision = precision

And then in the to_dict method you'd have to wrap the floats in this class:

def to_dict(self):
    return {
        "some_float": PFloat(self.some_float, 2)
    }

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I can make a class, but I think it is unnecessary complication, because we already have built-in round function which effectively does the same thing

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Like, we have 2 options:

  1. Rewrite json's encode method to change encoding of all floats
  2. Rewrite every to_dict method where we will round every float with its own precision (the "more control" mentioned above)

def round_floats(o):
if isinstance(o, float):
return round(o, config.JSON_ROUND_FLOATS_PRECISION)
if isinstance(o, dict):
return {k: round_floats(v) for k, v in o.items()}
if isinstance(o, (list, tuple)):
return [round_floats(x) for x in o]
return o

if config.JSON_ROUND_FLOATS:
return super().encode(round_floats(o))
else:
return super().encode(o)


json_encoder = CustomJSONEncoder(separators=(",", ":"))


class DisconnectedError(ConnectionError):
Expand Down
4 changes: 2 additions & 2 deletions server/servercontext.py
Original file line number Diff line number Diff line change
Expand Up @@ -132,8 +132,8 @@ async def client_connected(self, stream_reader, stream_writer):
asyncio.CancelledError,
):
pass
except Exception:
self._logger.exception()
except Exception as e:
self._logger.exception(e)
Askaholic marked this conversation as resolved.
Show resolved Hide resolved
finally:
del self.connections[connection]
# Do not wait for buffers to empty here. This could stop the process
Expand Down
8 changes: 6 additions & 2 deletions tests/integration_tests/test_game.py
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,10 @@ async def test_game_ended_rates_game(lobby_server):

@pytest.mark.rabbitmq
@fast_forward(30)
async def test_game_ended_broadcasts_rating_update(lobby_server, channel):
async def test_game_ended_broadcasts_rating_update(
lobby_server, channel, mocker,
):
mocker.patch("server.config.JSON_ROUND_FLOATS_PRECISION", 4)
mq_proto_all = await connect_mq_consumer(
lobby_server,
channel,
Expand Down Expand Up @@ -611,12 +614,13 @@ async def test_partial_game_ended_rates_game(lobby_server, tmp_user):


@fast_forward(100)
async def test_ladder_game_draw_bug(lobby_server, database):
async def test_ladder_game_draw_bug(lobby_server, database, mocker):
"""
This simulates the infamous "draw bug" where a player could self destruct
their own ACU in order to kill the enemy ACU and be awarded a victory
instead of a draw.
"""
mocker.patch("server.config.JSON_ROUND_FLOATS_PRECISION", 13)
player1_id, proto1, player2_id, proto2 = await queue_players_for_matchmaking(lobby_server)

msg1, msg2 = await asyncio.gather(*[
Expand Down
45 changes: 45 additions & 0 deletions tests/unit_tests/test_protocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
QDataStreamProtocol,
SimpleJsonProtocol
)
from server.protocol.protocol import json_encoder


@pytest.fixture(scope="session")
Expand Down Expand Up @@ -256,3 +257,47 @@ async def test_read_when_disconnected(protocol):

with pytest.raises(DisconnectedError):
await protocol.read_message()


def test_json_encoder_float_serialization():
assert json_encoder.encode(123.0) == "123.0"
assert json_encoder.encode(0.99) == "0.99"
assert json_encoder.encode(0.999) == "1.0"


@given(message=st_messages())
def test_json_encoder_encodes_server_messages(message):
new_encode = json_encoder.encode
old_encode = json.JSONEncoder(separators=(",", ":")).encode

assert new_encode(message) == old_encode(message)


def st_dictionaries():
value_types = (
st.booleans(),
st.text(),
st.integers(),
st.none(),
)
key_types = (*value_types, st.floats())
return st.dictionaries(
keys=st.one_of(*key_types),
values=st.one_of(
*value_types,
st.lists(st.one_of(*value_types)),
st.tuples(st.one_of(*value_types)),
)
)


@ given(dct=st_dictionaries())
def test_json_encoder_encodes_dicts(dct):
old_encode = json.JSONEncoder(separators=(",", ":")).encode
new_encode = json_encoder.encode

assert new_encode(dct) == old_encode(dct)

wrong_dict_key = (1, 2)
with pytest.raises(TypeError):
json_encoder.encode({wrong_dict_key: "a"})