Skip to content

Add new dependency library for vulkan tests #10136

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

Merged
merged 1 commit into from
Apr 17, 2025
Merged

Conversation

FFFrog
Copy link
Contributor

@FFFrog FFFrog commented Apr 14, 2025

Background:

We removed the inline flag of the symbol torch::jit::GetBackendMetaSerialization() from and moved it to .cpp in order to make the dynamic registration related to PyTorch serialization work properly.

However, after testing, it was found that Vulkan's tests depend on the symbol named torch::jit::GetBackendMetaSerialization(), so after the above changes, it is necessary to explicitly link libtorch_cpu (which contains the torch::jit::GetBackendMetaSerialization() symbol)

For more information, see PR, especially this comment

cc @SS-JIA @manuelcandales @cbilgin

**Background**:

We removed the inline flag of the symbol `torch::jit::GetBackendMetaSerialization()` from and moved it to .cpp in order to make the dynamic registration related to PyTorch serialization work properly.

However, after testing, it was found that Vulkan's tests depend on the symbol named `torch::jit::GetBackendMetaSerialization()`, so after the above changes, it is necessary to explicitly link libtorch_cpu (which contains the torch::jit::GetBackendMetaSerialization() symbol)

For more information, see [PR](pytorch/pytorch#147095), especially this [comment](pytorch/pytorch#147095 (comment))
@FFFrog FFFrog requested a review from SS-JIA as a code owner April 14, 2025 11:57
Copy link

pytorch-bot bot commented Apr 14, 2025

🔗 Helpful Links

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

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

✅ No Failures

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

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

@pytorch-bot pytorch-bot bot added the module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/ label Apr 14, 2025
@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 Apr 14, 2025
@FFFrog
Copy link
Contributor Author

FFFrog commented Apr 14, 2025

@pytorchbot label "topic: not user facing"

Copy link
Contributor

@SS-JIA SS-JIA left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! LGTM

@FFFrog
Copy link
Contributor Author

FFFrog commented Apr 15, 2025

@SS-JIA Thanks a lot.
By the way, how can I merge this PR into the trunk? This PR is waiting for this one to be merged first.

@FFFrog
Copy link
Contributor Author

FFFrog commented Apr 16, 2025

Hey! So sorry to bother @kirklandsign @shoumikhin @SS-JIA, could you please help to trigger the workflow and merge this pr into the trunk?

@SS-JIA
Copy link
Contributor

SS-JIA commented Apr 17, 2025

@FFFrog apologies for the delayed response. Merging now.

@SS-JIA SS-JIA merged commit cb80092 into pytorch:main Apr 17, 2025
5 of 8 checks passed
@kirklandsign
Copy link
Contributor

@FFFrog @SS-JIA seems this one broke internal test. Maybe revert, and test and land from internal first later?

@FFFrog
Copy link
Contributor Author

FFFrog commented Apr 18, 2025

@FFFrog apologies for the delayed response. Merging now.

Thank you a lot.

@FFFrog
Copy link
Contributor Author

FFFrog commented Apr 18, 2025

@FFFrog @SS-JIA seems this one broke internal test. Maybe revert, and test and land from internal first later?

Hey @kirklandsign , so sorry for breaking the internal tests, could you mind to show me the detailed information about the failure?

@SS-JIA
Copy link
Contributor

SS-JIA commented Apr 18, 2025

@FFFrog no worries. To address it properly I think we will need to test against the Meta internal repo. I will try to implement your changes in the internal repo and figure out a way to do it that doesn't break the internal tests.

@FFFrog
Copy link
Contributor Author

FFFrog commented Apr 18, 2025

@SS-JIA Thank you very much,please let me know if any progress.

keyprocedure pushed a commit to keyprocedure/executorch that referenced this pull request Apr 21, 2025
**Background**:

We removed the inline flag of the symbol
`torch::jit::GetBackendMetaSerialization()` from and moved it to .cpp in
order to make the dynamic registration related to PyTorch serialization
work properly.

However, after testing, it was found that Vulkan's tests depend on the
symbol named `torch::jit::GetBackendMetaSerialization()`, so after the
above changes, it is necessary to explicitly link libtorch_cpu (which
contains the torch::jit::GetBackendMetaSerialization() symbol)

For more information, see
[PR](pytorch/pytorch#147095), especially this
[comment](pytorch/pytorch#147095 (comment))

cc @SS-JIA @manuelcandales @cbilgin
keyprocedure pushed a commit to keyprocedure/executorch that referenced this pull request Apr 21, 2025
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. module: vulkan Issues related to the Vulkan delegate and code under backends/vulkan/ topic: not user facing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants