Skip to content
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

[local-benchmark tool] Enable populate modelUrl and input shapes via URL #6680

Merged
merged 7 commits into from
Jul 27, 2022

Conversation

Linchenn
Copy link
Collaborator

@Linchenn Linchenn commented Jul 26, 2022

This PR enables users add the following parameters via URL (would be helpful for custom models):

  1. Model URL, modelUrl. For example, modelUrl=https://storage.googleapis.com/tfjs-models/savedmodel/bodypix/mobilenet/float/075/model-stride16.json.
  2. Input shapes, ${InputName}Shape. For example, bodypix has an input named sub_2, then users could add sub_2Shape=1,1,1,3 in the URL to populate its shape.

After this PR, you could try:
https://tfjs-benchmarks.web.app/local-benchmark/index.html?benchmark=custom&backend=webgl&run=5&modelUrl=https://storage.googleapis.com/tfjs-models/savedmodel/bodypix/mobilenet/float/075/model-stride16.json&sub_2Shape=1,1,1,3&task=performance

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

if (urlState != null && urlState.has(shapeName)) {
shapeInput.setAttribute('value', urlState.get(shapeName));
state.inputs[inputIndex].shape =
shapeInput.value.split(',').map(x => +x);
Copy link
Member

Choose a reason for hiding this comment

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

Nit: Use the Number function.

Suggested change
shapeInput.value.split(',').map(x => +x);
shapeInput.value.split(',').map(Number);

Comment on lines +507 to +508
if (urlState.has('modelUrl'))
state.modelUrl = urlState.get('modelUrl');
Copy link
Member

Choose a reason for hiding this comment

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

This is probably okay for testing and benchmarking, but keep in mind it may fail if the model url has characters like ? or &. It would be better to encode the URL with encodeURIComponent, but the format makes it hard for users to create new URLs.

It looks like you're hooking into the UI element that allows setting the custom model URL. If we can make this bi-directional (i.e. if you set the url in the UI, it appears as a URL parameter), then we can use the correct encoding, and users can input new URLs in the UI.

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

thank you, do we have a section in the doc that explains the available keys for url configuration?

Reviewed 1 of 1 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @Linchenn and @mattsoulanille)


e2e/benchmarks/local-benchmark/index.html line 508 at r2 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

This is probably okay for testing and benchmarking, but keep in mind it may fail if the model url has characters like ? or &. It would be better to encode the URL with encodeURIComponent, but the format makes it hard for users to create new URLs.

It looks like you're hooking into the UI element that allows setting the custom model URL. If we can make this bi-directional (i.e. if you set the url in the UI, it appears as a URL parameter), then we can use the correct encoding, and users can input new URLs in the UI.

good point, we should encode the url string and other parameter values.

Copy link
Member

@mattsoulanille mattsoulanille 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: :shipit: complete! 1 of 1 approvals obtained (waiting on @Linchenn and @pyu10055)


e2e/benchmarks/local-benchmark/index.html line 508 at r2 (raw file):

Previously, pyu10055 (Ping Yu) wrote…

good point, we should encode the url string and other parameter values.

Good idea. In a different PR, we should make all the settings appear as URL parameters when you change them. We should also add new settings for any URL parameters that aren't currently listed in the UI. These changes should make it easier for people to share benchmark configs, and no one will have to look up what URL parameters are supported since all the config will be done in the UI.

We can probably make a generic function that adds a new setting and hooks it up to a url parameter. It looks like these have to be set all at once, so the URL update function will probably just read the current state and transform it to a string of URL params to push with window.history.replaceState().

I'm happy with merging this PR as-is if it makes it easier to make these changes.

Copy link
Collaborator Author

@Linchenn Linchenn left a comment

Choose a reason for hiding this comment

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

Yes, we have a section here, https://github.com/tensorflow/tfjs/tree/master/e2e/benchmarks/local-benchmark#benchmark-test. Updated. Thanks!

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @mattsoulanille and @pyu10055)


e2e/benchmarks/local-benchmark/index.html line 451 at r2 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

Nit: Use the Number function.

                  shapeInput.value.split(',').map(Number);

Done.


e2e/benchmarks/local-benchmark/index.html line 508 at r2 (raw file):

Previously, mattsoulanille (Matthew Soulanille) wrote…

Good idea. In a different PR, we should make all the settings appear as URL parameters when you change them. We should also add new settings for any URL parameters that aren't currently listed in the UI. These changes should make it easier for people to share benchmark configs, and no one will have to look up what URL parameters are supported since all the config will be done in the UI.

We can probably make a generic function that adds a new setting and hooks it up to a url parameter. It looks like these have to be set all at once, so the URL update function will probably just read the current state and transform it to a string of URL params to push with window.history.replaceState().

I'm happy with merging this PR as-is if it makes it easier to make these changes.

Thank you for thinking about it with details!

From my perspective, for most model urls, they are supposed to be a .json file (also ends up with .json) and they do not have ? or &. For other parameters, they also typically do not have special characters. Also, without encoding url and other parameters, the url itself is more informational (users could see the model url or other parameter values directly from the url).

However, I think encoding/decoding still have non-trivial numbers of use cases and generating URL functions would be vary useful. I opened an ISSUE #6682 to add this feature. Great catch!

Copy link
Collaborator

@pyu10055 pyu10055 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! 2 of 1 approvals obtained (waiting on @mattsoulanille)

@Linchenn Linchenn merged commit 18c8b9d into tensorflow:master Jul 27, 2022
@Linchenn Linchenn deleted the bmTool branch July 27, 2022 17:27
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