"lr state serialization" #2718
"lr state serialization" #2718
Conversation
| TensorToProto(*update_delta_, state.mutable_update_delta()); | ||
| auto str = state.SerializeAsString(); | ||
| *state_len = str.size(); | ||
| *state_len += str.size(); |
There was a problem hiding this comment.
Not sure why need to +=?
There was a problem hiding this comment.
because get the lr_state serialization length here.
std::string lr_str = this->lr_policy_->SerializeState(state_len)use = will overwrite the lr_state length.
| TensorToProto(*update_delta_, state.mutable_update_delta()); | ||
| auto str = state.SerializeAsString(); | ||
| *state_len = str.size(); | ||
| *state_len += str.size(); |
| optional double learning_rate = 101; | ||
| optional double lr_decay_a = 102; | ||
| optional double lr_decay_b = 103; | ||
| optional LrPolicyState lr_state = 101; |
There was a problem hiding this comment.
Do not re-use protobuf ids to change the fields. Well, since we it's still under development, this is fine. When people are using this code, we need to consider the backward compatibility.
There was a problem hiding this comment.
Thanks for the reminding. That's true when we maintain a project.
But what is the best practice of field number? Making the field number auto increment by 1 seems not a smart idea. By the way, the field number is only used by protoc compiler to represent an entry of protobuf. Even though protobuf support indexed value by field number, but it should not be used in user's code. Did you ever run into such a code base?
There was a problem hiding this comment.
Once the protobuf index is used by messages, it should never change for compatibility, for the old client or somewhere is still using the old .proto generated code.
- Do not change the current index
- Only append field not modify them.
2 participants
fix #2713