Add LightGBM example - #20
Conversation
dimakuv
left a comment
dimakuv
left a comment
There was a problem hiding this comment.
The name of this PR and the description have a typo: LightBGM -> LightGBM
Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @Gautham103)
a discussion (no related file):
-
Did you test it under SGX? I have doubts it works.
-
Please change the Manifest file, especially comments. Also, it looks like SGX version will require more tweaks to the manifest.
lightbgm/lightgbm.manifest.template, line 3 at r1 (raw file):
#!$(PALLOADER) # Memcached manifest file example
Memcached? Please fix mentions of memcached everywhere
lightbgm/lightgbm.manifest.template, line 18 at r1 (raw file):
# ID which must be specified as command-line argument. The assumed user ID is # irrelevant for consequent Memcached execution, so it may be any existing # host-OS username (e.g. "nobody" or even "root").
This whole comment is surely not needed for LightGBM
lightbgm/lightgbm.manifest.template, line 65 at r1 (raw file):
loader.env.MEMCACHED_SASL_PWDB = loader.env.T_MEMD_INITIAL_MALLOC = loader.env.MEMCACHED_SLAB_BULK_CHECK =
This is incorrect
lightbgm/lightgbm.manifest.template, line 107 at r1 (raw file):
# default port used by Memcached. Note how a missing interface name means "any # interface". net.allow_bind.memcachedport = :11211
This is incorrect
lightbgm/Makefile, line 62 at r1 (raw file):
-e 's|$$(PALLOADER)|'"$(abspath $(GRAPHENEDIR)/Runtime/pal_loader)"'|g' \ $< > $@ chmod 777 $@
This is weird. Why 777?
lightbgm/Makefile, line 90 at r1 (raw file):
# Also, create a link to pal_loader for simplicity. lightgbm: $(SRCDIR)/lightgbm
You have a trailing whitespace here
lightbgm/README.md, line 7 at r1 (raw file):
cd LibOS/shim/test/apps/lightbgm make clean make (In case of Graphene SGX, run make SGX=1 and make SGX_RUN=1)
Please use MD notation for embedded code snippets (triple-backticks)
mkow
left a comment
mkow
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 11 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @Gautham103)
a discussion (no related file):
Please fix the email address in your commit so that GitHub can recognize it (either by changing it in the commit or by adding it to your GitHub account). You can push-force if you choose the former.
lightbgm/Makefile, line 22 at r1 (raw file):
COMMIT = master ifeq ($(DEBUG),1)
Why leading space?
Same for many more places in this file. You have both unnecessary leading and trailing spaces added in random places.
lightbgm/README.md, line 1 at r1 (raw file):
Steps to run LightBGM on Graphene and Graphene SGX
Please refactor this whole file to proper Markdown.
Gautham103
left a comment
Gautham103
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @mkow)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Did you test it under SGX? I have doubts it works.
Please change the Manifest file, especially comments. Also, it looks like SGX version will require more tweaks to the manifest.
I have tested under SGX. It works fine without any tweaks to the manifest. I have changed the Manifest file also.
a discussion (no related file):
Previously, mkow (Michał Kowalczyk) wrote…
Please fix the email address in your commit so that GitHub can recognize it (either by changing it in the commit or by adding it to your GitHub account). You can push-force if you choose the former.
Done.
lightbgm/lightgbm.manifest.template, line 3 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Memcached? Please fix mentions ofmemcachedeverywhere
Done.
lightbgm/lightgbm.manifest.template, line 18 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This whole comment is surely not needed for LightGBM
Removed the comment
lightbgm/lightgbm.manifest.template, line 65 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is incorrect
Removed the comments
lightbgm/lightgbm.manifest.template, line 107 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is incorrect
Removed the comments
lightbgm/Makefile, line 22 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why leading space?
Same for many more places in this file. You have both unnecessary leading and trailing spaces added in random places.
It was a copy paste error. Removed unwanted spaces.
lightbgm/Makefile, line 62 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is weird. Why
777?
Now giving executable permission for the manifest file.
lightbgm/Makefile, line 90 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You have a trailing whitespace here
Removed white spaces
lightbgm/README.md, line 1 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Please refactor this whole file to proper Markdown.
Done.
lightbgm/README.md, line 7 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please use MD notation for embedded code snippets (triple-backticks)
Done.
chiache
left a comment
chiache
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv, @Gautham103, and @mkow)
lightbgm/lightgbm.manifest.template, line 3 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Memcached? Please fix mentions ofmemcachedeverywhere
+1. #2 is a good template to start with, but please update all the instructions and make sure all the claims are correct.
lightbgm/lightgbm.manifest.template, line 18 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This whole comment is surely not needed for LightGBM
+1.
lightbgm/lightgbm.manifest.template, line 65 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is incorrect
+1. This is Memcached-specific. Please update.
lightbgm/lightgbm.manifest.template, line 107 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is incorrect
+1. This is Memcached-specific. Please update.
lightbgm/lightgbm.manifest.template, line 172 at r1 (raw file):
# host OS instead of downloading the sources, building, and linking Memcached # against the built library (which is possible but would require more effort). sgx.trusted_files.libevent = file:/usr/lib/x86_64-linux-gnu/libevent-2.0.so.5
This is probably not necessary for LightGBM. Please check the dependency (remove this line and test if the application still work).
lightbgm/lightgbm.manifest.template, line 185 at r1 (raw file):
############################# SGX: ALLOWED FILES ###############################
The whole allowed files section might not be necessary for LightGBM (because it's not a neworked application). Please check if these files are actually accessed in the debug log).
lightbgm/Makefile, line 22 at r1 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why leading space?
Same for many more places in this file. You have both unnecessary leading and trailing spaces added in random places.
+1, Please clean up unnecessary whitespaces or indentions.
lightbgm/Makefile, line 62 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This is weird. Why
777?
I would suggest use chmod ugo+x $@.
lightbgm/Makefile, line 90 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You have a trailing whitespace here
ditto
Gautham103
left a comment
Gautham103
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 13 unresolved discussions, not enough approvals from maintainers (3 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @chiache, @dimakuv, and @mkow)
lightbgm/lightgbm.manifest.template, line 65 at r1 (raw file):
Previously, chiache (Chia-Che Tsai) wrote…
+1. This is Memcached-specific. Please update.
Done.
lightbgm/lightgbm.manifest.template, line 107 at r1 (raw file):
Previously, chiache (Chia-Che Tsai) wrote…
+1. This is Memcached-specific. Please update.
Done.
lightbgm/lightgbm.manifest.template, line 172 at r1 (raw file):
Previously, chiache (Chia-Che Tsai) wrote…
This is probably not necessary for LightGBM. Please check the dependency (remove this line and test if the application still work).
Done.
lightbgm/lightgbm.manifest.template, line 185 at r1 (raw file):
Previously, chiache (Chia-Che Tsai) wrote…
The whole allowed files section might not be necessary for LightGBM (because it's not a neworked application). Please check if these files are actually accessed in the debug log).
Done.
lightbgm/Makefile, line 22 at r1 (raw file):
Previously, chiache (Chia-Che Tsai) wrote…
+1, Please clean up unnecessary whitespaces or indentions.
Done.
lightbgm/Makefile, line 62 at r1 (raw file):
Previously, chiache (Chia-Che Tsai) wrote…
I would suggest use
chmod ugo+x $@.
Done.
lightbgm/Makefile, line 90 at r1 (raw file):
Previously, chiache (Chia-Che Tsai) wrote…
ditto
Done.
chiache
left a comment
chiache
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 3 files reviewed, 11 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: OSCAR Lab) (waiting on @dimakuv and @mkow)
dimakuv
left a comment
dimakuv
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 10 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: OSCAR Lab) (waiting on @chiache, @Gautham103, and @mkow)
lightbgm/lightgbm.manifest.template, line 122 at r1 (raw file):
# to 12 (each thread requires 128MB of memory): `memcached -t 12`. This is # an issue in Memcached source code, not related to Graphene. sgx.enclave_size = 1024M
Why did you delete this line? The default is good enough (the default is 256MB I believe)?
lightbgm/lightgbm.manifest.template, line 132 at r1 (raw file):
# thread which sits idle after spawning worker threads. Therefore, specifying # '16' allows to run a maximum of 13 Memcached threads (`--threads <= 13`). sgx.thread_num = 16
Why did you delete this line? The default is good enough (the default is 4 threads I believe)?
lightbgm/Makefile, line 90 at r1 (raw file):
Previously, Gautham103 wrote…
Done.
Not done.
lightbgm/Makefile, line 13 at r2 (raw file):
.PHONY=all
Should be .PHONY: all
lightbgm/Makefile, line 52 at r2 (raw file):
.PHONY=clean
.PHONY: clean
lightbgm/Makefile, line 56 at r2 (raw file):
$(RM) *.token *.sig *.manifest.sgx *.manifest pal_loader lightgbm .PHONY=distclean
.PHONY: distclean
lightbgm/README.md, line 7 at r2 (raw file):
cd <Graphene directory>/LibOS/shim/test/apps/lightbgm make clean make (In case of Graphene SGX, make SGX=1 and make SGX_RUN=1)
Change this line to e.g.: make # in case of SGX, type: make SGX=1
4 participants
This change is