Skip to content

Reuse GELU implementation from PyTorch core #8322

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
Merged
merged 3 commits into from
Feb 12, 2025
Merged

Reuse GELU implementation from PyTorch core #8322

merged 3 commits into from
Feb 12, 2025

Conversation

pytorchbot
Copy link
Collaborator

This PR was created by the merge bot to help merge the original PR into the main branch.
ghstack PR number: #7041
^ Please use this as the source of truth for the PR details, comments, and reviews
ghstack PR base: https://github.com/pytorch/executorch/tree/gh/swolchok/120/base
ghstack PR head: https://github.com/pytorch/executorch/tree/gh/swolchok/120/head
Merge bot PR base: https://github.com/pytorch/executorch/tree/gh/swolchok/124/orig
Merge bot PR head: https://github.com/pytorch/executorch/tree/gh/swolchok/120/orig
@diff-train-skip-merge

@pytorch-bot PyTorch Bot
Copy link

pytorch-bot bot commented Feb 7, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/8322

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure, 5 Pending

As of commit 2c10b96 with merge base 89dc36c (image):

NEW FAILURE - The following job has failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@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 Feb 7, 2025
Base automatically changed from gh/swolchok/124/orig to main February 11, 2025 19:19
@lucylq
Copy link
Contributor

lucylq commented Feb 11, 2025

Updated the base to main

@swolchok
Copy link
Contributor

I discovered one typo during conflict resolution already, waiting for at least unittest jobs to come back

@swolchok
Copy link
Contributor

swolchok commented Feb 11, 2025 •
edited
Loading

I cannot repro the build failures on this PR with a linux machine. ./install_executorch.sh --pybind xnnpack --use-pt-pinned-commit is failing; no repro with or without --use-pt-pinned-commit. (I am not running the full CI flow and I am using venv instead of conda, which may be why.) @huydhn they're MKL-related and you touched MKL lately, any ideas? diff train is blocked :\

@swolchok
Copy link
Contributor

trying to see if the following workflow reproduces:

  • pip uninstall torch
  • check out the pinned pytorch commit in local pytorch, python setup.py develop
  • go back to executorch, ./install_executorch.py --pybind xnnpack --use-pt-pinned-commit

@swolchok
Copy link
Contributor

following workflow reproduces

nope. install_executorch.py uninstalls torch installed this way. also getting ImportError: /data/users/swolchok/pytorch/torch/lib/libtorch_python.so: undefined symbol: _PyThreadState_GetCurrent so apparently the way I'm building pytorch is somehow wrong :\

@huydhn
Copy link
Contributor

huydhn commented Feb 11, 2025

I cannot repro the build failures on this PR with a linux machine. ./install_executorch.sh --pybind xnnpack --use-pt-pinned-commit is failing; no repro with or without --use-pt-pinned-commit. (I am not running the full CI flow and I am using venv instead of conda, which may be why.) @huydhn they're MKL-related and you touched MKL lately, any ideas? diff train is blocked :\

I haven't been able to connect the dots yet, but let me take a closer look

@github-actions github-actions bot mentioned this pull request Feb 11, 2025
Open
@huydhn huydhn mentioned this pull request Feb 12, 2025
Merged
swolchok pushed a commit that referenced this pull request Feb 12, 2025
@huydhn
Fix multiple find_package(Torch) calls (#8407)
1308d4d 
While debugging the build issue on #8322 w.r.t mkl, I undercover a complex interaction between #8322, #8248 (to install mkl), and https://github.com/pytorch/pytorch/blob/main/cmake/public/mkl.cmake from PyTorch.  The error is as follows:

```
CMake Error at /opt/conda/envs/py_3.10/lib/cmake/mkl/MKLConfig.cmake:744 (add_library): <-- This file comes from conda mkl
  add_library cannot create imported target "MKL::MKL" because another target
  with the same name already exists.
Call Stack (most recent call first):
  /opt/conda/envs/py_3.10/lib/python3.10/site-packages/torch/share/cmake/Caffe2/public/mkl.cmake:1 (find_package) <-- this is from PyTorch
  /opt/conda/envs/py_3.10/lib/python3.10/site-packages/torch/share/cmake/Caffe2/Caffe2Config.cmake:106 (include)
  /opt/conda/envs/py_3.10/lib/python3.10/site-packages/torch/share/cmake/Torch/TorchConfig.cmake:68 (find_package)
  CMakeLists.txt:753 (find_package)
```

The conclusion is that, with mkl installed, there should be just one `find_package(Torch)` call because the mkl target is defined globally.  The `torch` target, on the other hand, is only defined locally.

So, this change adds `if(NOT TARGET torch)` check to only call `find_package(Torch)` if needed.

### Testing

The change on top of #8322 looks like this f705b01

https://github.com/pytorch/executorch/actions/runs/13278590926?pr=8399
@swolchok swolchok force-pushed the gh/swolchok/120/orig branch from d7c261d to 0d95292 Compare February 12, 2025 17:55
@swolchok
Copy link
Contributor

rebased the branch locally, fixed conflicts, and force-pushed. hopefully we should acutlaly have picked up #8407 and will be able to land soon.

Github Executorch and others added 3 commits February 12, 2025 11:34
@swolchok
Reuse GELU implementation from PyTorch core
a33092f 
Pull Request resolved: #7041

kernels/optimized doesn't need to support embedded systems, so it can just take a header-only dep on PyTorch.

Note that, because we will pick up Sleef internally and ignore it
externally thanks to ATen vec, this PR gets to enable optimized GELU in OSS.

Testing: CI to make sure this doesn't break mobile build modes; happy to take advice on anything not currently covered that might break.
ghstack-source-id: 265190627
@exported-using-ghexport

Differential Revision: [D66335522](https://our.internmc.facebook.com/intern/diff/D66335522/)
@swolchok
apply @huydhn's fix to new code
57f2265
@swolchok
extra tweak from @huydhn
2c10b96
@swolchok swolchok force-pushed the gh/swolchok/120/orig branch from b9e7ddb to 2c10b96 Compare February 12, 2025 19:43
@swolchok swolchok merged commit 6727fea into main Feb 12, 2025
138 of 139 checks passed
@swolchok swolchok deleted the gh/swolchok/120/orig branch February 12, 2025 20:35
@github-actions github-actions bot mentioned this pull request Feb 17, 2025
Open
This was referenced Feb 24, 2025
Open
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucylq lucylq lucylq approved these changes

@tarun292 tarun292 tarun292 approved these changes

@kirklandsign kirklandsign kirklandsign approved these changes

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

Successfully merging this pull request may close these issues.
7 participants
@pytorchbot @lucylq @swolchok @huydhn @tarun292 @kirklandsign @facebook-github-bot