Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for Caffe2 ONNX export #4295

Closed
wants to merge 5 commits into from
Closed

Add support for Caffe2 ONNX export #4295

wants to merge 5 commits into from

Conversation

thiagocrepaldi
Copy link
Contributor

Currently all Caffe2 export tests (under tests/test_export_caffe2.py) fail because the latest onnx releases do not have onnx.optimizer submodule anymore (instead, a new module onnxoptimizer was created from it)

However, fuse_bn_into_conv optimization previously implemented within onnx.optimizer is already performed by torch.onnx.export too ruing ONNX export. Therefore onnx.optimizer dependency can be safely removed from detectron2 code.

Depends on pytorch/pytorch#75718
Fixes #3488
Fixes pytorch/pytorch#69674 (PyTorch repo)

ps: Although Caffe2 support is/will be deprecated, this PR relies on the fact that contributions are welcome as stated at docs/tutorials/deployment.md

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 1, 2022
@thiagocrepaldi
Copy link
Contributor Author

@ppwwyyxx @FrancescoMandru splitting up Detectron2 Caffe2 ONNX export in a separate (and clean) PR

@ppwwyyxx
Copy link
Contributor

ppwwyyxx commented Jun 1, 2022

It seems the test failures are not caused by this PR, but they are indeed related to caffe2 export. From the error message, PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python envvar might be the easiest fix. But feel free to ignore it.

@ppwwyyxx ppwwyyxx mentioned this pull request Jun 2, 2022
Closed
@thiagocrepaldi thiagocrepaldi force-pushed the thiabgofc/fix-caffe2-export branch from 6dcd01e to 4ba9841 Compare June 3, 2022 15:09
@thiagocrepaldi
Copy link
Contributor Author

It seems the test failures are not caused by this PR, but they are indeed related to caffe2 export. From the error message, PROTOCOL_BUFFERS_PYTHON_IMPLEMENTATION=python envvar might be the easiest fix. But feel free to ignore it.

This is caused by the ONNX graph code, which is shared by caffe2/onnx-only export
The ONNX graph is serialized to protobuf's proto
#4291 already adds the correct version of the protobuf - instead of letting it as implicit dependency which would cause the latest - broken - version to be installed

@thiagocrepaldi thiagocrepaldi force-pushed the thiabgofc/fix-caffe2-export branch 2 times, most recently from 3a4fa1f to 1fde678 Compare June 6, 2022 17:24
@thiagocrepaldi thiagocrepaldi mentioned this pull request Jun 6, 2022
Closed
@orionr
Copy link

orionr commented Jun 14, 2022

We are looking for the right POCs to review. Stay tuned.

@orionr
Copy link

orionr commented Jun 14, 2022

Also, can you please rebase since there seems to be conflicts? Thanks.

ppwwyyxx
ppwwyyxx reviewed Jun 14, 2022
View reviewed changes
setup.py Outdated Show resolved Hide resolved
ppwwyyxx
ppwwyyxx reviewed Jun 14, 2022
View reviewed changes
detectron2/export/c10.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link
Contributor

@zhanghang1989 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@thiagocrepaldi thiagocrepaldi force-pushed the thiabgofc/fix-caffe2-export branch from 1fde678 to eeff5f2 Compare June 15, 2022 15:46
@facebook-github-bot
Copy link
Contributor

@thiagocrepaldi has updated the pull request. You must reimport the pull request before landing.

@thiagocrepaldi
Copy link
Contributor Author

Also, can you please rebase since there seems to be conflicts? Thanks.

Thanks @orionr. Just updated the PR. Let me know if I can help

@facebook-github-bot
Copy link
Contributor

@thiagocrepaldi has updated the pull request. You must reimport the pull request before landing.

@thiagocrepaldi thiagocrepaldi force-pushed the thiabgofc/fix-caffe2-export branch from 389796a to ba9497a Compare June 15, 2022 16:01
@facebook-github-bot
Copy link
Contributor

@thiagocrepaldi has updated the pull request. You must reimport the pull request before landing.

@thiagocrepaldi
Copy link
Contributor Author

I have pushed a commit with automatic changes from dev/linter.sh. Environment dependencies are correct, as shown below:


Package                                  Version            
----------------------- ------------------------ 
black                                       22.3.0
cloudpickle                             2.1.0
fairscale                                  0.4.6
flake8                                     3.8.1
flake8-bugbear                      22.4.25
flake8-comprehensions         3.10.0
future                                     0.18.2
fvcore                                     0.1.5.post20220512
hydra-core              1.2.0
iopath                  0.1.9
isort                   4.3.21
matplotlib              3.5.2
omegaconf               2.2.2
packaging               21.3
panopticapi             0.1
Pillow                  9.1.1
psutil                  5.9.1
pycocotools             2.0.4
pydot                   1.4.2
Pygments                2.12.0
scipy                   1.8.1
Shapely                 1.8.2
tabulate                0.8.9
tensorboard             2.9.1
termcolor               1.1.0
timm                    0.6.2.dev0
tqdm                    4.64.0
yacs                    0.1.8

@thiagocrepaldi thiagocrepaldi requested a review from ppwwyyxx June 15, 2022 22:19
@thiagocrepaldi
Copy link
Contributor Author

@ppwwyyxx sorry, clicked the "Re-request review" in this PR by accident instead of #4315 :-(

Thiago Crepaldi added 4 commits June 20, 2022 10:56
Remove unnecessary constant outputs from ONNX exported graph
0af53a6 
`TracingAdapter` creates extra outputs (through `flatten_to_tuple`) to hold
metadata information to rebuild the original data format during
deserialization.

When exporting a PyTorch model to ONNX, the support to de-serialize the
output to the original formatThis is unnecessary during ONNX export as the original data will never
be reconstructed to its original format using Schema.__call__ API.
This PR suppresses such extra output constants during
torch.onnx.export() execution. Outside this API, the behavior is not
changed, ensuring BC.

Although not stricly necessary to achieve the same numerical results as
PyTorch, when a ONNX model schema is compared to PyTorch's, the diffrent
number of outputs (ONNX model will have more outputs than PyTorch) may
not only confuse users, but also result in false negative when coding
model comparison helpers.
Address comments
c376713
Reformatted code using dev/linter.sh
4493d97
Revert part of dev/linter.sh changes
7930b25
@thiagocrepaldi thiagocrepaldi force-pushed the thiabgofc/fix-caffe2-export branch from ba9497a to 7930b25 Compare June 20, 2022 14:58
@facebook-github-bot
Copy link
Contributor

@thiagocrepaldi has updated the pull request. You must reimport the pull request before landing.

@thiagocrepaldi
Copy link
Contributor Author

@ppwwyyxx sorry, clicked the "Re-request review" in this PR by accident instead of #4315 :-(

gentle ping

ppwwyyxx
ppwwyyxx approved these changes Jun 23, 2022
View reviewed changes
Copy link
Contributor

@ppwwyyxx ppwwyyxx left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks a lot for your patience!

@ppwwyyxx
Copy link
Contributor

@zhanghang1989 @wat3rBro can you help merge this?

@thiagocrepaldi
Copy link
Contributor Author

@orionr This one is also ready to go

@orionr
Copy link

orionr commented Jul 5, 2022

Checking with the team. Thanks.

@thiagocrepaldi
Copy link
Contributor Author

Checking with the team. Thanks.

Thanks @orionr @zhanghang1989 @wat3rBro. Any update on this merge?

@facebook-github-bot
Copy link
Contributor

@zhanghang1989 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@thiagocrepaldi
Copy link
Contributor Author

@zhanghang1989 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Thanks @zhanghang1989 @orionr @wat3rBro The internal test failed after 6h. Could you help me with some backtrace or relevant-non-proprietary log so that i can fix it? I would guess this is a false positive, as Caffe2 export is not currently supported by Detectron2 and the files I changed are only pertinent to Caffe2 export

@thiagocrepaldi
Copy link
Contributor Author

@zhanghang1989 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Thanks @zhanghang1989 @orionr @wat3rBro The internal test failed after 6h. Could you help me with some backtrace or relevant-non-proprietary log so that i can fix it? I would guess this is a false positive, as Caffe2 export is not currently supported by Detectron2 and the files I changed are only pertinent to Caffe2 export

gentle ping

@facebook-github-bot
Copy link
Contributor

@mcimpoi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

@thiagocrepaldi has updated the pull request. You must reimport the pull request before landing.

@thiagocrepaldi
Copy link
Contributor Author

@mcimpoi the fix wasnt pushed to the repo, just my local branch. I have pushed it now (after your reimport)

@facebook-github-bot
Copy link
Contributor

@mcimpoi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

1 similar comment
@facebook-github-bot
Copy link
Contributor

@mcimpoi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@thiagocrepaldi
Copy link
Contributor Author

@mcimpoi Thank you very much for helping with this PR, really appreciate it

@thiagocrepaldi thiagocrepaldi deleted the thiabgofc/fix-caffe2-export branch July 15, 2022 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ppwwyyxx ppwwyyxx ppwwyyxx approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to export PointRend model (from detectron2) to ONNX Exporting a model to ONNX; onnx.optimizer
4 participants
@thiagocrepaldi @ppwwyyxx @orionr @facebook-github-bot