Add TensorFlow examples - ResNet50 and BERT models#19
Add TensorFlow examples - ResNet50 and BERT models#19Satya1493 wants to merge 2 commits intogramineproject:masterfrom
Conversation
Signed-off-by: Satyanaraya Illa <satyanaraya.illa@intel.com>
dimakuv
left a comment
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @Satya1493)
a discussion (no related file):
I verified that this PR is the same as the previous #7, bar the changes to fs.mounts and SGX_SIGNER_KEY plus Michal's comments.
Generally looks good to me. Only a couple comments to address.
tensorflow/BERT/Makefile, line 45 at r1 (raw file):
gramine-sgx-sign \ --manifest $< \ --output $@
We still need to use SGX_SIGNER_KEY in this PR. See our discussion in #20. We'll remove all those SGX_SIGNER_KEY mentions when we have Gramine v1.2 released.
So please keep the previous version of this code snippet:
python.manifest.sgx: python.manifest
@test -s $(SGX_SIGNER_KEY) || \
{ echo "SGX signer private key was not found, please specify SGX_SIGNER_KEY!"; exit 1; }
gramine-sgx-sign \
--key $(SGX_SIGNER_KEY) \
--manifest $< --output $@
tensorflow/BERT/python.manifest.template, line 21 at r1 (raw file):
{ path = "{{ python.distlib }}", uri = "file:{{ python.distlib }}" }, { path = "{{ pythondistpath }}", uri = "file:{{ pythondistpath }}" }, ]
We still need to use the old syntax in this PR. See our discussion in #20. We'll change to the new syntax when we have Gramine v1.2 released.
So please use the old syntax from #7. (But don't forget that you changed /tmp to the "tmpfs" type.)
Also, looks like you removed /etc directory -- which is good. Looks like it was redundant.
tensorflow/ResNet50/Makefile, line 34 at r1 (raw file):
gramine-sgx-sign \ --manifest $< \ --output $@
ditto (keep the old code with SGX_SIGNER_KEY)
tensorflow/ResNet50/python.manifest.template, line 23 at r1 (raw file):
{ path = "{{ python.distlib }}", uri = "file:{{ python.distlib }}" }, { path = "{{ pythondistpath }}", uri = "file:{{ pythondistpath }}" }, ]
ditto
Signed-off-by: Satyanaraya Illa <satyanaraya.illa@intel.com>
Satya1493
left a comment
There was a problem hiding this comment.
Reviewable status: 3 of 7 files reviewed, 4 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)
tensorflow/BERT/Makefile, line 45 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We still need to use
SGX_SIGNER_KEYin this PR. See our discussion in #20. We'll remove all thoseSGX_SIGNER_KEYmentions when we have Gramine v1.2 released.So please keep the previous version of this code snippet:
python.manifest.sgx: python.manifest @test -s $(SGX_SIGNER_KEY) || \ { echo "SGX signer private key was not found, please specify SGX_SIGNER_KEY!"; exit 1; } gramine-sgx-sign \ --key $(SGX_SIGNER_KEY) \ --manifest $< --output $@
Done.
tensorflow/BERT/python.manifest.template, line 21 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We still need to use the old syntax in this PR. See our discussion in #20. We'll change to the new syntax when we have Gramine v1.2 released.
So please use the old syntax from #7. (But don't forget that you changed
/tmpto the "tmpfs" type.)Also, looks like you removed
/etcdirectory -- which is good. Looks like it was redundant.
Done.
tensorflow/ResNet50/Makefile, line 34 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto (keep the old code with
SGX_SIGNER_KEY)
Done.
tensorflow/ResNet50/python.manifest.template, line 23 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done.
dimakuv
left a comment
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2, 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
a discussion (no related file):
@aneessahib Could someone from your team (who didn't work on this TensorFlow PR) check that the PR works correctly (TF builds and runs with the latest release of Gramine -- v1.1)?
|
Got it tested. The PR works fine. |
|
Thanks @aneessahib . This PR is pending a review from @mkow (or someone else from ITL). |
dimakuv
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, 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)
a discussion (no related file):
Add the explicit BSD-3 license, see #90
Signed-off-by: Satyanaraya Illa satyanaraya.illa@intel.com
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