[to_edge] Allow core aten op exception list #5237
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/5237
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 96219bc with merge base cac2c05 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
larryliu0820
requested review from
tarun292,
zhxchen17 and
Gasoonjia
and removed request for
tarun292
|
@larryliu0820 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Why dont we just fix these when they happen then instead of allowing people to bypass? Fundamentally Im unsure of the value of a verifier if the recommended course of action when it fails is just to bypass. If bypassing it is fine then why not just always bypass? If its not fine in general why is it fine for this specific user who its failing for? |
Summary: Currently when a non-core ATen operator shows up in the exported graph, `to_edge()` will fail and the only option is to disable IR validity check by setting `_check_ir_validity=False`. However this is unsafe to do, instead we should still run the rest of the checks. This PR adds support to allow users to bypass core ATen ops check, by passing a list of non-core ATen ops into `to_edge()`. Note that: * This is different than `ops_set_to_not_decompose` in `to_edge_transform_and_lower`, as the ops in `_core_aten_ops_exception_list` are not intended to be kept but more likely showing up because of missing decompositions or missing core ATen tag in `native_functions.yaml`. For this reason, we are combining two lists (`ops_set_to_not_decompose` and `_core_aten_ops_exception_list`) and pass to verifier. * I updated the error log to encourage people to use `_core_aten_ops_exception_list` instead of using `_check_ir_validity=False`. Test Plan: Added unit test Reviewed By: tarun292 Differential Revision: D62469015 Pulled By: larryliu0820
facebook-github-bot
force-pushed
the
config_exception
branch
from
e7cb0f0 to
806c030
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62469015 |
Summary: Currently when a non-core ATen operator shows up in the exported graph, `to_edge()` will fail and the only option is to disable IR validity check by setting `_check_ir_validity=False`. However this is unsafe to do, instead we should still run the rest of the checks. This PR adds support to allow users to bypass core ATen ops check, by passing a list of non-core ATen ops into `to_edge()`. Note that: * This is different than `ops_set_to_not_decompose` in `to_edge_transform_and_lower`, as the ops in `_core_aten_ops_exception_list` are not intended to be kept but more likely showing up because of missing decompositions or missing core ATen tag in `native_functions.yaml`. For this reason, we are combining two lists (`ops_set_to_not_decompose` and `_core_aten_ops_exception_list`) and pass to verifier. * I updated the error log to encourage people to use `_core_aten_ops_exception_list` instead of using `_check_ir_validity=False`. Test Plan: Added unit test Reviewed By: tarun292 Differential Revision: D62469015 Pulled By: larryliu0820
facebook-github-bot
force-pushed
the
config_exception
branch
from
806c030 to
4cc0f26
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62469015 |
It takes time for us to implement decompositions or adding an operator into core ATen. Bypassing core ATen op check doesn't mean we are bypassing all other checks in the verifier, it's an escape patch for a user to express "I know this op exists in the graph that is not core ATen and there's no portable kernel for it. I'm confident it will be handled downstream". |
Summary: Currently when a non-core ATen operator shows up in the exported graph, `to_edge()` will fail and the only option is to disable IR validity check by setting `_check_ir_validity=False`. However this is unsafe to do, instead we should still run the rest of the checks. This PR adds support to allow users to bypass core ATen ops check, by passing a list of non-core ATen ops into `to_edge()`. Note that: * This is different than `ops_set_to_not_decompose` in `to_edge_transform_and_lower`, as the ops in `_core_aten_ops_exception_list` are not intended to be kept but more likely showing up because of missing decompositions or missing core ATen tag in `native_functions.yaml`. For this reason, we are combining two lists (`ops_set_to_not_decompose` and `_core_aten_ops_exception_list`) and pass to verifier. * I updated the error log to encourage people to use `_core_aten_ops_exception_list` instead of using `_check_ir_validity=False`. Test Plan: Added unit test Reviewed By: tarun292 Differential Revision: D62469015 Pulled By: larryliu0820
facebook-github-bot
force-pushed
the
config_exception
branch
from
4cc0f26 to
96219bc
Compare
|
This pull request was exported from Phabricator. Differential Revision: D62469015 |
|
@larryliu0820 merged this pull request in 7942d2c. |
Summary: Currently when a non-core ATen operator shows up in the exported graph,
to_edge()will fail and the only option is to disable IR validity check by setting_check_ir_validity=False. However this is unsafe to do, instead we should still run the rest of the checks.This PR adds support to allow users to bypass core ATen ops check, by passing a list of non-core ATen ops into
to_edge().Note that:
ops_set_to_not_decomposeinto_edge_transform_and_lower, as the ops in_core_aten_ops_exception_listare not intended to be kept but more likely showing up because of missing decompositions or missing core ATen tag innative_functions.yaml. For this reason, we are combining two lists (ops_set_to_not_decomposeand_core_aten_ops_exception_list) and pass to verifier._core_aten_ops_exception_listinstead of using_check_ir_validity=False.Test Plan: Added unit test
Reviewers:
Subscribers:
Tasks:
Tags: