Build optimized operators lib with -fexceptions #7546
Conversation
It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/7546
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New Failure, 2 Cancelled JobsAs of commit 8e042ac with merge base 8494b90 ( NEW FAILURE - The following job has failed:
CANCELLED JOBS - The following jobs were cancelled. Please retry:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
Pull Request resolved: #7546 It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? ghstack-source-id: 260556841 @exported-using-ghexport Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/)
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
swolchok
added
the
release notes: build
|
converting to draft to prevent accidental merge until the PR is updated to include a proper PyTorch pin bump; right now it points directly at pytorch/pytorch#144341 . It will still be reviewable whenever I get CI fixed |
build/Codegen.cmake
Outdated
| ) | ||
| ) | ||
| if(GEN_ADD_EXCEPTION_BOUNDARY) | ||
| set(_gen_command "${_gen_command} --add-exception-boundary") |
There was a problem hiding this comment.
I think the set command use semi-colon separator instead of space, i.e. /opt/conda/envs/py_3.10/bin/python3;-m;torchgen.gen_executorch;--source-path=/pytorch/executorch/configurations/../codegen;--install-dir=/pytorch/executorch/pip-out/temp.linux-x86_64-cpython-310/cmake-out/configurations/optimized_native_cpu_ops_lib;--tags-path=/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/packaged/ATen/native/tags.yaml;--aten-yaml-path=/opt/conda/envs/py_3.10/lib/python3.10/site-packages/torchgen/packaged/ATen/native/native_functions.yaml;--op-selection-yaml-path=/pytorch/executorch/pip-out/temp.linux-x86_64-cpython-310/cmake-out/configurations/optimized_native_cpu_ops_lib/selected_operators.yaml;--add-exception-boundary
The failure is complaining about a wrong filename https://github.com/pytorch/executorch/actions/runs/12660296601/job/35287265644#step:14:1992
| set(_gen_command "${_gen_command} --add-exception-boundary") | |
| set(_gen_command "${_gen_command}" --add-exception-boundary) |
There was a problem hiding this comment.
strange, I thought I built this locally. Giving it a shot now.
There was a problem hiding this comment.
this does seem to have improved the situation (thanks!) but the torchgen version still seems to be wrong: https://github.com/pytorch/executorch/actions/runs/12680876952/job/35343491373?pr=7546#step:9:2186
There was a problem hiding this comment.
@huydhn still seeing at least some wrong-torchgen-version failures:
- test-arm-backend-delegation
- test-arm-reference-delegation
- test-llava-runner-linux
- test-phi-3-mini-runner-linux
- test-eval_llama-wikitext-linux
- test-eval_llama-mmlu-linux
- test-llama_runner_eager-linux
however, we do have mac successes. looking into the common thread for these failures now.
There was a problem hiding this comment.
the ARM one is easy: the workflow directly invokes install_executorch from .ci/scripts/utils.sh without "use-pt-pinned-commit". Is there a reason "use-pt-pinned-commit" isn't the default or only option?
test-llava-runner-linux reruns install_requirements.sh and doesn't pass --use-pt-pinned-commit, same with test-phi-3-mini-runner, test-eval_llama-wikitext-linux, test-eval_llama-mmlu-linux, and test-llama_runner_eager-linux. I will send a PR to fix those.
It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
Pull Request resolved: #7546 It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? ghstack-source-id: 260686358 @exported-using-ghexport Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/)
It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
Pull Request resolved: #7546 It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? ghstack-source-id: 260777714 @exported-using-ghexport Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/)
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
Pull Request resolved: #7546 It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? ghstack-source-id: 260814512 @exported-using-ghexport Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/)
Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
Pull Request resolved: #7546 It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? ghstack-source-id: 260930165 @exported-using-ghexport Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/)
It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
…,reference}-delegation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
…gation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
Pull Request resolved: #7546 It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? ghstack-source-id: 260935342 @exported-using-ghexport Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/)
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
…,reference}-delegation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
…gation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
Pull Request resolved: #7546 It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? ghstack-source-id: 260988789 @exported-using-ghexport Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/)
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
…,reference}-delegation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
…gation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
…,reference}-delegation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
…gation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
It should be permissible for this library to use exceptions, since it is not required to supported embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
…,reference}-delegation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
…gation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
|
This isn't quite right. -fno-exceptions (and, to a lesser extent, -fno-rtti) should be all-or-none within a single static library or binary, otherwise one runs the risk of surprising behavior similar to that exhibited by One Definition Rule (ODR) violations. Instead, we need a mechanism that makes sure we build everything with -fexceptions if we are including optimized kernels, extensions, or any other component that could share code with PyTorch; we can only use |
|
#7736 should determine how to replace this |
It should be permissible for this library to use exceptions, since it is not required to support embedded systems. TODO: presumably we need to manage rollout of the torchgen patch? Differential Revision: [D67904052](https://our.internmc.facebook.com/intern/diff/D67904052/) [ghstack-poisoned]
|
This pull request was exported from Phabricator. Differential Revision: D67904052 |
…,reference}-delegation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
…gation" Without this, these builds don't respect the torchgen pinned commit and thus fail with #7546. Differential Revision: [D67996459](https://our.internmc.facebook.com/intern/diff/D67996459/) [ghstack-poisoned]
Stack from ghstack (oldest at bottom):
It should be permissible for this library to use exceptions, since it is not required to support embedded systems.
TODO: presumably we need to manage rollout of the torchgen patch?
Differential Revision: D67904052