Skip to content

Use google protobuf in pytorch onnx import/export#8469

Closed
li-roy wants to merge 10 commits intopytorch:masterfrom
li-roy:pn
Closed

Use google protobuf in pytorch onnx import/export#8469
li-roy wants to merge 10 commits intopytorch:masterfrom
li-roy:pn

Conversation

@li-roy
Copy link
Contributor

@li-roy li-roy commented Jun 14, 2018

No description provided.

apaszke
apaszke reviewed Jun 14, 2018
View reviewed changes
Copy link
Contributor

@apaszke apaszke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not a complete review)

torch/csrc/jit/export.cpp 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

orionr
orionr reviewed Jun 14, 2018
View reviewed changes
Copy link
Contributor

@orionr orionr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

torch/csrc/jit/export.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/export.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/import.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/onnx/init.cpp Outdated

This comment was marked as off-topic.

Sign in to view

torch/csrc/jit/export.cpp Outdated

This comment was marked as off-topic.

Sign in to view

@ezyang
Copy link
Contributor

ezyang commented Jun 15, 2018

Windows test failure looks legit

@li-roy
Copy link
Contributor Author

li-roy commented Jun 15, 2018

@pytorchbot retest this please

@li-roy
Copy link
Contributor Author

li-roy commented Jun 15, 2018

cc: @zdevito

@ailzhang
Copy link
Contributor

ailzhang commented Jul 10, 2018

@li-roy do you want to resolve the conflicts and retest this?

@li-roy
Copy link
Contributor Author

li-roy commented Jul 10, 2018

@ailzhang I think i'm still waiting for the windows build to be changed.

@bddppq
Copy link
Contributor

bddppq commented Jul 10, 2018

@li-roy The windows protobuf issue should already have been resolved

@li-roy
Copy link
Contributor Author

li-roy commented Jul 10, 2018

@bddppq Ah ok, didn't realize it was on master already. In that case i'll rebase and test it. Thanks for looking into it!

@li-roy
Copy link
Contributor Author

li-roy commented Jul 19, 2018

@pytorchbot retest this please

1 similar comment
@li-roy
Copy link
Contributor Author

li-roy commented Jul 20, 2018

@pytorchbot retest this please

houseroad
houseroad reviewed Jul 20, 2018
View reviewed changes
torch/csrc/jit/export.cpp Outdated

This comment was marked as off-topic.

Sign in to view

This comment was marked as off-topic.

Sign in to view

houseroad
houseroad reviewed Jul 20, 2018
View reviewed changes
torch/csrc/jit/export.cpp Outdated

This comment was marked as off-topic.

Sign in to view

@houseroad houseroad mentioned this pull request Jul 26, 2018
Closed
facebook-github-bot pushed a commit that referenced this pull request Jul 27, 2018
@houseroad @facebook-github-bot
Suppress the vptr warning in ubsan (#9909)
94439d7 
Summary:
Unblock #8469
Pull Request resolved: #9909

Differential Revision: D9023650

Pulled By: houseroad

fbshipit-source-id: 7682a9cd7905e98c802b820ad59745672b32970d
jramseyer pushed a commit to jramseyer/pytorch that referenced this pull request Jul 30, 2018
@houseroad @jramseyer
Suppress the vptr warning in ubsan (pytorch#9909)
efe6445 
Summary:
Unblock pytorch#8469
Pull Request resolved: pytorch#9909

Differential Revision: D9023650

Pulled By: houseroad

fbshipit-source-id: 7682a9cd7905e98c802b820ad59745672b32970d
@li-roy li-roy closed this Jul 31, 2018
@dzhulgakov
Copy link
Collaborator

dzhulgakov commented Jul 31, 2018

why did we close it? let's land it first, before the #9746 which might take a few iterations

@li-roy li-roy reopened this Jul 31, 2018
houseroad
houseroad approved these changes Jul 31, 2018
View reviewed changes
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG, let's land this first.

facebook-github-bot
facebook-github-bot reviewed Jul 31, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

facebook-github-bot
facebook-github-bot reviewed Jul 31, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

facebook-github-bot
facebook-github-bot reviewed Jul 31, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

facebook-github-bot
facebook-github-bot reviewed Aug 1, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

li-roy is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
@houseroad
Suppress the vptr warning in ubsan (pytorch#9909)
51729e0 
Summary:
Unblock pytorch#8469
Pull Request resolved: pytorch#9909

Differential Revision: D9023650

Pulled By: houseroad

fbshipit-source-id: 7682a9cd7905e98c802b820ad59745672b32970d
goodlux pushed a commit to goodlux/pytorch that referenced this pull request Aug 15, 2018
Use google protobuf in pytorch onnx import/export
4cb764b 
Summary: Pull Request resolved: pytorch#8469

Reviewed By: houseroad

Differential Revision: D9102041

Pulled By: li-roy

fbshipit-source-id: 805c473745d181b71c7deebf0b9afd0f0849ba4f
@ezyang ezyang added the merged label Jun 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

@zdevito zdevito zdevito approved these changes

@houseroad houseroad houseroad approved these changes

@anderspapitto anderspapitto Awaiting requested review from anderspapitto

@bddppq bddppq Awaiting requested review from bddppq

@colesbury colesbury Awaiting requested review from colesbury

@dzhulgakov dzhulgakov Awaiting requested review from dzhulgakov

@ezyang ezyang Awaiting requested review from ezyang

@gchanan gchanan Awaiting requested review from gchanan

@jamesr66a jamesr66a Awaiting requested review from jamesr66a

@smessmer smessmer Awaiting requested review from smessmer

@soumith soumith Awaiting requested review from soumith

@Yangqing Yangqing Awaiting requested review from Yangqing

+2 more reviewers

@orionr orionr orionr left review comments

@apaszke apaszke apaszke left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@li-roy @ezyang @ailzhang @bddppq @dzhulgakov @orionr @zdevito @apaszke @facebook-github-bot @houseroad