-
Notifications
You must be signed in to change notification settings - Fork 929
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
CFR solvers de/serialization #222
Conversation
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 I believe this copy could also be avoided by making the parameter of |
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! |
No worries about the delay. I don't have a lot of experience with To clarify my thinking a bit; I'm mostly trying to avoid copying of instances where size might actually be a problem (i.e. I definitely didn't mean to e.g. change de/ser for game and state instances for the reasons you outlined above. |
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!) |
FYI:
de/serialization is supported in the Julia wrapper. But string_view is not supported. |
f6c0959
to
421f164
Compare
I just realized I need to make sure deserialization works correctly for cases where EDIT: I solved this by prefixing the |
0dcd041
to
a8f643a
Compare
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.
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? |
This is good to know, I wasn't aware of design choices for infostates.
I'm using the newline character to separate the map elements (i.e. each 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 Note that a similar approach would be used for de/serialization of 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 |
I've added serialization for 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 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 |
EDIT: I believe I have addressed all of the issues regarding the copying/moving of large CFR tables. |
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 |
|
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. |
Serialization and pickling support for After this is merged, I will follow up with two additional PRs:
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? |
Perfect, but there's a conflicting file (cfr.h). Can you pull from master and submit the merged header?
Sounds good!
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? |
I have rebased against the latest master.
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. |
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. |
…miter instead of using info state length
This PR is now deemed finished and ready for review. |
"[SolverSpecificState]"; | ||
constexpr const char* kSerializeSolverValuesTableSectionHeader = | ||
"[SolverValuesTable]"; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some comments here explaining on a high-level the serialization support in the CFR solvers, and how to enable it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
InitializeInfostateNodes(*root_state_); | ||
} | ||
|
||
CFRSolverBase::CFRSolverBase(std::shared_ptr<const Game> game, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm missing why we need this extra constructor, can you explain?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
protected: | ||
const Game& game_; | ||
std::shared_ptr<const Game> game_; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you explain why you need this to change to shared_ptr ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -26,6 +26,23 @@ | |||
namespace open_spiel { | |||
namespace algorithms { | |||
|
|||
// Serialization of the MCCFR solver is in agreement with de/serialization of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why copy these again here? Just put them in cfr.h and re-use them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
ExternalSamplingMCCFRSolver::ExternalSamplingMCCFRSolver( | ||
std::shared_ptr<const Game> game, std::shared_ptr<Policy> default_policy, | ||
std::unique_ptr<std::mt19937> rng, AverageType avg_type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// Serialization of the MCCFR solver is in agreement with de/serialization of | ||
// regular CFR solvers, i.e. take a look at the PartiallyDeserializedCFRSolver() | ||
// method for more info. | ||
constexpr const int kSerializationVersion = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, let's avoid the duplication
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@lanctot I see that an RNG was added to 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? |
Hi @inejc, sorry for the delays.. I'm finally getting to this.
Yes only for initialization, so does not affect this PR.
Ok, will do. |
This PR addresses #149.
The end goal is to implement de/serialization for
CFRSolver, CFRPlusSolver
(1),CFRBRSolver
(2),OutcomeSamplingMCCFRSolver
(3) andExternalSamplingMCCFRSolver
(4).Tasks:
CFRInfoStateValuesTable
(used by 1, 2, 3, 4)Policy (TabularPolicy and UniformPolicy)
(used by 2, 3, 4)