Skip to content

Comments

Caffemodel protobuf snapshots with shared weights shouldn't save multiple copies#2955

Closed
danielgordon10 wants to merge 1 commit intoBVLC:masterfrom
danielgordon10:snapshot-fix
Closed

Caffemodel protobuf snapshots with shared weights shouldn't save multiple copies#2955
danielgordon10 wants to merge 1 commit intoBVLC:masterfrom
danielgordon10:snapshot-fix

Conversation

@danielgordon10
Copy link

@danielgordon10 danielgordon10 commented Aug 21, 2015

Sorry, I had to delete the other branch #2946 because of the squashing. Continue discussion here.

It does seem feasible to me to write separate diffs for each shared param but not need to write the data. The data among shared parameters will always be the same, but based on the path through those parameters, the diffse could be different on different layers. Looking at the HDF5 writer, a similar precaution is taken. The comment in ToHDF5 on line 977 appears to support this argument.


This change is Reviewable

@danielgordon10
Copy link
Author

One way or another, the ToProto code must work in an analogous way to the ToHDF5 code.

So here I actually feel like the HDF5 code is less organized than the ToProto code. Through the principle of code reuse, we would want the parent serializer to call the child serializer rather than re-implement the logic. So there comes a question - do we want to be able to serialize a blob on its own? In the Proto case, that is possible. In the HDF5 case, it is not currently written that way.

So I could certainly pull the logic that writes blobs/layers into the net. But then we should probably also delete the ToProto methods in Layer and in Blob. And personally, I feel they might be useful in the future.

@longjon longjon changed the title Caffemodel snapshots with shared weights don't have multiple copies Caffemodel protobuf snapshots with shared weights shouldn't save multiple copies Aug 21, 2015
@longjon
Copy link
Contributor

longjon commented Aug 23, 2015

Yes, I see now that there's already asymmetry in the protobuf vs. HDF5 serialization pathways, and I could see an argument for a bit of reorganization there in one direction or the other.

However, the most immediate thing is to fix the duplicate storage bug, so I'd be happy to merge the most straightforward patch that does that, and revisit other issues later.

You don't have to create a new PR to rewrite history; just git push -f to your PR branch (keeping in mind that your remote branch will now point to different commits). Perhaps try this out by git commit --amending your commit with a descriptive message?

@danielgordon10
Copy link
Author

What changes would you like me to make that would qualify as the "most straightforward patch"?

@danielgordon10
Copy link
Author

Ping

@danielgordon10
Copy link
Author

So what's the deal here? Is this dead?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants