Skip to content

Conversation

@stephanwlee
Copy link
Contributor

Major changes:

  1. All demo fixtures have proper file extension defined
  2. Because of 1, router has changed significantly to embed the demo url generation logic with appropriate file extensions

Minor changes are:
a. audio_dashboard had missing </template> only causing demo to not work
b. graph_dashboard was missing an import statement for tf_storage

Copy link
Contributor

Choose a reason for hiding this comment

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

This appears to be unused?

Copy link
Contributor

Choose a reason for hiding this comment

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

Given that this is new functionality, it seems better to me to just require passing in an actual URLSearchParams object, rather than sticking with the custom QueryParams type. You can construct URLSearchParams directly from a map, so most callsites can be updated by just wrapping a new URLSearchParams() around the argument. I don't actually know that we have any callsites that use repeated/list-valued parameters, but if so, they can pass in a list of pairs instead or build up the instance via append().

This way the code in e.g. the audio plugin can be much simpler since there's no need to create URLSearchParams, convert it into QueryParams, and then reconvert back.

Copy link
Contributor

Choose a reason for hiding this comment

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

typo: s/correcrt/correct/ here and below

Copy link
Contributor

Choose a reason for hiding this comment

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

The extension is already ignored unless we're in demo mode, so simpler to just pass '.wav' all the time and let Router figure it out.

@stephanwlee
Copy link
Contributor Author

stephanwlee commented Oct 15, 2018

Ugh, it looks like, although you can pass object to the constructor of URLSearchParams and get the right instance, it is defined to take string or another instance of URLSearchParams (https://developer.mozilla.org/en-US/docs/Web/API/URLSearchParams/URLSearchParams#Parameters). Either that or someone is not updated correctly and TypeScript disallows passing a object :(

@stephanwlee stephanwlee merged commit 05b22e8 into tensorflow:master Oct 17, 2018
@stephanwlee stephanwlee deleted the test2 branch October 17, 2018 01:24
@wchargin
Copy link
Contributor

@stephanwlee: This appears to break the --path_prefix functionality?

Before this commit, running

bazel run //tensorboard -- --logdir ~/tensorboard_data/ --path_prefix /test/

and then navigating to http://localhost:6006/test/ yielded a fully
functioning TensorBoard instance (albeit with broken webfonts). After
this commit, it fails to load the set of active dashboards.

Network traffic indicates that the frontend is hitting /data/...
instead of /test/data/... as it did before this commit.

Git bisect says:

05b22e8 is the first bad commit

Could you please investigate?

@wchargin
Copy link
Contributor

From what I can tell, it looked like the result of the default router’s
pluginsListing() method changed in this commit:

  • old: data/plugins_listing;
  • new: /data/plugins_listing.

The --path_prefix argument never propagated explicitly into the
router; it’s just that we were previously fetching relative paths so
everything worked out, but as of this commit that is no longer the case.

stephanwlee added a commit to stephanwlee/tensorboard that referenced this pull request Nov 27, 2018
PR tensorflow#1512 introduced a bug where it formed a complete path without using
existing pathname. Previously without forming the complete path, it was
treated as a relative path from current window.location.pathname.
stephanwlee added a commit that referenced this pull request Nov 29, 2018
PR #1512 introduced a bug where it formed a complete path without using
existing pathname. Previously without forming the complete path, it was
treated as a relative path from current window.location.pathname. We decided
to revert back to the old behavior without using `new URL` which was not
providing a clear benefit.
stephanwlee added a commit to stephanwlee/tensorboard that referenced this pull request Jan 2, 2019
PR tensorflow#1512 introduced a bug where it formed a complete path without using
existing pathname. Previously without forming the complete path, it was
treated as a relative path from current window.location.pathname. We decided
to revert back to the old behavior without using `new URL` which was not
providing a clear benefit.
stephanwlee added a commit that referenced this pull request Jan 3, 2019
PR #1512 introduced a bug where it formed a complete path without using
existing pathname. Previously without forming the complete path, it was
treated as a relative path from current window.location.pathname. We decided
to revert back to the old behavior without using `new URL` which was not
providing a clear benefit.
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