Skip to content

Catanatron Rust Rewrite#288

Open
bcollazo wants to merge 88 commits intomasterfrom
feature/catanatron-rust
Open

Catanatron Rust Rewrite#288
bcollazo wants to merge 88 commits intomasterfrom
feature/catanatron-rust

Conversation

@bcollazo
Copy link
Copy Markdown
Owner

This is a rewrite of catanatron_core in Rust.

I decided to take the engine a little bit more seriously, and (inspired by the best chess-engines) for that it sounds we have to go compiled. Tried out a C++ version but enjoyed Rust better.

There's probably many not-so-great Rust practices in this code as I am learning Rust from scratch to do this. If you identify improvements, please share! Will keep this PR in DRAFT and open until it is in a stable and complete state.

Initial experiments suggests the Rust version might be at least 20x faster than the Python version. I am mainly interested in seeing how the MCTS algorithm performs with a lot more rollouts, and also how does Expected-Minimax / Stochastic Alpha-Beta pruning performs with a much deeper look-ahead. Lastly, I think for all the ML/DL approaches scale will be needed, and so being able to run a lot of rollouts sounds like could be helpful (it looks like we can load models in rust for inference and thus rollouts: https://github.com/tensorflow/rust).

@netlify
Copy link
Copy Markdown

netlify bot commented Oct 19, 2024

Deploy Preview for catanatron-staging canceled.

Name Link
🔨 Latest commit 7decbfa
🔍 Latest deploy log https://app.netlify.com/sites/catanatron-staging/deploys/67c7b660a65ef30008de4912

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 19, 2024

Pull Request Test Coverage Report for Build 13667152376

Details

  • 80 of 111 (72.07%) changed or added relevant lines in 8 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-1.9%) to 94.05%

Changes Missing Coverage Covered Lines Changed/Added Lines %
catanatron_core/catanatron/models/map_template.py 38 39 97.44%
catanatron_core/catanatron/test_rust_game.py 0 30 0.0%
Totals Coverage Status
Change from base Build 11872894353: -1.9%
Covered Lines: 1391
Relevant Lines: 1479

💛 - Coveralls

Copy link
Copy Markdown
Collaborator

@tonypr tonypr left a comment

Choose a reason for hiding this comment

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

Notes:

  • copy benchmark might need revisiting.

Comment on lines +60 to +61
let copied_struct1 = black_box(state.clone());
black_box(copied_struct1);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

black_box is applied twice here, should it be just once?

Comment on lines +317 to +319
} else {
panic!("Something went wrong");
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Might using match instead of the if/else if conditionals allow you to remove this panic statement?

c.bench_function("Copy 600-length array", |b| {
let mut iteration_number = 0;
b.iter(|| {
let mut copied_array = array;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Is there a missing .clone() call here?

i.e. Should this be
let mut copied_array = array.clone();
?

@zarns
Copy link
Copy Markdown
Contributor

zarns commented Feb 15, 2025

Now comes the tricky part, 🚀 integrating the Rust code into the Python server/players. pyo3/maturin seem like the correct route forward. We probably want some of the AI players to stay in Python, but having some players rewritten in Rust may make sense. If some of the players are going to use pytorch/sb3, then I'd be hesitant to rewrite the server in Actix or something like that. Trying to figure out how pyo3/maturin work to see where the boundaries between Rust/Python should belong.
Not sure if there's a good way to retain the catanatron_core unit tests without a real overhaul.
I'm sure there are logic flaws in my Rust impl, but feeling it'll be easier to identify issues once the bindings are in place and I can test the game flow

@bcollazo
Copy link
Copy Markdown
Owner Author

Hey Zarns, makes sense! Without much investigation I'd be hesitant to rewrite the web server code (since that part doesn't at a glance seem relevant for what I envisioned Rust set out to do which was faster rollouts and deeper alpha beta search).

For re-using the unit tests... Could we create a library with a similar (almost identical, and willing to change some of the Python one if it'd make it easier) API to the catanatron (core) package that we can "pip install" and re-use the test suite? Its also ok if we can only easily enjoy part of the test suite as well with some flagging of sorts (like running pytest --rust-ready or so, like we do to just run pytest --integration-tests or so). The wrapper package can have some stubs/not-implemented-methos-that-throw. Sorry I haven't investigated this part, but I have accidentally installed the pypi catanatron instead of the one from the commit source in CI before (and was driving me crazy once hehe); so thats why I feel there might be a way to get the same "signature"/"API" with a different pip installable.

I think we at least re-write the RandomPlayer and the AlphaBetaPlayer, and look to have a fast Game.play() thats backed by rust (or Game.play_many() or so).

I haven't been able to sit down on this, but thinking this Saturday might be a good day for me to explore more these thoughts. Just wanted to share some of my thoughts early in case they spark something on your end!

@pachewise
Copy link
Copy Markdown
Contributor

AlphaBeta for sure I think would benefit from refactoring, but I think RandomPlayer is so basic that it's probably more effort than it's worth right now to refactor in Rust. Up to you...

Game logic for sure should be Rust - I think those optimizations were something @tonypr was trying with C++.

@bcollazo
Copy link
Copy Markdown
Owner Author

Hey, so looked up this a bit. https://github.com/PyO3/maturin definitely looks like the way to go; agreed @zarns !

If I understand correctly, I'd say we will ultimately want this catanatron_rust folder to be the "maturin" project that has a structure like:

catanatron_rust/
├── Cargo.toml
├── pyproject.toml
└── src
    └── lib.rs

We probably want a main.rs under src/ that serves as a CLI into the rust core library, just for debugging purposes (not to take on catanatron_experimental/play.py.

Haven't gotten to it, but Chat GPT says if the lib.rs has some code like this:

use pyo3::prelude::*;

#[pyclass]
struct Game {
    num_players: usize,
}

#[pymethods]
impl Game {
    #[new]
    fn new(num_players: usize) -> Self {
        Game { num_players }
    }
    
    /// A stub method that simulates playing the game.
    fn play(&self) {
        println!("Playing a stub game with {} players.", self.num_players);
    }
}

/// The main module: creates a submodule named "game" and adds the Game class to it.
#[pymodule]
fn catanatron(py: Python, m: &PyModule) -> PyResult<()> {
    // Create a submodule named "game"
    let game_mod = PyModule::new(py, "game")?;
    game_mod.add_class::<Game>()?;
    
    // Add the submodule to the main module
    m.add_submodule(game_mod)?;
    Ok(())
}

then after building with Maturin we should get this API:

from catanatron.game import Game
game = Game(4)
game.play()

which is similar (just for illustration purposes) to the one we already have in catanatron. Again, its ok if for now we have the "package" have a conflicting name with the Pure Python implementation, I'd imagine we could change that (to say pip install catanatron[rust] or a whole another package pip install catanatron_rust).

I did noticed we still have work to do on having this branch be able to consistently run valid games. cargo run fails every now and then. Though it seems most pieces are there and that we can probably start bug-squashing now to get it steady. I'd say we're at a point we can probably delete all the benchmark-y code I had in main.rs, and change it to just run N simulations where N is a number (and not panic 😅). We can probably add env_logger and start adding some info!( and debug!( pieces here and there to aid debugging. I don't think we should try to emulate/recreate Python's rich output, but some simple logging to at least follow the actions and state mutations is probably good!

Sorry again I haven't have much time to jump into this deeper! But sharing some thoughts in case some others find the time! Does this all makes sense to you guys? Let me know!

And @pachewise I am hoping the RandomPlayer implementation looks something like (and thus not too big of a lift 🤞 🤞 and leveraging all the work on his branch 🔥 ):

impl Player for RandomPlayer {
    fn decide(
        &mut self,
        rng: &mut StdRng,
        _: &State,
        possible_actions: Vec<Action>,
    ) -> Action {
        possible_actions
            .choose(rng)
            .expect("There should always be at least one playable action")
            .clone()
    }
}

@bcollazo bcollazo marked this pull request as ready for review March 5, 2025 02:28
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.

5 participants