-
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
[android] android gradle project for ops #2897
Conversation
345846c
to
807519a
Compare
c370ef2
to
872324a
Compare
In private communication, @fmassa reached out to me to see how we could avoid slapping mobile ifdefs everywhere, and was also interested if some of the restructuring in #2998 could help making this happen. For the benefit of @fmassa and @datumbox, let me just describe some of the high level constraints that feed into the C++ library design process.
I can tell you how PyTorch main library handles these things, and also mention why torchvision situation might be different.
OK, so what should you do here? TBH, the mobile ifdefs are probably a fine stopgap for now. The important thing is knowing that these don't stop you from moving to a more flexible solution later; the ifdefs are pretty mild and easily to remove later. If y'all in torchvision are planning to add a lot of operators in the near future, this matter is more pressing, but this is OK for bootstrapping. |
Awesome summary! @dreiss also brought up the topic of using selective build to replace those macros. As discussed offline, it might be similar to maskrcnn ops that we've already done. @fmassa please let us know if you want to have more details on per-app selective build! |
872324a
to
b39f365
Compare
@ezyang , Thanks a lot for your detailed answer. I read the way how Selective build works. My main intention to use ifdefs (apart excluding dependency on Python) was to save some binary size, not including backward ops. I checked with and without it - the difference is only 60K. I think, for the first iteration, that amount of savings does not worth to have ifdefs in so many places and have a big divergence desktop-mobile :) I have updated PR removing them. @fmassa Does torchvision plan to add a lot more new ops that size can grow significantly and there can be the case that we do not want to include all of them to mobile? In that case we can repeat torch selective build approach and use fixed list of ops for torchvision mobile. |
41a940c
to
4d9825f
Compare
Codecov Report
@@ Coverage Diff @@
## master #2897 +/- ##
==========================================
- Coverage 73.78% 72.75% -1.03%
==========================================
Files 103 99 -4
Lines 9507 8979 -528
Branches 1524 1431 -93
==========================================
- Hits 7015 6533 -482
+ Misses 2019 1999 -20
+ Partials 473 447 -26
Continue to review full report at Codecov.
|
android/ops/CMakeLists.txt
Outdated
../../torchvision/csrc/cpu/*.h | ||
../../torchvision/csrc/cpu/*.cpp | ||
../../torchvision/csrc/*.h | ||
../../torchvision/csrc/*.cpp) |
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.
FYI there are currently discussions for changing the structure of the library. If we do that, we will need to change this part.
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.
@IvanKobzarev We changed the structure. Have a look on the latest master.
0e551fe
to
f1ede53
Compare
Don't need to have two variants of the model anymore, but I'm not removing it for now
1e2e5e5
to
de37a9c
Compare
d7897c1
to
eb5b7e8
Compare
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 have one question, otherwise looks great to me, thanks a lot @IvanKobzarev !
android/test_app/app/build.gradle
Outdated
arguments "-DANDROID_STL=c++_shared" | ||
} | ||
} | ||
buildConfigField("String", "MODULE_ASSET_NAME", "\"frcnn.pt\"") |
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 we need to change this file to match frcnn_mnetv3.pt
?
eb5b7e8
to
37c1aa6
Compare
@IvanKobzarev @fmassa awesome work, great to see this merged! |
} | ||
|
||
final long moduleForwardDuration = SystemClock.elapsedRealtime() - moduleForwardStartTime; | ||
final long analysisDuration = SystemClock.elapsedRealtime() - startTime; |
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.
@IvanKobzarev @fmassa Why do we use two duration vars initialized and estimated on exactly the same point? This seems unnecessary. Perhaps the intention was for moduleForwardDuration
to estimate the duration of the mModule.forward()
call? Or perhaps the intention of analysisDuration
is to measure just the unpacking of the response?
Summary: * [android] android gradle project for ops * Change CMakeLists to latest PyTorch * Use mobilenet_v3 models for detection Don't need to have two variants of the model anymore, but I'm not removing it for now * Fix orientation when angle = 0 * [android][test_app] Fix YUV decoding * Use smaller version of mobilenet model * Divide inputs by 255 again * [android] assets mobilenetv3 Reviewed By: fmassa Differential Revision: D26341416 fbshipit-source-id: 4bc79ce6fbca4ae744ccba128c7d81ec12827cdd Co-authored-by: Francisco Massa <fvsmassa@gmail.com>
PR introduces android gradle target that contains prebuilt torchvision ops library for android with android test application example how to use it.
The user workflow:
Content layout:
android/ops
android torchvision ops library setup, that uses nightly pytorch android dependencies with prebuilit libraries to link,
android/ops/CMakeLists.txt for linking libtorchvision.so
android/ops/CMakeLists.txt adds directive -DMOBILE which is used to strip python dependencies and backward ops that can be stripped from compilation for android.
Compilation includes only sources of ops, without image, video processing
CMakeLists.txt setup repeats the setup for android native apps that use prebuilt pytorch android binaries: https://pytorch.org/tutorials/recipes/android_native_app_with_custom_op.html
android/gradle_scripts
gradle scripts that repeat main pytorch setup in https://github.com/pytorch/pytorch/tree/master/android/gradle
android/test_app
example of android application that uses the result library of android/ops, contains several flavors to build application with dependency on local android/ops folder or with maven artifact 'org.pytorch:torchvision_ops:0.0.1-SNAPSHOT'
Testing
Generate faster rcnn pretrained model for test application using
android/test_app/make_assets.py
that will produce pt file inandroid/app/src/main/assets/frcnn.pt
that will be packaged inside applicationConnect android device that adb devices shows it
Install application that feeds camera output and draws the result bounding boxes on the screen;
cd $VISION_ROOT
gradle -p android test_app:installFrcnnCameraDebug
Publishing
edit
android/gradle.properties
to changeVERSION_NAME=0.0.1-SNAPSHOT
uploads snapshot 'org.pytorch:torchvision_ops:0.0.1-SNAPSHOT' to sonatype repository, that can be added to the app:
Will upload maven artifact to bintray that will be synced with jcenter (adding new artifact may need additional registration on bintray) that will be used in gradle as:
At the moment snapshot artifact 'org.pytorch:torchvision_ops:0.0.1-SNAPSHOT' is already published on sonatype repo: https://oss.sonatype.org/#nexus-search;quick~org.pytorch
https://oss.sonatype.org/service/local/repositories/snapshots/content/org/pytorch/torchvision_ops/