Skip to content

Update symbolics policy to emit aten::ATen for Caffe2 build only#74680

Closed
thiagocrepaldi wants to merge 4 commits intopytorch:masterfrom
thiagocrepaldi:thiagofc/gen-aten-ops-for-caffe2-on-symbolics
Closed

Update symbolics policy to emit aten::ATen for Caffe2 build only#74680
thiagocrepaldi wants to merge 4 commits intopytorch:masterfrom
thiagocrepaldi:thiagofc/gen-aten-ops-for-caffe2-on-symbolics

Conversation

@thiagocrepaldi
Copy link
Collaborator

@thiagocrepaldi thiagocrepaldi commented Mar 24, 2022

Currently ONNX exporter symbolics can emit ATen operators when operator_export_type==ONNX_ATEN_FALLBACK. However, this is a behavior specific to Caffe2 builds, as the intend use of ONNX_ATEN_FALLBACK is to emit ATen operators only when there is no ONNX equivalent.

The reason Caffe2 choses to emit ATen operators when ONNX counterpart exists is for performance on their particular engine implementation, which might not be true for other implementations. e.g. ONNX Runtime can optimize the generated ONNX graph into something more efficient

This PR must be merged only after #73954

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Mar 24, 2022 •
edited
Loading

🔗 Helpful links

💊 CI failures summary and remediations

As of commit 97f380d (more details on the Dr. CI page):

💚 💚 Looks good so far! There are no failures yet. 💚 💚

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@facebook-github-bot facebook-github-bot added the oncall: jit Add this issue/PR to JIT oncall triage queue label Mar 24, 2022
@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/gen-aten-ops-for-caffe2-on-symbolics branch 3 times, most recently from f8f3b7d to 86ce15c Compare March 24, 2022 20:25
@thiagocrepaldi thiagocrepaldi changed the title Update symbolics to emit aten::ATen for Caffe2 build only Update symbolics policy to emit aten::ATen for Caffe2 build only Mar 25, 2022
@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/gen-aten-ops-for-caffe2-on-symbolics branch from 86ce15c to 6bba258 Compare March 28, 2022 15:35
@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/gen-aten-ops-for-caffe2-on-symbolics branch 5 times, most recently from 984e567 to 3efb270 Compare April 15, 2022 19:16
@thiagocrepaldi thiagocrepaldi marked this pull request as ready for review April 18, 2022 14:25
@thiagocrepaldi thiagocrepaldi added module: onnx Related to torch.onnx enhancement Not as big of a feature, but technically not a bug. Should be easy to fix release notes: onnx torch.onnx related changes that should show up in the release notes topic: improvements topic category labels Apr 18, 2022
@thiagocrepaldi thiagocrepaldi requested a review from garymm April 18, 2022 15:17
garymm
garymm reviewed Apr 18, 2022
View reviewed changes
Copy link
Collaborator

@garymm garymm left a comment

Choose a reason for hiding this comment

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

Thanks!

@garymm garymm self-assigned this Apr 18, 2022
Update symbolics to emit aten::ATen for Caffe2 build only
1b6b096 
Signed-off-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>
@thiagocrepaldi thiagocrepaldi requested a review from garymm April 18, 2022 17:53
@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/gen-aten-ops-for-caffe2-on-symbolics branch from 3efb270 to 977a7da Compare April 18, 2022 18:32
garymm
garymm approved these changes Apr 18, 2022
View reviewed changes
Copy link
Collaborator

@garymm garymm left a comment

Choose a reason for hiding this comment

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

⛈️

@thiagocrepaldi
Copy link
Collaborator Author

thiagocrepaldi commented Apr 19, 2022

@pytorchmergebot merge this

@pytorchmergebot
Copy link
Collaborator

pytorchmergebot commented Apr 19, 2022

Merge failed due to Matched rule superuser, but it was not reviewed yet by any of:glaringlee,anjali411,simpkins,bugra,dzhulgakov, ...
Raised by https://github.com/pytorch/pytorch/actions/runs/2189719980

@thiagocrepaldi thiagocrepaldi added the onnx-needs-import This PR is related to ONNX, but touches files outside of merge rule patterns, and hence needs import label Apr 19, 2022
@thiagocrepaldi
Copy link
Collaborator Author

thiagocrepaldi commented Apr 19, 2022 •
edited
Loading

@malfet adding you as we need to test/jit/test_export_modes.py onnx test in the merge_rules.json - or we can move it to onnx test folder (from jit) with your approval

Also asking permission for aten/src/ATen/core/interned_strings.h as we often need to add ONNX operators names to this file, such as _(onnx, Min) at #75861

@thiagocrepaldi
Copy link
Collaborator Author

thiagocrepaldi commented Apr 19, 2022

@pytorchmergebot merge this

@thiagocrepaldi thiagocrepaldi linked an issue Apr 19, 2022 that may be closed by this pull request
[ONNX] ONNX_ATEN_FALLBACK supports ONNX Runtime #75623
Closed
facebook-github-bot pushed a commit that referenced this pull request Apr 19, 2022
@facebook-github-bot
Update symbolics policy to emit aten::ATen for Caffe2 build only (#74680
f304e84 
)

Summary:
Currently ONNX exporter symbolics can emit ATen operators when `operator_export_type==ONNX_ATEN_FALLBACK`. However, this is a behavior specific to Caffe2 builds, as the intend use of `ONNX_ATEN_FALLBACK` is to emit ATen operators only when there is no ONNX equivalent.

The reason Caffe2 choses to emit ATen operators when ONNX counterpart exists is for performance on their particular engine implementation, which might not be true for other implementations. e.g. ONNX Runtime can optimize the generated ONNX graph into something more efficient

This PR must be merged only after #73954

Pull Request resolved: #74680
Approved by: https://github.com/garymm, https://github.com/malfet

Test Plan: contbuild & OSS CI, see https://hud.pytorch.org/commit/pytorch/pytorch/eab3f428833ce9e11e2db97e516fe4b78ea2e4dc

Reviewed By: seemethere

Differential Revision: D35751465

fbshipit-source-id: 079d2a9db7174e68a9f8740f08364a598630dabc
malfet pushed a commit that referenced this pull request Apr 20, 2022
@malfet
Update symbolics policy to emit aten::ATen for Caffe2 build only
83208e7 
Currently ONNX exporter symbolics can emit ATen operators when `operator_export_type==ONNX_ATEN_FALLBACK`. However, this is a behavior specific to Caffe2 builds, as the intend use of `ONNX_ATEN_FALLBACK` is to emit ATen operators only when there is no ONNX equivalent.

The reason Caffe2 choses to emit ATen operators when ONNX counterpart exists is for performance on their particular engine implementation, which might not be true for other implementations. e.g. ONNX Runtime can optimize the generated ONNX graph into something more efficient

This PR must be merged only after #73954
Pull Request resolved: #74680
Approved by: https://github.com/garymm, https://github.com/malfet

(cherry picked from commit eab3f42)
@thiagocrepaldi thiagocrepaldi deleted the thiagofc/gen-aten-ops-for-caffe2-on-symbolics branch May 4, 2023 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@garymm garymm garymm approved these changes

@malfet malfet malfet approved these changes

@BowenBao BowenBao Awaiting requested review from BowenBao

@shubhambhokare1 shubhambhokare1 Awaiting requested review from shubhambhokare1

@seemethere seemethere Awaiting requested review from seemethere

@janeyx99 janeyx99 Awaiting requested review from janeyx99

@atalman atalman Awaiting requested review from atalman

Assignees

@garymm garymm

Labels

cla signed enhancement Not as big of a feature, but technically not a bug. Should be easy to fix module: onnx Related to torch.onnx oncall: jit Add this issue/PR to JIT oncall triage queue onnx-needs-import This PR is related to ONNX, but touches files outside of merge rule patterns, and hence needs import open source release notes: onnx torch.onnx related changes that should show up in the release notes topic: improvements topic category

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[ONNX] ONNX_ATEN_FALLBACK supports ONNX Runtime

6 participants

@thiagocrepaldi @facebook-github-bot @pytorchmergebot @garymm @malfet @pytorchbot