Skip to content

Conversation

@Satya1493
Copy link

@Satya1493 Satya1493 commented Mar 22, 2022

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 Reviewable

Signed-off-by: Satyanaraya Illa <satyanaraya.illa@intel.com>
Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

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>
Copy link
Author

@Satya1493 Satya1493 left a comment

Choose a reason for hiding this comment

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

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_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 $@

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 /tmp to the "tmpfs" type.)

Also, looks like you removed /etc directory -- 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.

Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

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)?


@dimakuv dimakuv requested a review from mkow April 11, 2022 10:53
@aneessahib
Copy link

Got it tested. The PR works fine.

@dimakuv
Copy link

dimakuv commented Apr 14, 2022

Thanks @aneessahib . This PR is pending a review from @mkow (or someone else from ITL).

Copy link

@dimakuv dimakuv left a comment

Choose a reason for hiding this comment

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

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


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants