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

cmake torchao_ops_mps_linear_fp_act_xbit_weight #1304

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

manuelcandales
Copy link
Contributor

Summary: Move from setup.py to cmake for building custom torchao mps ops

Differential Revision: D66120124

Copy link

pytorch-bot bot commented Nov 18, 2024

🔗 Helpful Links

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

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit a388689 with merge base ca52cdc (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 18, 2024
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D66120124

@manuelcandales manuelcandales added the topic: not user facing Use this tag if you don't want this PR to show up in release notes label Nov 18, 2024
target_compile_definitions(torchao_ops_mps_linear_fp_act_xbit_weight_aten PRIVATE USE_ATEN=1)

# Enable Metal support
if (APPLE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to gate from specific version of OS? I think we should. both iOS and MacOS

Copy link
Contributor

@kimishpatel kimishpatel left a comment

Choose a reason for hiding this comment

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

mostly nits, but one question on file removal

-B ${CMAKE_OUT}
cmake --build ${CMAKE_OUT} -j 16 --target install --config Release

rm ../../kernels/mps/src/metal_shader_lib.h
Copy link
Contributor

Choose a reason for hiding this comment

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

wait what is this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

after I am done building and installing, I don't need the generated metal_shader_lib.h anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok then generated artifacts should be stored in build directory as temporary artifacts or somewhere else. Adding in the repo folder and removing it can leave the repo in dirty state if build fails. Also generally clean

torchao/experimental/ops/mps/build.sh Outdated Show resolved Hide resolved

python ../../kernels/mps/codegen/gen_metal_shader_lib.py

export CMAKE_PREFIX_PATH=$(python -c 'from distutils.sysconfig import get_python_lib; print(get_python_lib())')
Copy link
Contributor

Choose a reason for hiding this comment

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

why cmake prefix path points to site-packages?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's where the cmake stuff is. I am working on a conda environment.

Copy link
Contributor

Choose a reason for hiding this comment

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

what do you mean thats where the cmake stuff is. prefix path is used for looking up packages https://cmake.org/cmake/help/latest/variable/CMAKE_PREFIX_PATH.html and other stuff

Comment on lines 11 to 16
path_libtorchao_ops_mps_aten = os.path.abspath(
os.path.join(
os.path.dirname(__file__), "../cmake-out/lib/libtorchao_ops_mps_linear_fp_act_xbit_weight_aten.dylib"
)
)
torch.ops.load_library(path_libtorchao_ops_mps_aten)
Copy link
Contributor

Choose a reason for hiding this comment

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

  • If and when we integrate experimental with torchao's pip installation, like pip install . or setup then at that type this lib should be installed in site-packages or some place like that instead of making it available via cmake-out in this way
  • Put it under try catch and throw error message if not found

Copy link
Contributor

@kimishpatel kimishpatel left a comment

Choose a reason for hiding this comment

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

mostly nits, but one question on file removal

@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D66120124

manuelcandales added a commit to manuelcandales/ao that referenced this pull request Nov 19, 2024
Summary:
Pull Request resolved: pytorch#1304

Move from setup.py to cmake for building custom torchao mps ops

Differential Revision: D66120124
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D66120124

manuelcandales added a commit to manuelcandales/ao that referenced this pull request Nov 19, 2024
Summary:
Pull Request resolved: pytorch#1304

Move from setup.py to cmake for building custom torchao mps ops

Differential Revision: D66120124
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D66120124

manuelcandales added a commit to manuelcandales/ao that referenced this pull request Nov 19, 2024
Summary:
Pull Request resolved: pytorch#1304

Move from setup.py to cmake for building custom torchao mps ops

Differential Revision: D66120124
manuelcandales added a commit to manuelcandales/ao that referenced this pull request Nov 20, 2024
Summary:
Pull Request resolved: pytorch#1304

Move from setup.py to cmake for building custom torchao mps ops

Differential Revision: D66120124
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D66120124

manuelcandales added a commit to manuelcandales/ao that referenced this pull request Nov 20, 2024
Summary:
Pull Request resolved: pytorch#1304

Move from setup.py to cmake for building custom torchao mps ops

Differential Revision: D66120124
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D66120124

Copy link
Contributor

@kimishpatel kimishpatel left a comment

Choose a reason for hiding this comment

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

Stamping for now. Do consider addressing comments before landing

torchao/experimental/ops/mps/build.sh Outdated Show resolved Hide resolved
torchao/experimental/ops/mps/test/test_lowbit.py Outdated Show resolved Hide resolved
torchao/experimental/ops/mps/test/test_lowbit.py Outdated Show resolved Hide resolved
torchao/experimental/ops/mps/test/test_quantizer.py Outdated Show resolved Hide resolved
torchao/experimental/ops/mps/test/test_quantizer.py Outdated Show resolved Hide resolved
torchao/experimental/ops/mps/test/test_quantizer.py Outdated Show resolved Hide resolved
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D66120124

manuelcandales added a commit to manuelcandales/ao that referenced this pull request Nov 20, 2024
Summary:
Pull Request resolved: pytorch#1304

Move from setup.py to cmake for building custom torchao mps ops

Differential Revision: D66120124
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D66120124

manuelcandales added a commit to manuelcandales/ao that referenced this pull request Nov 20, 2024
Summary:
Pull Request resolved: pytorch#1304

Move from setup.py to cmake for building custom torchao mps ops

Differential Revision: D66120124
manuelcandales added a commit to manuelcandales/ao that referenced this pull request Nov 21, 2024
Summary:
Pull Request resolved: pytorch#1304

Move from setup.py to cmake for building custom torchao mps ops

Differential Revision: D66120124
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D66120124

1 similar comment
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D66120124

manuelcandales added a commit to manuelcandales/ao that referenced this pull request Nov 21, 2024
Summary:
Pull Request resolved: pytorch#1304

Move from setup.py to cmake for building custom torchao mps ops

Reviewed By: kimishpatel

Differential Revision: D66120124
Summary:
Pull Request resolved: pytorch#1304

Move from setup.py to cmake for building custom torchao mps ops

Reviewed By: kimishpatel

Differential Revision: D66120124
@facebook-github-bot
Copy link

This pull request was exported from Phabricator. Differential Revision: D66120124

@facebook-github-bot facebook-github-bot merged commit 7489c7d into pytorch:main Nov 21, 2024
19 of 20 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. fb-exported topic: not user facing Use this tag if you don't want this PR to show up in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants