CFR solvers de/serialization

[inejc]

This PR addresses #149.

The end goal is to implement de/serialization for CFRSolver, CFRPlusSolver (1), CFRBRSolver (2), OutcomeSamplingMCCFRSolver (3) and ExternalSamplingMCCFRSolver (4).

Tasks:

• add de/serialization support for CFRInfoStateValuesTable (used by 1, 2, 3, 4)
• add de/serialization support for Policy (TabularPolicy and UniformPolicy) (used by 2, 3, 4)
• add de/serialization support for 1
• add de/serialization support for 2
• add de/serialization support for 3
• add de/serialization support for 4

[inejc]

It seems that we have to copy the second part of the split string here in order to be able to pass the reference to DeserializeCFRInfoStateValues:
https://github.com/deepmind/open_spiel/blob/37bdd9a1a4117b2f95c5f9bf8f35b5a3a030490f/open_spiel/algorithms/cfr.cc#L487

I believe this copy could also be avoided by making the parameter of DeserializeCFRInfoStateValues of type absl::string_view instead of const std::string& (in fact, given the use case, it might make sense to make all deserialization methods take absl::string_view). What are your thoughts about this?

[lanctot]

Sorry it will take me a bit of time to get to this and your other PR as I'm quite busy with some deadlines right now.

I am generally not a fan of string_view, I find its use is often justified as software engineering for the sake of it; like, do we really need the two or three nanonseconds we're saving on string processing when it's already expected to be slow? And it's caused a few bugs because I expect them to work like strings and they don't in all cases because they're basically not strings. Especially in OpenSpiel I have tried hard to avoid these kind of (IMO) unnecessary optimizations because I want the C++ code to still be accessible to everyone including C++ non-experts.

All that said: the case of serialization I tend to agree, they might be saving enough time that it's worth having them in this case. So, how about this: want to submit a separate PR that changes the signature of the serialization methods and just checks that all the tests still work? What I'm worried about is that pybind11 doesn't handle string_view and it might break the pickle serialization on the python side (same is true on the Julia API to be honest, but I'm not sure if we even support serialization in Julia; any opinion on this @findmyway?)

If it's an easy enough change and the tests all still work (in C++, Python, and Julia) then I'd be happy to support string_view for the serialization / deserialization methods, but would prefer it as its own PR separate from this one. Let me know what you think!

[inejc]

No worries about the delay.

I don't have a lot of experience with string_view so there very well might be drawbacks that I'm missing.

To clarify my thinking a bit; I'm mostly trying to avoid copying of instances where size might actually be a problem (i.e. CFRInfoStateValuesTable) and hence I think it would suffice if only DeserializeCFRInfoStateValuesTable and DeserializeCFRInfoStateValues would take string_view instead of the reference for now. Other de/ser is not as critical IMO; explicit Policy shouldn't be used for large game CFR solvers and AverageType is non-critical.

I definitely didn't mean to e.g. change de/ser for game and state instances for the reasons you outlined above.

[lanctot]

Oh I think I misunderstood. If you are asking to use string_view in one of your own functions only then that's perfectly fine :)

I thought you were suggesting a change to the signature of the core API. (Which, to be clear I am still fine with if we could do it without breaking tests!)

[findmyway]

FYI:

same is true on the Julia API to be honest, but I'm not sure if we even support serialization in Julia

de/serialization is supported in the Julia wrapper. But string_view is not supported.

[inejc]

I just realized I need to make sure deserialization works correctly for cases where info_state strings contain delimiters used for splitting.

EDIT: I solved this by prefixing the info_state data with an integer length and using that for splitting rather than any special delimiters that might be present in the data itself. Note that newline characters still shouldn't be present in info_state data, perhaps we should state that explicitly here https://github.com/deepmind/open_spiel/blob/39a072880d272ff4d9817ec570ccb1ea5810617a/open_spiel/spiel.h#L395

[lanctot]

EDIT: I solved this by prefixing the info_state data with an integer length and using that for splitting rather than any special delimiters that might be present in the data itself.

Since you've already implemented it and it's only in the serialized state data, this is fine. But I would prefer to have just a user-configurable delimiter that and an error if that delimiter is found in any of the infostate strings. It's a bit cleaner. I did this, for example, in corr_dist.cc (you can take a look if you want. But not necessary to change if you don't want to.. it's not critical.

Note that newline characters still shouldn't be present in info_state data, perhaps we should state that explicitly here

https://github.com/deepmind/open_spiel/blob/39a072880d272ff4d9817ec570ccb1ea5810617a/open_spiel/spiel.h#L395

This, however, I would prefer not to adopt. Why do you need it, anyway? I'd be extremely surprised to find that our infostate strings don't have newlines in them now, they were designedto be human-readable.

We should not disallow any particular characters or sequence of characters in the infostate string generally.. and newline character is very common. I'm quite confused on this point; can you explain why you'd want / need this?

[inejc]

I'd be extremely surprised to find that our infostate strings don't have newlines in them now, they were designedto be human-readable.

This is good to know, I wasn't aware of design choices for infostates.

I'm quite confused on this point; can you explain why you'd want / need this?

I'm using the newline character to separate the map elements (i.e. each info_state, values pair is in a separate line). Based on your clarifications I believe it would be more appropriate if the methods for both serialization and deserialization would take a user-configurable delimiter that would be used for all of the splittings.

For example, the following table

info_state_values_table = {
  {"0:0,0;0", CFRInfoStateValues({0, 1, 2}, 0.1)},
  {"1:1,1;1", CFRInfoStateValues({0, 1, 2, 3}, 0.2)}
};

SerializeCFRInfoStateValuesTable(info_state_values_table, "~");

would be serialized into

0:0,0;0~0,1,2;0.1,0.1,0.1;0.1,0.1,0.1;0.333333,0.333333,0.333333~1:1,1;1~0,1,2,3;0.2,0.2,0.2,0.2;0.2,0.2,0.2,0.2;0.25,0.25,0.25,0.25

and deserialization would then simply iterate over pairs of lines.

Additionally, separators , and ; (or any other that we fix for serialization of values) then mustn't be used when calling the SerializeCFRInfoStateValuesTable method.

Note that a similar approach would be used for de/serialization of TabularPolicy.

Please let me know what you think about this approach or whether I'm missing a more elegant solution.

[lanctot]

Please let me know what you think about this approach or whether I'm missing a more elegant solution.

This sounds great to me. I would only suggest that we use a longer/weirder default delimiter (specifically, not a single-character delimiter), a sequence that is that is unlikely to ever appear in the infostate strings.. that way the users almost never need to use a custom delimiter and can just use the preset default without thinking about it.

Also, I have not looked at the code yet, but if you're parsing and chopping up strings I strongly recommend absl::StrJoin and absl::StrSplit. If you have not used these: they are wonderful. There are several examples of their use throughout the code.

[inejc]

I've added serialization for CFRSolver and CFRPlusSolver. When deserializing, the user has to provide the reference to the game instance, i.e https://github.com/deepmind/open_spiel/blob/3bcd00a0f09865631e7a6a9dd98a966e54ad203c/open_spiel/algorithms/cfr.cc#L573 https://github.com/deepmind/open_spiel/blob/3bcd00a0f09865631e7a6a9dd98a966e54ad203c/open_spiel/algorithms/cfr_test.cc#L233 due to CFRSolverBase storing the reference to the game. This also prevents adding pickling support AFAIK (since pickle.loads doesn't take arbitrary parameters). To solve this I propose changing const Game& game_ to a shared pointer in https://github.com/deepmind/open_spiel/blob/3bcd00a0f09865631e7a6a9dd98a966e54ad203c/open_spiel/algorithms/cfr.h#L162 I think that's already the case for 3 and 4. Please let me know what you think.

Additionally, as I pointed out previously, I took special care to minimize unnecessary copies of potentially large CFR tables (mainly due to memory concerns). Despite that, I don't think copying of the CFRInfoStateValuesTable is elided here https://github.com/deepmind/open_spiel/blob/3bcd00a0f09865631e7a6a9dd98a966e54ad203c/open_spiel/algorithms/cfr.cc#L580 so maybe this should be implemented differently. Perhaps we should pass a pointer to the empty table initialized within the solver to the DeserializeCFRInfoStateValuesTable since this is an internal call. I'm curious about what you think about this as well.

Given the use case and memory concerns, I believe it would make sense to go a step further and return pointers to solver instances from all the DeserializeSomeCFRSolver methods.

[inejc]

In essence, I'm interested in changing all internal methods that de/serialize CFRInfoStateValuesTable to accept pointers and modify instances pointed to rather than accepting and returning copies. Particularly, I want to change SerializeCFRInfoStateValuesTable to accept a pointer to the resulting string and DeserializeCFRInfoStateValuesTable to accept a pointer to the resulting CFRInfoStateValuesTable.

EDIT: I believe I have addressed all of the issues regarding the copying/moving of large CFR tables.

[inejc]

Pickling support for 1 and 2 was added along with a python example. The output of the script is the following

$ python open_spiel/python/examples/cfr_cpp_example.py --solver=cfr
Iteration 0 exploitability: 0.458333
Iteration 1 exploitability: 0.270833
Iteration 2 exploitability: 0.194444
Iteration 3 exploitability: 0.140625
Iteration 4 exploitability: 0.121389
Iteration 5 exploitability: 0.096726
Iteration 6 exploitability: 0.107970
Iteration 7 exploitability: 0.090455
Iteration 8 exploitability: 0.077568
Iteration 9 exploitability: 0.068699
Persisting the model...
Loading the model...
Exploitability of the loaded model: 0.068698
Iteration 10 exploitability: 0.060470
Iteration 11 exploitability: 0.056175
Iteration 12 exploitability: 0.057411
Iteration 13 exploitability: 0.055493
Iteration 14 exploitability: 0.053719
Iteration 15 exploitability: 0.050510
Iteration 16 exploitability: 0.050083
Iteration 17 exploitability: 0.049263
Iteration 18 exploitability: 0.045239
Iteration 19 exploitability: 0.040669

[inejc]

@lanctot quick question: does policy_overrides_ in CFRBRSolver need to be serialized at all? It seems that they are always computed on the fly from best_response_computers_.
It seems that policy_overrides_ doesn't hold any state.

[lanctot]

Correct the BR policies never need to be saved as they are regenerated each iterarion.

For the precision would be good to support a user-specified number of decimal places (with a sensible default) but in addition also the ability to express the double literally as its bitwise representation in string form (8-byte hex digit). This would mean it is not human readable and architecture/OS dependent but who cares, it's lossless. Might even make sense for this to be the default.

Also you should think about getting this in soon. It is turning out to be rather large, touching many files. Is the basic stuff ready? Because you can iterate more on it afterward once it's in.

[inejc]

Also you should think about getting this in soon. It is turning out to be rather large, touching many files. Is the basic stuff ready? Because you can iterate more on it afterward once it's in.

Serialization and pickling support for CFRSolver, CFRPlusSolver, and CFRBRSolver is done so I think this is a good time to stop working on this PR. I removed the WIP tag and the code can be reviewed as I won't push any new commits.

After this is merged, I will follow up with two additional PRs:

• one for addressing your double precision comment above
• one for serialization of OutcomeSamplingMCCFRSolver and ExternalSamplingMCCFRSolver

I'm not familiar with the exact nature of the CFR-related calculations but I'm wondering whether double-precision is needed for CFR values or could we save some memory by using floats?

[lanctot]

Serialization and pickling support for CFRSolver, CFRPlusSolver, and CFRBRSolver is done so I think this is a good time to stop working on this PR. I removed the WIP tag and the code can be reviewed as I won't push any new commits.

Perfect, but there's a conflicting file (cfr.h). Can you pull from master and submit the merged header?

After this is merged, I will follow up with two additional PRs:

* one for addressing your double precision comment above

* one for serialization of `OutcomeSamplingMCCFRSolver` and `ExternalSamplingMCCFRSolver`

Sounds good!

I'm not familiar with the exact nature of the CFR-related calculations but I'm wondering whether double-precision is needed for CFR values or could we save some memory by using floats?

Almost every CFR has been run with doubles and I know from my thesis work that using floats leads to a considerable difference in convergence rates in practice, so I think we should stick with doubles. Should be relatively easy for someone to switch if they want to. We could add a using type that makes it easier for people but I doubt it would get used enough that it's worth justifying the custom type. Is this something that you would use?

[inejc]

Perfect, but there's a conflicting file (cfr.h). Can you pull from master and submit the merged header?

I have rebased against the latest master.

We could add a using type that makes it easier for people but I doubt it would get used enough that it's worth justifying the custom type. Is this something that you would use?

That's ok, the question was more out of curiosity. I think I will explore using floats once I actually start running experiments on a large game to see the effects.

[lanctot]

After this is merged, I will follow up with two additional PRs:

* one for addressing your double precision comment above

* one for serialization of `OutcomeSamplingMCCFRSolver` and `ExternalSamplingMCCFRSolver`

I think I take it back: please continue to use this PR to add these. I only say it because I'm quite sure I will not have the time to import it this week, and I will be on vacation next week, and the following two weeks I will be busy with something else.

But after all that I will be able to import this PR. And I will do #240 now, so it will get merged.

Or..... I could also try to recruit someone else from the team to import it. I'll try that and see how that goes :) But I still think it makes sense just to finish those last two pieces you mention above.

[inejc]

This PR is now deemed finished and ready for review.

[lanctot]

Can you add some comments here explaining on a high-level the serialization support in the CFR solvers, and how to enable it?

[inejc]

Done.

[lanctot]

Sorry, I'm missing why we need this extra constructor, can you explain?

[inejc]

The only difference with the existing one is that it also accepts iteration (and that it accepts the game instance as a shared_ptr but this is not really relevant). If we remove this constructor I believe the only other alternative is to create a setter for iteration_. Rather than creating setters for fields that get deserialized and need to be set on new instances after deserialization, I created new constructors. Maybe there is a better approach?

[inejc]

Please note that InitializeInfostateNodes() is not called in this constructor since the table is expected to be deserialized into an empty table later (using the DeserializeCFRInfoStateValuesTable() method).

[lanctot]

Can you explain why you need this to change to shared_ptr ?

[inejc]

Because a game is deserialized by calling LoadGame within some deserialization function. I don't think it's possible to create a new solver instance and assigning a reference to the game without managing the game instance by the solver creator? Maybe I'm missing something here but if the deserialization function creates a smart pointer to the game instance and then sets the reference the game instance it would get destroyed after the deserialization function exits and if the function creates a regular pointer, we have a memory leak. Is there a better approach?

[lanctot]

Why copy these again here? Just put them in cfr.h and re-use them?

[inejc]

Done.

[lanctot]

Why pass in the RNG rather than the seed? Passing in the complex object here has the problem of forcing one to use std::mt19937 but also making it harder to expose to python.

[inejc]

This constructor is only meant for deserialization purposes, i.e. having only the seed is not sufficient to reconstruct the RNG if it was already sampled from. For the same reason, this function should not be exposed to Python as this is already done implicitly via pickling. As above, please let me know if I should expose setters rather than create "deserialization" constructors.

[lanctot]

Same here, let's avoid the duplication

[inejc]

Done.

[inejc]

@lanctot I see that an RNG was added to CFRSolverBase. Is this something that also needs to be serialized now (I assume not as it is only used for initialization)?

Also: may I please ask for a quick look at the last resolve merge conflicts for cfr.cc, cfr.h and policy.cc commit as I'm not familiar with the incoming changes from master to see whether I resolved the conflicts correctly?

[lanctot]

Hi @inejc, sorry for the delays.. I'm finally getting to this.

@lanctot I see that an RNG was added to CFRSolverBase. Is this something that also needs to be serialized now (I assume not as it is only used for initialization)?

Yes only for initialization, so does not affect this PR.

Also: may I please ask for a quick look at the last resolve merge conflicts for cfr.cc, cfr.h and policy.cc commit as I'm not familiar with the incoming changes from master to see whether I resolved the conflicts correctly?

Ok, will do.