Add iteration counts to MPP protocol - #378
Conversation
…ion per port which can be updated
maarten-ic
left a comment
maarten-ic
left a comment
There was a problem hiding this comment.
See comments below. Let me know if you have any questions!
LourensVeen
left a comment
LourensVeen
left a comment
There was a problem hiding this comment.
I've left a few comments, but I mainly want to say that this is very, very nice. Clear, clean, well-structured, well-documented, despite the complexity of the algorithm. Couldn't be happier 😄
maarten-ic
left a comment
maarten-ic
left a comment
There was a problem hiding this comment.
Part 1 of the C++ review -- I didn't look at the tests yet. Nice work!
| if (first_run_.is_set() | ||
| #ifdef MUSCLE_ENABLE_MPI | ||
| && mpi_barrier_.is_root() | ||
| #endif |
There was a problem hiding this comment.
Only do this check on the root? @maarten-ic
There was a problem hiding this comment.
I agree this check should be done on the root process only.
My latest commit changes the logic slightly, such that any thrown exceptions are handled with the existing try/except logic for MPI-enabled instances.
To ensure TimelineStates can be reliably compared for equality. Since the list of ports should be fairly small (and checkpoints are not commonly generated), I don't expect a performance problem because of this.
Ensure any thrown exceptions in `communicator_->finish_reuse_iteration()` are handled correctly with MPI. Also updated the Python code to follow the same flow as C++.
LourensVeen
left a comment
LourensVeen
left a comment
There was a problem hiding this comment.
You know, C++ gets criticised sometimes, but there's really not much wrong with it if you just use it right... 😄 Looks really nice, I didn't check the logic again in detail because I trust it matches the Python side, but I found a few style things you may want to look at. It's pretty much ready to go though.
|
|
||
| bool TimelinePorts::has_participated(std::string const & port_name, Optional<int> slot) const { | ||
| PortAndSlot key(port_name, slot); | ||
| for (auto const & p : participated) |
There was a problem hiding this comment.
std::find would make that a one-liner I think?
| @@ -17,7 +18,7 @@ namespace libmuscle { namespace _MUSCLE_IMPL_NS { | |||
| class Snapshot { | |||
| public: | |||
| enum class VersionByte : char { | |||
There was a problem hiding this comment.
Should probably be inline static const char version_byte = '2' in C++17 to match the Python side. Or could even be static const char version_byte already with a separate definition in snapshot.cpp. But we can also change that later.
| resume_overlay_ = snapshot.settings_overlay; | ||
|
|
||
| port_manager_.restore_message_counts(snapshot.port_message_counts); | ||
| if (!snapshot.is_final_snapshot) { |
There was a problem hiding this comment.
The Python side has a comment here, maybe good to replicate? I've already forgotten what this was for...
| * @param our_ndims Number of dimensions of our instance set. | ||
| * @param peer_dims Dimensions of the peer instance set of this port. | ||
| */ | ||
| Port( |
There was a problem hiding this comment.
Now that we have a ymmsl::Port, should this derive from it like in Python?
| // Note: this header is only ever pulled in via the LIBMUSCLE_MOCK_TIMELINE_MANAGER | ||
| // swap point inside timeline_manager.hpp, which is already inside the | ||
| // libmuscle::_MUSCLE_IMPL_NS namespace at that point, so it isn't opened here again. |
There was a problem hiding this comment.
The other mocks do reopen the namespace. Is there a reason to deviate from that here? Seems good to be consistent, and safer in case something changes?
| settings["b"] = true; | ||
| Message message(1.0, 2.0, "test_data", settings); | ||
| Snapshot snapshot ({}, 0, {}, false, message, {}); | ||
| TimelineStateFixture timeline_state_fixture; |
There was a problem hiding this comment.
Would it make sense to use a fixture class here, like we're doing in e.g. test_communicator.cpp?
| mock_port_manager_.get_message_counts.return_value = port_message_counts; | ||
| mock_port_manager_.settings_in_connected.return_value = false; | ||
|
|
||
| TimelineStateFixture timeline_state_fixture; |
There was a problem hiding this comment.
Same here, it could be in the fixture and then we could just use it, like communicator_.
| ExpectedActions: TypeAlias = list[tuple[str, Port, list[int]]] | ||
|
|
||
|
|
||
| class SubTimelineState(TypedDict): |
There was a problem hiding this comment.
Looking at this again, why is this a TypedDict and TimelineState below a dataclass?
3 participants
See issue #373