Add TensorFlow examples - ResNet50 and BERT models #7
Conversation
Signed-off-by: Sonali Saha <[email protected]>
Signed-off-by: Satyanaraya Illa <[email protected]>
Satya1493
force-pushed
the
Satya1493/tensorflow
branch
from
619192a to
c05a2de
Compare
There was a problem hiding this comment.
Reviewed 4 of 7 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @mkow and @Satya1493)
tensorflow/README.md, line 54 at r2 (raw file):
- To run int8 inference on ``gramine-direct`` (non-SGX version), replace ``gramine-sgx`` with ``gramine-direct`` in the above command. - To run int8 inference on native baremetal (outside Gramine), replace ``gramine-sgx ./python`` with
on native baremetal (outside Gramine) -> natively (outside Gramine)
tensorflow/README.md, line 71 at r2 (raw file):
- To run inference on ``gramine-direct`` (non-SGX version), replace ``gramine-sgx`` with ``gramine-direct`` in the above command. - To run inference on native baremetal (outside Gramine), replace ``gramine-sgx ./python`` with
on native baremetal (outside Gramine) -> natively (outside Gramine)
tensorflow/README.md, line 89 at r2 (raw file):
- To get the number of cores per socket, do ``lscpu | grep 'Core(s) per socket'``. ## Performance considerations
Could you copy-paste this section from https://github.com/gramineproject/examples/pull/6/files? I already reviewed the OpenVINO PR, and all comments pertaining to this Performance considerations section were already resolved in that OpenVINO PR.
It looks to me that these sections are 95% identical, except for libos.check_invalid_pointers manifest option. Don't you want to add the description of this option in your README?
tensorflow/BERT/Makefile, line 1 at r2 (raw file):
# BERT sample for Tensorflow
Tensorflow -> TensorFlow
tensorflow/BERT/python.manifest.template, line 2 at r2 (raw file):
libos.entrypoint = "{{ entrypoint }}" loader.preload = "file:{{ gramine.libos }}"
Latest Gramine uses loader.entrypoint instead of loader.preload. Please see this commit: ebc051b
tensorflow/BERT/python.manifest.template, line 8 at r2 (raw file):
loader.insecure__use_cmdline_argv = true loader.insecure__use_host_env = true loader.insecure__disable_aslr = true
OpenVINO PR (#6) also has libos.check_invalid_pointers = false. Do you want to add this option here, for performance? Or does it break TensorFlow?
tensorflow/BERT/python.manifest.template, line 55 at r2 (raw file):
"file:{{ arch_libdir }}/", "file:/usr/{{ arch_libdir }}/", "file:{{ entrypoint }}/",
You have a / at the end here, this can't be right -- the {{ entrypoint }} is a file, not a directory. Please remove the trailing /.
tensorflow/ResNet50/Makefile, line 1 at r2 (raw file):
# ResNet50 sample for Tensorflow
Tensorflow -> TensorFlow
tensorflow/ResNet50/python.manifest.template, line 2 at r2 (raw file):
libos.entrypoint = "{{ entrypoint }}" loader.preload = "file:{{ gramine.libos }}"
Latest Gramine uses loader.entrypoint instead of loader.preload. Please see this commit: ebc051b
tensorflow/ResNet50/python.manifest.template, line 8 at r2 (raw file):
loader.insecure__use_cmdline_argv = true loader.insecure__use_host_env = true loader.insecure__disable_aslr = true
OpenVINO PR (#6) also has libos.check_invalid_pointers = false. Do you want to add this option here, for performance? Or does it break TensorFlow?
tensorflow/ResNet50/python.manifest.template, line 60 at r2 (raw file):
"file:/usr/{{ arch_libdir }}/", "file:resnet50v1_5_int8_pretrained_model.pb", "file:{{ entrypoint }}/",
You have a / at the end here, this can't be right -- the {{ entrypoint }} is a file, not a directory. Please remove the trailing /.
tensorflow/ResNet50/python.manifest.template, line 69 at r2 (raw file):
"file:/tmp/", "file:/etc/", "file:/proc/",
This is redundant, you're not mounting /proc/ FS in this manifest file. Just remove this line.
Signed-off-by: Satyanaraya Illa <[email protected]>
There was a problem hiding this comment.
Reviewable status: 2 of 7 files reviewed, 12 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
tensorflow/README.md, line 54 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
on native baremetal (outside Gramine)->natively (outside Gramine)
Done.
tensorflow/README.md, line 71 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
on native baremetal (outside Gramine)->natively (outside Gramine)
Done.
tensorflow/README.md, line 89 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you copy-paste this section from https://github.com/gramineproject/examples/pull/6/files? I already reviewed the OpenVINO PR, and all comments pertaining to this
Performance considerationssection were already resolved in that OpenVINO PR.It looks to me that these sections are 95% identical, except for
libos.check_invalid_pointersmanifest option. Don't you want to add the description of this option in your README?
Done.
tensorflow/BERT/Makefile, line 1 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Tensorflow->TensorFlow
Done.
tensorflow/BERT/python.manifest.template, line 2 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Latest Gramine uses
loader.entrypointinstead ofloader.preload. Please see this commit: ebc051b
Done.
tensorflow/BERT/python.manifest.template, line 8 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
OpenVINO PR (#6) also has
libos.check_invalid_pointers = false. Do you want to add this option here, for performance? Or does it break TensorFlow?
It breaks TensorFlow, hence didn't add.
tensorflow/BERT/python.manifest.template, line 55 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You have a
/at the end here, this can't be right -- the{{ entrypoint }}is a file, not a directory. Please remove the trailing/.
Done.
tensorflow/ResNet50/Makefile, line 1 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Tensorflow->TensorFlow
Done.
tensorflow/ResNet50/python.manifest.template, line 2 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Latest Gramine uses
loader.entrypointinstead ofloader.preload. Please see this commit: ebc051b
Done.
tensorflow/ResNet50/python.manifest.template, line 8 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
OpenVINO PR (#6) also has
libos.check_invalid_pointers = false. Do you want to add this option here, for performance? Or does it break TensorFlow?
It breaks TF, hence didn't add.
tensorflow/ResNet50/python.manifest.template, line 60 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You have a
/at the end here, this can't be right -- the{{ entrypoint }}is a file, not a directory. Please remove the trailing/.
Done.
tensorflow/ResNet50/python.manifest.template, line 69 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is redundant, you're not mounting
/proc/FS in this manifest file. Just remove this line.
Done.
Satya1493
force-pushed
the
Satya1493/tensorflow
branch
from
57c39c8 to
6b68ed8
Compare
There was a problem hiding this comment.
Reviewed 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @mkow)
There was a problem hiding this comment.
Reviewed 2 of 7 files at r1, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 17 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @Satya1493)
a discussion (no related file):
Who's the author of this PR actually? It was submitted by @Satya1493, but the first commit says that it's authored by @sahason.
tensorflow/README.md, line 1 at r3 (raw file):
## Inference on TensorFlow BERT and ResNet50 models
Please add an empty line after the headings (you seem to do this only for some of the headings).
tensorflow/README.md, line 6 at r3 (raw file):
inference. ### Bidirectional Encoder Representations from Transformers (BERT):
We usually don't put colons after headings, please remove.
tensorflow/README.md, line 7 at r3 (raw file):
### Bidirectional Encoder Representations from Transformers (BERT): BERT is a method of pre-training language representations and then use that trained model for
use -> using (or maybe you meant something different?)
tensorflow/README.md, line 20 at r3 (raw file):
## Pre-requisites - Upgrade pip/pip3. - Install TensorFlow using ``pip install intel-tensorflow-avx512==2.4.0``.
Please use single backticks instead of double.
tensorflow/README.md, line 22 at r3 (raw file):
- Install TensorFlow using ``pip install intel-tensorflow-avx512==2.4.0``. ## Build BERT or ResNet50 samples
samples -> sample
tensorflow/README.md, line 29 at r3 (raw file):
- To build the SGX version, do ``make PYTHONDISTPATH=path_to_python_dist_packages/ SGX=1`` - Typically, ``path_to_python_dist_packages`` is ``/usr/local/lib/python3.6/dist-packages``, but can change based on python's installation directory.
These instructions were pretty unintuitive to me. Is this a list of steps to follow? Or a list of commands to pick from when you want to do a specific thing? Some entries suggest the former ("To build BERT sample, do cd BERT. To build ResNet50 sample, do cd ResNet50." - this point alone is just "cd", no real commands?) and some suggest the latter ("To clean the sample, do make clean").
tensorflow/README.md, line 29 at r3 (raw file):
- To build the SGX version, do ``make PYTHONDISTPATH=path_to_python_dist_packages/ SGX=1`` - Typically, ``path_to_python_dist_packages`` is ``/usr/local/lib/python3.6/dist-packages``, but can change based on python's installation directory.
Some sentences here end with a period and some doesn't, please fix
tensorflow/README.md, line 75 at r3 (raw file):
## Notes on optimal performance Above commands are for a 36 core system. Please set the following options accordingly for optimal
36 core -> 36-core
tensorflow/README.md, line 112 at r3 (raw file):
TCMalloc and mimalloc are memory allocator libraries from Google and Microsoft that can help improve performance significantly based on the workloads. Only one of these allocators can be used.
can be used -> can be used at the same time
tensorflow/BERT/Makefile, line 23 at r3 (raw file):
BERT_FP32_MODEL = https://storage.googleapis.com/intel-optimized-tensorflow/models/v2_4_0/fp32_bert_squad.pb collateral:
This should be PHONY
tensorflow/BERT/python.manifest.template, line 62 at r3 (raw file):
] sgx.allowed_files = [
If we want to allow these then we should clearly write in the readme (and maybe also here?) that this is completely insecure and what the user should do to secure this.
Same for other insecure switches.
tensorflow/BERT/python.manifest.template, line 63 at r3 (raw file):
sgx.allowed_files = [ "file:/tmp/",
why not using Gramine's tmpfs?
tensorflow/ResNet50/Makefile, line 12 at r3 (raw file):
endif .PHONY: all collateral
Please move .PHONY: collateral to the line right before collateral:.
tensorflow/ResNet50/Makefile, line 36 at r3 (raw file):
--key $(SGX_SIGNER_KEY) \ --manifest python.manifest \ --output $@
please unify with the BERT example
tensorflow/ResNet50/python.manifest.template, line 67 at r3 (raw file):
] sgx.allowed_files = [
ditto
tensorflow/ResNet50/python.manifest.template, line 68 at r3 (raw file):
sgx.allowed_files = [ "file:/tmp/",
ditto (tmpfs)
|
@Satya1493 Will you continue working on this PR? |
|
Changes for TensorFlow are present at #19. It incorporates all comments from this PR. |
Signed-off-by: Satyanaraya Illa [email protected]
Description of the changes
TensorFlow examples for ResNet50 and BERT models. The samples run inference using pre-trained models.
How to test this PR?
Please follow steps present at tensorflow/README.md
This change is