Skip to content
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.

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

[1/2] Wean off of PYBIND in favor of torch.ops.load_library #1276

Merged
merged 1 commit into from
Nov 13, 2024
Merged

Conversation

janeyx99
Copy link
Contributor

@janeyx99 janeyx99 commented Nov 13, 2024

The no ghstack version of #1275 to see if the mac tests pass (they do).

Pave the path for python agnostic ao by removing depending on PYBIND (which is not python agnostic).

Concretely, what happened this PR?

  • no more PYBIND, so no more init.cpp
  • for all non-CUDA platforms, ao no longer has custom cpp extensions, and thus ao is a pure python lib
  • so we skip auditwheel (which is used for when the platform is linux) for all non-cuda wheel builds. (This does also mean we now needlessly build the same python wheel for every other platform rocm, cpu, xpu...)
  • no more PYBIND also means no more torchao._C, which we're replacing with a load_library call

This PR should have no failures. The next PR will be targeting the wheel process to only output one wheel for every python version. If you're curious, the next PR looks like #1277

Copy link

pytorch-bot bot commented Nov 13, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/1276

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

✅ No Failures

As of commit acd19e0 with merge base f96e5ec (image):
💚 Looks good so far! There are no failures yet. 💚

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 Nov 13, 2024
@janeyx99 janeyx99 added the topic: for developers Use this tag if this PR is mainly developer facing label Nov 13, 2024
@janeyx99 janeyx99 marked this pull request as ready for review November 13, 2024 19:16
WHEEL_NAME=$(ls dist/)
# Prepare manywheel, only for CUDA.
# The wheel is a pure python wheel for other platforms.
if [[ "$CU_VERSION" == cu* ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

is this also true for rocm?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, but it looks like the current .cu files are not built for rocm based on the use_cuda here https://github.com/pytorch/ao/blob/main/setup.py#L64

Copy link
Member

Choose a reason for hiding this comment

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

Ok! heads up to @petrex who has been working on adding custom hip kernel support

Copy link
Collaborator

Choose a reason for hiding this comment

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

@janeyx99 This #1201 would build hip kernels.

@janeyx99 janeyx99 merged commit c546c5c into main Nov 13, 2024
45 of 46 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. topic: for developers Use this tag if this PR is mainly developer facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants