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 States Navigator to UI #279

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

HassanJbara
Copy link
Contributor

@HassanJbara HassanJbara commented Jul 9, 2024

For visiting old game states, it goes without saying that they need to be saved somewhere. However, saving the hundreds of game states of each game is very costly if we were to use a db, even with optimizations, and the db is unnecessary overhead in my opinion. Therefore:

1- Changes to the flask server:

  • remove db service from docker-compose
  • remove sql code and replace it with code for reading and writing pickle files
  • add debugging apis: /info, /games/<string:game_id>/info

2- Changes to main script:

  • use pickle files for storing game states instead of a database for reduced overhead
  • rename --db cli option to --pickle
  • rename DatabaseAccumulator to PickleAccumulator
  • remove StepDatabaseAccumulator

3- Changes to the react app:

  • add navigation buttons to ActionsToolbar for previous and next states
  • add state counter and state picker

This is the best way of doing this in my opinion, but I would love to hear what you think.

closes #54.

Copy link

netlify bot commented Jul 9, 2024

👷 Deploy request for catanatron-staging pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit d2f5d07

Copy link
Owner

@bcollazo bcollazo left a comment

Choose a reason for hiding this comment

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

Hey! Thanks for thinking about this! I think the Pickle+Filesystem idea is interesting.

Let's not remove the database infrastructure because its needed to deploy catanatron as a web app in the cloud (most application deployment solutions out-of-the-box provide ephemeral filesystems), plus might as well keep it as an option for users.

Can you thus instead make this pull request just add the --pickle option to use the filesystem for persistance (in addition to --json, --db, ...)? Also, I'd like for Pull Requests to be small and single-focused, so I'd say lets first add the --pickle backend, and then we worry about the Frontend. It can be in a separate PR if it helps. Let's also not change the default use of it to be pickle just yet; again lets present it as a new option.

How much faster is using the filesystem than the database?

@HassanJbara
Copy link
Contributor Author

Hey! Thanks for thinking about this! I think the Pickle+Filesystem idea is interesting.

Let's not remove the database infrastructure because its needed to deploy catanatron as a web app in the cloud (most application deployment solutions out-of-the-box provide ephemeral filesystems), plus might as well keep it as an option for users.

Can you thus instead make this pull request just add the --pickle option to use the filesystem for persistance (in addition to --json, --db, ...)? Also, I'd like for Pull Requests to be small and single-focused, so I'd say lets first add the --pickle backend, and then we worry about the Frontend. It can be in a separate PR if it helps. Let's also not change the default use of it to be pickle just yet; again lets present it as a new option.

How much faster is using the filesystem than the database?

I also try to keep pull requests as small as possible, but the reason I grouped these together is because implementing state-navigation requires saving all intermediary states, which is with a db simply not feasible. Using pickle requires practically no overhead, in other words it runs almost as fast as no saving at all (you can try it on the pr branch). Using a db, however, is so slow it's unusable (>2s per state).

As for the web app, I can find a way to keep the app itself connected to a db, yet I ultimately believe the focus should be on training a catan AI first, rather than optimising a web app.

I'll look into breaking this into a pickle specific PR first.

@pachewise
Copy link
Contributor

what's the performance of pickling the game vs some other serialization format? (ideally one that doesn't allow arbitrary code execution)

@HassanJbara
Copy link
Contributor Author

what's the performance of pickling the game vs some other serialization format? (ideally one that doesn't allow arbitrary code execution)

I haven't tested different methods, you can suggest something if you know. Again, given that this is a research project, security isn't the main concern and anyone using the script should know what they are doing.

@zarns
Copy link
Contributor

zarns commented Nov 16, 2024

Feels like we should lean away from pickling as a long-term solution bc it allows arbitrary code execution. I haven't messed around with the db much, but it seems like that would be the better direction to take. Anybody have a clearer idea on what the scope would be to make that happen?
Also, in order to test those changes locally, could you run the db instance thru Docker and run the front/backend from a terminal? Or is it necessary to use docker-compose to work on them locally?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow to "replay games" in UI
4 participants