New serialization format#12384

Closed

houseroad wants to merge 15 commits intopytorch:masterfrom

houseroad:torchproto2

Member

houseroad commented Oct 5, 2018 •

edited

Loading

Addressed Dima's feedback.

The proposal is here: https://fb.quip.com/TbQmAuqIznCf

houseroad requested review from Yangqing, dzhulgakov, goldsborough and zdevito

October 8, 2018 21:42

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

dzhulgakov has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad requested a review from jamesr66a

October 8, 2018 22:05

zdevito reviewed

View reviewed changes

Contributor

zdevito left a comment

I'd like to see a better overall picture about where this is going in the long term. When we are done merging the formats, what do we want the new format to be?

The format as-is seems like a mishmash of random fields needed by caffe2's current model and a few places string for where a future torch models format would fit in. It feels like it is code that we will have to clean up in the future. There are tons of Argument and NetDef fields sitting throughout the format. Some input/output fields in MethodDef will be redundant with how torch represents networks. I also do not see how Tensors will be stored with proper storages where two tensors can be views of the same storage. This is an important part of the PyTorch format.

I'd like to get this to the point where the core structure reflects the ideal we are working toward that will be shared by all of our serialization formations, with the per-backend fields put inside sub objects. Otherwise it is very hard to see what can be shared among formats vs what is only filled in for a specific format.

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

houseroad commented

View reviewed changes

Member Author

houseroad left a comment

Great thanks to @zdevito for the valuable input! I agree with that we should have a clean design at this moment instead of cleaning it up in the future. I will remove the unnecessary fields, make sure that from the organization side, nothing looks redundant.

At high level, since right now we need to support both static graph and torch script, we aim unifying the model format (proto), APIs and tooling. Later when everything is ready, we can start unifying the methods.

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

houseroad force-pushed the torchproto2 branch from d5c7e02 to 8d5829d Compare

October 9, 2018 00:20

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Yangqing reviewed

View reviewed changes

caffe2/core/blob_serialization.cc Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

dzhulgakov reviewed

View reviewed changes

Collaborator

dzhulgakov left a comment

For the OpDef changes - they probably require the longer discussion on input/argument distinction and what's the longer term migration path. Before that discussion adding random fields might be premature. So if you want to proceed with other parts of this diff (modules and tensors) - maybe commenting out and keeping NetDef as is mostly is best.

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

Yangqing reviewed

View reviewed changes

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

kennyhorror reviewed

View reviewed changes

caffe2/core/blob_serialization.cc Outdated

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

houseroad force-pushed the torchproto2 branch from 68dd65e to edf90d9 Compare

October 11, 2018 20:48

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad commented

View reviewed changes

Member Author

houseroad left a comment

Thanks a lot for the valuable inputs from @Yangqing @dzhulgakov @kennyhorror .

I have made the following changes:

premature design are removed, i.e., named_inputs/outputs and Non-tensor type support
unnecessary fields are removed, e.g., debug_info in multiple structures
extract external data into a separate structure, strides and id/path are binded to ExternalData
add another layer of abstract - ParameterDef, and move is_buffer and requires_gradient to it
more comments are added

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

dzhulgakov reviewed

View reviewed changes

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

dzhulgakov approved these changes

View reviewed changes

Collaborator

dzhulgakov left a comment

minor naming tweaks and ship it

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

Yangqing reviewed

View reviewed changes

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

caffe2/proto/caffe2.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

caffe2/proto/torch.proto Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Contributor

Yangqing commented Oct 16, 2018

Maybe a rebase and see if circleCI passes? It is showing weird failure at the moment.

houseroad added 6 commits

October 16, 2018 10:10


          This is the old commit before address Dima's comments

bb61358


          add comments in proto


          add more comments

659bb3b


          better support for method type

d7397e1


          address Zach's comments

5fae5a5


          move parameters to Module

243ec78

houseroad added 9 commits

October 16, 2018 10:10


          lint

a7bc74c


          address some of the comments, refine the external data and parameter …

…parts


          address the comments

eef3a85


          make TORCH_WRAPPER default for exteranl data

b990724


          fix the syntax

74be616


          more comments

2c45e86


          add more comments

cdaf545


          address the comments

03ca848


          address comments, mainly focus on the naming

af60ce1

houseroad force-pushed the torchproto2 branch from 7af7473 to af60ce1 Compare

October 16, 2018 17:10

facebook-github-bot reviewed

View reviewed changes

Contributor

facebook-github-bot left a comment

houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

Member Author

houseroad commented Oct 16, 2018

Yeah, I guess some docker images are updated and this caused the problems. Just rebased the PR, and let's see.

facebook-github-bot closed this in

30aaa07

ezyang added the merged label

laurentdupin pushed a commit to laurentdupin/pytorch that referenced this pull request


          New serialization format (pytorch#12384)

397da85

Summary:
Addressed Dima's feedback.

The proposal is here: https://fb.quip.com/TbQmAuqIznCf
Pull Request resolved: pytorch#12384

Reviewed By: dzhulgakov

Differential Revision: D10246743

Pulled By: houseroad

fbshipit-source-id: c80db0c35d60ca32965275da705f2b1dfb2a7265

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

Reviewers

zdevito zdevito left review comments

dzhulgakov dzhulgakov approved these changes

goldsborough Awaiting requested review from goldsborough

jamesr66a Awaiting requested review from jamesr66a

+3 more reviewers

Yangqing Yangqing left review comments

kennyhorror kennyhorror left review comments

facebook-github-bot facebook-github-bot left review comments

Labels

None yet