Fixes path_prefix flag #1623

stephanwlee · 2018-11-22T02:51:42Z

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.

wchargin · 2018-11-22T03:32:59Z

tensorboard/components/tf_backend/router.ts

@@ -62,7 +62,13 @@ export function createRouter(pathPrefix = 'data', demoMode = false): Router {
  };
 };

-let _router: Router = createRouter();
+export function getDefaultRouter(): Router {
+  const sep = window.location.pathname.endsWith('/') ? '' : '/';


It’s not great that this introduces a side-effect at module load time.
We now need to reason about the first time that this module is imported;
it becomes harder to reason about in isolation.

Before #1512, we issued requests to relative paths. Can we just go back
to doing that instead of switching to absolute paths and then trying to
relativize them? We wouldn’t need to worry about window.location at
all, right?

I contemplated and decided to keep the logic that uses new URL for its sanitization/normalization but unfortunately forces us to reason with absolute path. Not sure if I understand what you mean by side-effect at the bootstrap time.

Noted. I haven’t carefully looked at the ways in which we use URL;
I’ll do so after the holiday.

wchargin · 2018-11-22T03:33:25Z

tensorboard/components/tf_backend/router.ts

 * @param demoMode {boolean=} Whether to modify urls for filesystem demo usage.
 */
-export function createRouter(pathPrefix = 'data', demoMode = false): Router {
+export function createRouter(pathPrefix, demoMode = false): Router {


Why is the default prefix no longer "data"? This is a breaking
change—does it just not happen to break any clients?

I vetted all the callsites manually

I vetted all the callsites manually

I mean, this is not possible in principle, right? createRouter is a
publicly exported function defined in a module whose Bazel target has
visibility //visibility:public in a workspace that we explicitly
encourage people to import, both inside Google and beyond. By all
accounts it is a public API. Are we in the habit of breaking our public
contracts?

I also still don’t understand why you want to make this change.

wchargin · 2018-11-22T03:35:28Z

tensorboard/components/tf_backend/test/backendTests.ts

@@ -81,6 +81,23 @@ describe('backend tests', () => {
      assert.equal(r.runs(), '/foo/runs');
    });

+    describe('default router', () => {
+      beforeEach(function() {
+        this.router = getDefaultRouter();


Why assign to this.router and introduce this state instead of just
using

assert.equal( getDefaultRouter().runs(), `${window.location.pathname}/data/runs` );

in the one test case that actually uses this?

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 · 2018-11-27T16:59:43Z

Argh, I just read the proper API doc for URL and found out the constructor takes an optional second parameter letting us achieve what we wanted. I have revised the code. PTAL

wchargin

This is definitely better: thanks. It’s unfortunate that we depend on
window.location at all, but I see that existing code does this, too,
so at least this isn’t a regression.

wchargin · 2018-11-27T17:47:46Z

tensorboard/components/tf_backend/test/backendTests.ts

        });

        it('ignores custom extension', function() {
-          assert.equal(
+          assertRelativePath(
              this.router.pluginRoute('scalars', '/a', undefined, 'meow'),


Why does this typecheck? The Router interface declares pluginRoute
as a function taking two parameters, not four. Do we have implicit any
propagation somehow?

Thanks for catching the error!

Apparently this is legal 🤷‍♂️ : http://www.typescriptlang.org/play/#src=export%20interface%20Router%20%7B%0D%0A%20%20%20%20foo%3A%20(bar%3A%20boolean%2C%20baz%3A%20string)%20%3D%3E%20string%3B%0D%0A%7D%3B%0D%0A%0D%0Aexport%20function%20createRouter()%3A%20Router%20%7B%0D%0A%20%20%20%20return%20%7B%0D%0A%20%20%20%20%20%20%20%20foo%3A%20(bar%3A%20boolean%2C%20baz%3A%20string%2C%20random%3F%3A%20string)%3A%20string%20%3D%3E%20%7B%0D%0A%20%20%20%20%20%20%20%20%20%20%20%20return%20'hello'%3B%0D%0A%20%20%20%20%20%20%20%20%7D%2C%0D%0A%20%20%20%20%7D%3B%0D%0A%7D

Fixed it.

No, that’s not what I’m concerned about. That’s perfectly fine.

At the callsite in this comment, you are invoking a function with more
arguments than the number of declared parameters. TypeScript disallows
this (which it must—this is the only sound thing to do). But this
type error was not caught. Why?

From a brief experiment, it looks like attaching router onto Mocha's scope makes us lose all the typing info. Will refactor to avoid that.

Yep, that’s what I suspected. Thanks: please do refactor thus.

nfelt · 2018-11-27T19:58:44Z

tensorboard/components/tf_backend/router.ts

  // Use URL to normalize pathPrefix with leading slash and without.
-  url.pathname = pathPrefix + path;
+  const maybeRelativePath = pathPrefix + path;
+  const url = new URL(maybeRelativePath, window.location.href);


Do we need to use the actual window.location here? Could we just pass "http://localhost" or even "http://ignored" instead? That would make it clearer that we're only dealing with relative URL paths.

The point of using window.location.href is that if you’ve navigated to
http://localhost:6006/some-prefix/ then that prefix will be included
in the eventual URL. Doesn’t this break if you use "http://localhost"
instead of window.location.href? Am I missing something?

But maybeRelativePath already includes the configured pathPrefix. Unless the idea here was to make TensorBoard automatically detect the pathPrefix from the href, I don't think it's necessary to get it from window.location.href.

And actually, I don't think any path information is retained from the second parameter to URL at all, from testing it just now - it only extracts the origin anyway:

> new URL('prefix/foo', 'http://xyz') URL {href: "http://xyz/prefix/foo", origin: "http://xyz", protocol: "http:", username: "", password: "", …} > new URL('prefix/foo', 'http://xyz/') URL {href: "http://xyz/prefix/foo", origin: "http://xyz", protocol: "http:", username: "", password: "", …} > new URL('prefix/foo', 'http://xyz/foo') URL {href: "http://xyz/prefix/foo", origin: "http://xyz", protocol: "http:", username: "", password: "", …} > new URL('/prefix/foo', 'http://xyz/foo') URL {href: "http://xyz/prefix/foo", origin: "http://xyz", protocol: "http:", username: "", password: "", …}

Note

> new URL('prefix/foo', 'http://xyz/foo/bar') < URL {href: "http://xyz/foo/prefix/foo", origin: "http://xyz", protocol: "http:", username: "", password: "", …} > new URL('prefix/foo', 'http://xyz/foo/') < URL {href: "http://xyz/foo/prefix/foo", origin: "http://xyz", protocol: "http:", username: "", password: "", …}

Your tests are correct but nonexhaustive. If you’re on http://xyz/foo
and navigate to prefix/bar, you will visit http://xyz/prefix/bar.
You need to include a trailing slash to force an additional path
component:

> String(new URL("y/z", "http://foo/")) "http://foo/y/z" > String(new URL("y/z", "http://foo/bar/")) "http://foo/bar/y/z" > String(new URL("y/z", "http://foo/bar/baz/")) "http://foo/bar/baz/y/z"

But maybeRelativePath already includes the configured pathPrefix

Yes, but the pathPrefix is always "data", right? (Admittedly, this
naming is very misleading.)

Ugh, you're right. It also turns out the pathPrefix naming situation was exacerbated when I suggested renaming dataDir to pathPrefix because I had managed to conflate dataDir and the --path_prefix flag:
#1563 (comment)

So really we should probably change everything here back to dataDir instead of pathPrefix. The router used to entirely ignore the path prefix (because it only operated on relative paths). With this change, the use of window.location.href will bake it into the final pathname as an absolute URL path (starting with /), but it's still not explicitly referenced.

The router used to entirely ignore the path prefix (because it only
operated on relative paths). With this change, the use of
window.location.href will bake it into the final pathname as an
absolute URL path (starting with /), but it's still not explicitly
referenced.

Yes, exactly.

So really we should probably change everything here back to dataDir
instead of pathPrefix.

Fine with me. It’s worth noting that the value of this argument is in
fact always "data" in public code, except for tests of the router
itself.

wchargin

Code looks better; thanks!

Some questions since I’ve never actually seen these demos working:

What’s our procedure for testing the demo logic?
How do we generate the data for these demos, and how do we run them?
Can we run them with a --path_prefix, or are path prefixes and demo mode mutually exclusive?
Is any of this automated?

wchargin · 2018-11-28T17:44:25Z

tensorboard/components/tf_backend/router.ts

-  url.pathname = `${pathPrefix}/${normalizedPath}${ext}`;
-  return url.pathname + url.search;
+  url.pathname = `${dataDir}/${normalizedPath}${ext}`;
+  return url.pathname;


Can this be safely replaced with

return `${dataDir}/${normalizedPath}${ext}`;

or do we need the special getter-setter semantics of URL.pathname?
(For instance, the current code will always return a string that starts
with a slash, even if dataDir does not. Is this intentional?)

If the latter, could you please leave a comment explaining why? It’s
certainly not obvious at a glance.

For instance, the current code will always return a string that starts
with a slash, even if dataDir does not. Is this intentional?

Yup, this is intentional. demos data shims are served at /data/ regardless of --path_prefix so we want to form an absolute path.
About using getter-setter, I was implicitly using URL's ability to sanitize the path and form a legal URL. Because we had undone that part in prodURL, technically, there is no reason to do that for demo url. (moreover, because we do not render output of the router into source of an anchor tag or in dangerous way, I think the sanitization of URL we form is not strictly required so that's that)

If the latter, could you please leave a comment explaining why
Will do

wchargin · 2018-11-29T18:36:32Z

tensorboard/components/tf_backend/router.ts

-  if (params) url.search = params.toString();
-  return url.pathname + url.search;
+  let relativePath = dataDir + route;
+  if (params) {


Note (not a big deal): if I read this correctly, the behavior is
different when params is new URLSearchParams() vs. undefined: in
the former case you get URLs like /data/foo? and /data/foo?bar=1&,
while in the latter case you get /data/foo and /data/foo?bar=1. To
my knowledge, these should be handled identically by the backend, so I’m
fine with the code as is, even though it would IMHO be more sensible to
write as

function createProdDataPath( dataDir: string, route: string, ext: string, params: URLSearchParams = new URLSearchParams() ): string { let relativePath = dataDir + route; if (String(params)) { const delimiter = route.includes("?") ? "&" : "?"; relativePath += delimiter + String(params); } return relativePath; }

wchargin · 2018-11-29T18:37:45Z

Thanks! I haven’t read the demo-mode code/tests very carefully, as we’re
just going to remove those (#1640). Prod code looks good to me.

falaki · 2018-12-27T21:19:25Z

Hi @stephanwlee and @wchargin is this bug fix going to be included in a patch release of 1.12 (e.g. 1.12.2)?

stephanwlee · 2018-12-27T21:27:08Z

@falaki Sorry, this change did not make it into 1.12.1 and we do not have a plan to release another patch before 1.13. However, 1.13 should come out early next year.

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.

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 · 2019-01-03T02:14:01Z

@falaki We just released 1.12.2 patch with the fix. Thanks for being patient with the release.

zhangchen-qinyinghua mentioned this pull request Nov 22, 2018

Solve the --path_prefix problem #1620

Closed

wchargin reviewed Nov 22, 2018

View reviewed changes

stephanwlee added 2 commits November 26, 2018 19:37

Fixes path_prefix flag

e6ed295

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.

Use two arg constructor for URL for relative path

7fc49ba

stephanwlee force-pushed the fix3 branch from dc17b47 to 7fc49ba Compare November 27, 2018 04:14

wchargin reviewed Nov 27, 2018

View reviewed changes

fixed the router typing

8b3b3bf

nfelt reviewed Nov 27, 2018

View reviewed changes

stephanwlee added 4 commits November 27, 2018 13:36

Not using mocha scope for TypeScript type inference

7b9ea26

Change to arrow func to denote that we will not be using mocha scope

789f11b

Revert back to old behavior and added more tests

9f0a7b4

Remove unused search in demo path

1b4b88c

wchargin reviewed Nov 28, 2018

View reviewed changes

wchargin mentioned this pull request Nov 28, 2018

path_prefix no longer working in 1.12.0 #1632

Closed

Renamed methods and added comment for clarity

ec07366

wchargin approved these changes Nov 29, 2018

View reviewed changes

stephanwlee added 2 commits November 29, 2018 10:54

Handle the case when plugin route passes an empty search params

2009d0d

Minor code change

242b866

stephanwlee merged commit 71f68bf into tensorflow:master Nov 29, 2018

stephanwlee deleted the fix3 branch November 29, 2018 19:10

mengxr mentioned this pull request Dec 28, 2018

[WIP] [ES-9408] apply a patch to fix relative path issue in Tensorboard AnacondaRecipes/tensorflow_recipes#9

Closed

nfelt mentioned this pull request Feb 25, 2019

404 on some images and fonts behind proxy #1885

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes path_prefix flag #1623

Fixes path_prefix flag #1623

stephanwlee commented Nov 22, 2018

wchargin Nov 22, 2018

stephanwlee Nov 22, 2018

wchargin Nov 22, 2018

wchargin Nov 22, 2018

stephanwlee Nov 22, 2018

wchargin Nov 22, 2018

wchargin Nov 22, 2018

stephanwlee commented Nov 27, 2018

wchargin left a comment

wchargin Nov 27, 2018

stephanwlee Nov 27, 2018

wchargin Nov 27, 2018

stephanwlee Nov 27, 2018

wchargin Nov 27, 2018

nfelt Nov 27, 2018

wchargin Nov 27, 2018

nfelt Nov 27, 2018

stephanwlee Nov 27, 2018 •

edited

Loading

wchargin Nov 27, 2018

nfelt Nov 27, 2018

wchargin Nov 27, 2018

wchargin left a comment

wchargin Nov 28, 2018

stephanwlee Nov 28, 2018

wchargin Nov 29, 2018

wchargin commented Nov 29, 2018

falaki commented Dec 27, 2018 •

edited

Loading

stephanwlee commented Dec 27, 2018

stephanwlee commented Jan 3, 2019

Fixes path_prefix flag #1623

Fixes path_prefix flag #1623

Conversation

stephanwlee commented Nov 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanwlee commented Nov 27, 2018

wchargin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanwlee Nov 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchargin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wchargin commented Nov 29, 2018

falaki commented Dec 27, 2018 • edited Loading

stephanwlee commented Dec 27, 2018

stephanwlee commented Jan 3, 2019

stephanwlee Nov 27, 2018 •

edited

Loading

falaki commented Dec 27, 2018 •

edited

Loading