Rewrite ProtoLogger in C++

nimazareian commented 6 days ago

Description

Currently, ProtoLogger (used for saving all protobufs so we can replay them at a later time) is written in Python, running as part of the Thunderscope process. Given the nature of Python being a single core program (can read about Python GIL), the Thunderscope process would spend a significant amount of its time not updating GUI and saving the large amount of protobufs we have with ProtoLogger. In this PR, I have re-written ProtoLogger to be in C++ as part of our FullSystem AI process. This allows us to have true parallelism and help improve the performance of Thunderscope significantly on lower-end devices.

Some notable improvements:

there are no extra protobuf serializations happening. The serialized protobuf messages that we send/receive are directly stored in the replay file, avoiding extra conversions. Previous implementation would receive serialized protobufs, deserialize them, send them to ProtoLogger, and then serialize them again.
ProtoLogger is given up to 0.5 sec to flush its buffer. Previously, we would abruptly shutdown ProtoLogger without flushing its buffer, causing it to possibly not log any protobufs it had received towards the end of the test. This was specially a problem in simulated tests that were ran without Thunderscope (i.e. ran much faster than realtime and quickly shutdown, not giving time to log most of the test).
The first line of all new replay files has some metadata about the file format version. This should allow us to have the current implementation be able to read old replay files, while also giving us the freedom to safely make breaking changes in the future regarding how the replay files are formatted while also keeping backwards compatibility.
The path to the replay file is always printed. Previously it was only printed at the end of pytests, but now it's also printed when running AI vs AI or playing an actual game.

With the new ProtoLogger being part of Full System, we can theoretically even support replays for simulated C++ tests. Though, currently the way the logger is initialized for GTests and simulated tests is a bit weird (initialized twice), making it a bit difficult to pass ProtoLogger to the logger. So I decided to skip this for now.

Testing Done

Ran AI vs AI and simulated pytests (with and without Thunderscope) and watched its replay back without any issues.

Resolved Issues

Should improve Thunderscope's refresh rate on computers that previously struggled with ProtoLogger taking too much time.

Length Justification and Key Files to Review

proto_logger.h/cpp unix_full_system_main.cpp unix_simulator_backend.h/cpp full_system.py

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

[ ] Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
[ ] Remove all commented out code
[ ] Remove extra print statements: for example, those just used for testing
[ ] Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue

Mr-Anyone commented 4 days ago

After tinkering a little bit with the build system, I've managed to separate the LOG and LoggerSingleton As such, LOG basically only depends on 3rd party code which solves the circular import problem.

Honestly, LoggerSingleton and our version of LOG shouldn't be in the same header file, which was the cause of this problem.

We can now call LOG inside proto_logger.cpp and thread_safe_buffer.cpp.

The branch is tested on the following build targets:

./tbots.py run thunderscope
./tbots.py build thunderloop
./tbots.py build er_force_simulator_main
./tbots.py build simulated_er_force_sim_test_fixture
./tbots.py build tbots_gtest_main

https://github.com/Mr-Anyone/Thunderbot_Software/tree/fullsystem_proto_logger_buildsystem

There are 24 files changed, 243 insertions(+), 159 deletions(-). I am not sure if you would want to rebase that branch into this PR.

nimazareian commented 3 days ago

@Mr-Anyone Okay that's great! I think since this PR is already somewhat long and there's been a few revisions of reviews already, we can open a PR with your changes once this is merged.

nimazareian commented 1 day ago

Just noticed that RobotLog and RobotCrash protos received by RobotCommunication were not being logged by ProtoLogger since FullSystem did not receive those values. They've has now been added.

UBC-Thunderbots / Software