-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[benchmark tools] Update mobilenet models #6729
Conversation
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.
Reviewable status: 0 of 1 approvals obtained (waiting on @lina128 and @qjia7)
e2e/benchmarks/local-benchmark/README.md
line 70 at r2 (raw file):
It's easy to set up a web server to host benchmarks and run against them via e2e/benchmarks/local-benchmark/index.html. You can manually specify the optional url parameters as needed. Here are the list of supported url parameters: * Model related parameters:
Just added alpha
and separated the parameters into two categories for easy understanding.
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.
LGTM with one change.
Nice work!
@@ -198,7 +199,7 @@ <h2>TensorFlow.js Model Benchmark</h2> | |||
numWarmups: warmupTimes, | |||
numRuns: runTimes, | |||
numProfiles: profileTimes, | |||
benchmark: 'mobilenet_v2', | |||
benchmark: 'mobilenet_v3', |
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.
Forget to change to MobileNetV3
?
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.
Done. Thank you!
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.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128, @Linchenn, and @qjia7)
e2e/benchmarks/local-benchmark/index.html
line 202 at r3 (raw file):
Previously, Linchenn wrote…
Done. Thank you!
does mobilenet_v2 have architecture and alpha configurations?
e2e/benchmarks/local-benchmark/README.md
line 78 at r1 (raw file):
<b>inputType</b>: same as inputTypes<br> <b>modelUrl</b>: same as modelUrl, for custom models only<br> <b>${InputeName}Shape</b>: the input shape array, separated by comma, for custom models only. For example, bodypix's [graph model](https://storage. googleapis.com/tfjs-models/savedmodel/bodypix/mobilenet/float/075/ model-stride16.json) has an input named sub_2, then users could add '`sub_2Shape=1,1,1,3`' in the URL to populate its shape.<br>
this url looks weird, please double check
e2e/benchmarks/local-benchmark/README.md
line 70 at r2 (raw file):
Previously, Linchenn wrote…
Just added
alpha
and separated the parameters into two categories for easy understanding.
architecture and alpha are related to certain type of models (mobilenet, maybe posenet?), might be good to point it out.
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.
Thank you!
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @qjia7)
e2e/benchmarks/local-benchmark/index.html
line 202 at r3 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
does mobilenet_v2 have architecture and alpha configurations?
Yes, I added it here https://github.com/tensorflow/tfjs/pull/6729/files#diff-4c36570d490a92c2848da5b19e986e30c6663f7d929d58f8fd342534e6e793b6R121.
e2e/benchmarks/local-benchmark/README.md
line 78 at r1 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
this url looks weird, please double check
This is updated and you may have to click show full diff
:
e2e/benchmarks/local-benchmark/README.md
line 70 at r2 (raw file):
Previously, pyu10055 (Ping Yu) wrote…
architecture and alpha are related to certain type of models (mobilenet, maybe posenet?), might be good to point it out.
Good catch! Done.
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.
As discussed with Ping, I moved alpha
model parameter into architecture
model parameter, because alpha
is a special parameter for mobilenet only and it could be considered a part of architecture
definition.
@qjia7 FYI.
Reviewable status: complete! 1 of 1 approvals obtained (waiting on @lina128, @pyu10055, and @qjia7)
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.
LGTM
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.
As discussed with Ping, I moved alpha model parameter into architecture model parameter, because alpha is a special parameter for mobilenet only and it could be considered a part of architecture definition.
Still LGTM. Thank you!
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.
Reviewed 3 of 3 files at r5, all commit messages.
Reviewable status: complete! 3 of 1 approvals obtained (waiting on @pyu10055 and @qjia7)
As discussed in the spreadsheet, we will supports alpha variable for mobilenet V2 and V3, so this PR:
075 and 100 alphas
andsmall and large architectures
for mobilenet v3.050, 075 and 100 alphas
for mobilenet v2.To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.
This change is