Skip to content

Adding export registration and tests for custom ops#4248

Merged
snnn merged 14 commits into
microsoft:masterfrom
neginraoof:neraoof/customOpExport
Jun 26, 2020
Merged

Adding export registration and tests for custom ops#4248
snnn merged 14 commits into
microsoft:masterfrom
neginraoof:neraoof/customOpExport

Conversation

@neginraoof
Copy link
Copy Markdown
Contributor

@neginraoof neginraoof commented Jun 16, 2020

Description:
Adding export registration and tests for custom onnxruntime ops.

Motivation and Context

  • TO enable export and running custom ops without standard ONNX spec

@neginraoof neginraoof requested a review from a team as a code owner June 16, 2020 17:26
thiagocrepaldi
thiagocrepaldi reviewed Jun 16, 2020
View reviewed changes
Comment thread tools/python/test_custom_ops_export.py Outdated
Comment thread tools/python/test_custom_ops_export.py
Comment thread tools/python/test_custom_ops_export.py
@pranavsharma
Copy link
Copy Markdown
Contributor

pranavsharma commented Jun 17, 2020

The names of the new files seem generic. However this pertains to pytorch converter only, right? Can we update the names of the files to reflect that?

@jywu-msft
Copy link
Copy Markdown
Member

jywu-msft commented Jun 17, 2020

The names of the new files seem generic. However this pertains to pytorch converter only, right? Can we update the names of the files to reflect that?

also, if these are test cases, they don't seem to belong in the tools directory?

@neginraoof
Copy link
Copy Markdown
Contributor Author

neginraoof commented Jun 17, 2020 •
edited
Loading

@jywu-msft I agree. I'm still not sure what's the best place for the code (for both op registrations and tests). Please let me know if you have a suggestion.
If you're only concerned about the test file, one option is to create a separate test folder (under tools) and move these over.

@thiagocrepaldi
Copy link
Copy Markdown
Contributor

thiagocrepaldi commented Jun 17, 2020

@jywu-msft I agree. I'm still not sure what's the best place for the code (for both op registrations and tests). Please let me know if you have a suggestion.
If you're only concerned about the test file, one option is to create a separate test folder (under tools) and move these over.

In order for the CI to find tests and copy them to build dir, they have to be in specific places. As this test is useful for both inference and training, I suggest putting them https://github.com/microsoft/onnxruntime/tree/master/onnxruntime/test/python. It has to have test in the name too

thiagocrepaldi
thiagocrepaldi suggested changes Jun 23, 2020
View reviewed changes
Comment thread tools/python/test_custom_ops_pytorch_exporter.py
@neginraoof
Adding test folder
1a34e8b
@neginraoof
Merge branch 'neraoof/customOpExport' of github.com:neginraoof/onnxru…
66de2f1 
…ntime into neraoof/customOpExport

# Conflicts:
#	tools/ci_build/github/azure-pipelines/linux-ort-pytorch-ci-pipeline.yml
thiagocrepaldi
thiagocrepaldi previously approved these changes Jun 23, 2020
View reviewed changes
@neginraoof
Copy link
Copy Markdown
Contributor Author

neginraoof commented Jun 23, 2020

cc @BowenBao Could you please review the PR and document for export of custom ops? Thanks!

BowenBao
BowenBao reviewed Jun 24, 2020
View reviewed changes
Comment thread docs/ExportPyTorchCustomOps.md
Comment thread tools/test/test_custom_ops_pytorch_exporter.py Outdated
thiagocrepaldi
thiagocrepaldi reviewed Jun 25, 2020
View reviewed changes
Comment thread tools/test/test_custom_ops_pytorch_exporter.py Outdated
thiagocrepaldi
thiagocrepaldi previously approved these changes Jun 25, 2020
View reviewed changes
@snnn
Copy link
Copy Markdown
Contributor

snnn commented Jun 25, 2020 

     [flake8 PEP8 ERROR] C:/agent/_work/1/s/tools/test\test_custom_ops_pytorch_exporter.py:136:37: E128 continuation line under-indented for visual indent

@snnn snnn merged commit 37cbe85 into microsoft:master Jun 26, 2020
@pranavsharma pranavsharma mentioned this pull request Aug 30, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@pranavsharma pranavsharma pranavsharma left review comments

@BowenBao BowenBao BowenBao left review comments

@thiagocrepaldi thiagocrepaldi thiagocrepaldi approved these changes

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.

6 participants

@neginraoof @pranavsharma @jywu-msft @thiagocrepaldi @snnn @BowenBao