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

[iOS] added workflows for libtorchvision_ops.a binary build #3582

Merged
merged 10 commits into from
Mar 30, 2021

Conversation

husthyc
Copy link
Contributor

@husthyc husthyc commented Mar 18, 2021

Stack from ghstack:

husthyc added a commit that referenced this pull request Mar 18, 2021
ghstack-source-id: e333d1496d8a1bd0dca230c8fe8290b2baca0a14
Pull Request resolved: #3582
@husthyc husthyc requested review from fmassa and xta0 March 18, 2021 05:02
Copy link
Member

@fmassa fmassa left a 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?

Comment on lines 45 to 52
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
Copy link
Member

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?

Copy link
Contributor Author

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.

Comment on lines 27 to 44
# 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"
Copy link
Member

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?

Copy link
Contributor Author

@husthyc husthyc Mar 26, 2021

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

husthyc added a commit that referenced this pull request Mar 26, 2021
ghstack-source-id: e4abc12d4d7e0a11bef088a57b91a657af4080fe
Pull Request resolved: #3582
husthyc added a commit that referenced this pull request Mar 26, 2021
ghstack-source-id: 4f8968fa38915600c06e18745d7ab08024936cc7
Pull Request resolved: #3582
@husthyc
Copy link
Contributor Author

husthyc commented Mar 26, 2021

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?

Fixed.

husthyc added a commit that referenced this pull request Mar 26, 2021
ghstack-source-id: c8e299f0165ff7894840449e51f85035e7a6a57f
Pull Request resolved: #3582
husthyc added a commit that referenced this pull request Mar 29, 2021
ghstack-source-id: 55211ac70ac1ea9e920e7d079b7a28efc9c2db9e
Pull Request resolved: #3582
husthyc added a commit that referenced this pull request Mar 29, 2021
ghstack-source-id: 26e99f8397ea9760111215ad2e1e1a016d883322
Pull Request resolved: #3582
husthyc added a commit that referenced this pull request Mar 29, 2021
ghstack-source-id: 100f558df75c9de207ad53c40d2867d9c1d91322
Pull Request resolved: #3582
husthyc added a commit that referenced this pull request Mar 29, 2021
ghstack-source-id: e57ef604e70bc5e4b2b360cc2385087b03ade085
Pull Request resolved: #3582
husthyc added a commit that referenced this pull request Mar 29, 2021
ghstack-source-id: 43c9896daa7fd701106e3954af2963d0c85e7d3f
Pull Request resolved: #3582
@fmassa fmassa changed the base branch from gh/husthyc/2/base to master March 30, 2021 08:51
Copy link
Member

@fmassa fmassa left a 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!

@fmassa fmassa merged commit 897ac9c into master Mar 30, 2021
@fmassa fmassa deleted the gh/husthyc/2/head branch March 30, 2021 08:53
facebook-github-bot pushed a commit that referenced this pull request Apr 2, 2021
…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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants