-
Notifications
You must be signed in to change notification settings - Fork 7k
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
[iOS] added workflows for libtorchvision_ops.a binary build #3582
Conversation
[ghstack-poisoned]
ghstack-source-id: e333d1496d8a1bd0dca230c8fe8290b2baca0a14 Pull Request resolved: #3582
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a look at the PR and it looks great, thanks!
I have a couple of suggestions, let me know what you think.
Additionally, linter is failing https://app.circleci.com/pipelines/github/pytorch/vision/6972/workflows/02a89cd4-2c6b-453a-b859-f63895f63f03/jobs/470076 , could you fix those as well?
cd ${VISION_IOS_ROOT}/build | ||
cmake -DLIBTORCH_HEADER_ROOT=${LIBTORCH_HEADER_ROOT} \ | ||
-DCMAKE_TOOLCHAIN_FILE=${VISION_IOS_ROOT}/../cmake/iOS.cmake \ | ||
-DIOS_ARCH=${IOS_ARCH} \ | ||
-DIOS_PLATFORM=${IOS_PLATFORM} \ | ||
.. | ||
make | ||
rm -rf ${VISION_IOS_ROOT}/build |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use ios/build_ios.sh
from torchvision here instead of manually replicating that script here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a good idea, fixed.
# clone main pytorch repo | ||
mkdir -p ${VISION_IOS_ROOT}/lib | ||
mkdir -p ${VISION_IOS_ROOT}/build | ||
git clone --recursive https://github.com/pytorch/pytorch.git | ||
TORCH_ROOT="${VISION_IOS_ROOT}/pytorch" | ||
|
||
# run build script | ||
chmod a+x ${TORCH_ROOT}/scripts/build_ios.sh | ||
echo "########################################################" | ||
cat ${TORCH_ROOT}/scripts/build_ios.sh | ||
echo "########################################################" | ||
echo "IOS_ARCH: ${IOS_ARCH}" | ||
echo "IOS_PLATFORM: ${IOS_PLATFORM}" | ||
export IOS_ARCH=${IOS_ARCH} | ||
export IOS_PLATFORM=${IOS_PLATFORM} | ||
unbuffer ${TORCH_ROOT}/scripts/build_ios.sh 2>&1 | ts | ||
|
||
LIBTORCH_HEADER_ROOT="${TORCH_ROOT}/build_ios/install/include" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think there would be a way of pulling the pre-compiled PyTorch binaries for iOS here, so that we avoid compiling PyTorch? Compiling PyTorch in our CI significantly increases our CI time (the ios jobs are taking 30min to run)
What we currently do for our other torchvision CIs is that we take the latest PyTorch-nightly build (that has been uploaded to conda / pip) and use it to compile torchvision. Could we do something similar here? Like taking the latest PyTorch-ios that has been uploaded to S3 and start from there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, we can do that, but I remember you left a comment under the design doc as following:
One thing to keep in mind when building the binaries is that the pytorch used to build torchvision should be the same as the one we make available via the pytorch binaries, otherwise there can be symbol issues.
That is the reason that I and Tao decided to build PyTorch from the source code. It would be even easier if we download the PyTorch iOS binary from S3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we use the PyTorch iOS binaries from S3 to build torchvision (and the CI job times are aligned), this means that the nightlies from both torchvision and PyTorch will work, while if we pull from master this guarantee doesn't apply anymore (except if the git hashes are the same).
The comment in the doc was to try to point out exactly this: if we make available in the PyTorch nightlies a slightly different version than the one that torchvision was compiled with, there can be symbol issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
[ghstack-poisoned]
ghstack-source-id: e4abc12d4d7e0a11bef088a57b91a657af4080fe Pull Request resolved: #3582
[ghstack-poisoned]
ghstack-source-id: 4f8968fa38915600c06e18745d7ab08024936cc7 Pull Request resolved: #3582
Fixed. |
[ghstack-poisoned]
ghstack-source-id: c8e299f0165ff7894840449e51f85035e7a6a57f Pull Request resolved: #3582
[ghstack-poisoned]
ghstack-source-id: 55211ac70ac1ea9e920e7d079b7a28efc9c2db9e Pull Request resolved: #3582
[ghstack-poisoned]
ghstack-source-id: 26e99f8397ea9760111215ad2e1e1a016d883322 Pull Request resolved: #3582
[ghstack-poisoned]
ghstack-source-id: 100f558df75c9de207ad53c40d2867d9c1d91322 Pull Request resolved: #3582
[ghstack-poisoned]
ghstack-source-id: e57ef604e70bc5e4b2b360cc2385087b03ade085 Pull Request resolved: #3582
[ghstack-poisoned]
ghstack-source-id: 43c9896daa7fd701106e3954af2963d0c85e7d3f Pull Request resolved: #3582
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks a lot!
…3582) Summary: * [iOS] added workflows for libtorchvision_ops.a binary build [ghstack-poisoned] * Update on "[iOS] added workflows for libtorchvision_ops.a binary build" [ghstack-poisoned] * Update on "[iOS] added workflows for libtorchvision_ops.a binary build" [ghstack-poisoned] * Update on "[iOS] added workflows for libtorchvision_ops.a binary build" [ghstack-poisoned] * Update on "[iOS] added workflows for libtorchvision_ops.a binary build" [ghstack-poisoned] * Update on "[iOS] added workflows for libtorchvision_ops.a binary build" [ghstack-poisoned] * Update on "[iOS] added workflows for libtorchvision_ops.a binary build" [ghstack-poisoned] Reviewed By: fmassa Differential Revision: D27433924 fbshipit-source-id: 2f7b07ae21d6f57a022a4748b8ae15cd71968818 Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
Stack from ghstack: