ng: support path_prefix at router level #4423

stephanwlee · 2020-12-04T06:03:35Z

Problem:
TensorBoard has a flag, path_prefix, which serves frontend and the data
endpoints at your specified, if the flag is passed, pathname. While
exact use case slips my mind, it is certainly used by dependents. Now,
the problem is that new Angular based TensorBoard has a custom router
for in-app navigation (we may add new routes for experiment list and
others) it is unaware of the path prefix.

Router works by matching set of predefined routes to current pathname
and finds a match. However, the configuration is unaware (rightfully) of
the path prefix and it results in zero match, which in turn cause
navigation to the default path, "/". This totally breaks the assumption
of the path prefix as all the request from the frontend will be sent to
"/data/..." as opposed to "[path_prefix]/data/..."

Remediation:
Previously, there was no reason to propagate the prefix information to
the frontend. Application only did hash changes and never required a
navigation. With this change, we now:

server side render meta tag that depicts the relative path to the root of the
application
bootstrap Angular app from the relative root path and uses that information
for matching routes and navigation

Tests:

manually tested with/without path_prefix
tested dynamic plugin with/without path_prefix (projector)

wchargin · 2020-12-04T06:47:37Z

A passing thought and question…

One technique that I’ve used in the past is to simply inspect
window.location.pathname to get the path prefix on the frontend when
the app starts, before any navigation. This is especially simple for
TensorBoard since we’re a single-page app, but it’s also easy to extend
to multi-page apps, even with client side routing. Then you don’t need
any special backend logic, and in fact this works perfectly with a
static server even when you don’t know the path prefix upfront.

It seems to me like that would be a bit simpler. Is there a reason that
we prefer introducing a new /data/config route?

Or, in other words—

Previously, there was no reason to propagate the prefix information to
the frontend. Application only did hash changes and never required a
navigation.

—why does this have to change?

stephanwlee · 2020-12-04T17:08:17Z

One technique that I’ve used in the past is to simply inspect
window.location.pathname to get the path prefix on the frontend when
the app starts, before any navigation

There is no guarantee that application will only have "/" route and it is legal for users to start the app at "/foo/bar/experiment/123/"

wchargin · 2020-12-04T22:14:44Z

There is no guarantee that application will only have "/" route and it is legal for users to start the app at "/foo/bar/experiment/123/"

If I launch with --path_prefix /foo/bar at this patch and navigate to
/foo/bar/experiment/123/, it redirects me to just /foo/bar/, and
makes initial requests to /foo/bar/experiment/123/data/runs but then
later makes requests to just /foo/bar/data/runs. Is that working as
intended? (Like, it doesn’t matter for the OSS data providers, but it
doesn’t seem right to me.)

Anwyay, what I’ve typically done for this is to include in each page’s
static HTML a relative path reference from that page to the root of the
application:

<!-- /index.html -->
<body initial-root=".">
  <h1>Home</h1>
</body>

<!-- /about/roadmap/index.html -->
<body initial-root="../..">
  <h1>Our roadmap</h1>
</body>

Then, when index.js starts, it grabs body.dataset.initialRoot and
records normalize(`${pathname}/${initialRoot}/`) as the path prefix,
which works independent of the pathname, can be hosted statically, and
doesn’t add another RPC to the critical path.

But I recognize that this is probably a bit harder for us since we don’t
really do proper server side rendering, so okay with me to proceed with
your approach.

psybuzz

This PR seems to break Time Series in standalone TB :/

The app 404s on a request to https://.../experiment/defaultExperimentId/data/plugin/timeseries/tags because metrics_data_source uses a url starting with '/'. I wonder, should we patch TBHttpClient to add the path_prefix whenever its methods are called with an absolute-looking url?

Re: stamping the initial HTML with path_prefix (or the relative path to it, as per wchargin)
I don't have a strong preference between that approach and the current PR. If we end up needing to switch to the stamped HTML approach for performance reasons later, most of this PR's frontend code doesn't have to change.

psybuzz · 2020-12-04T21:37:08Z

tensorboard/webapp/app_routing/effects/app_routing_effects.ts

@@ -44,6 +45,7 @@ import {Navigation, Route} from '../types';
 /** @typehack */ import * as _typeHackNgrxEffects from '@ngrx/effects/effects';
 /** @typehack */ import * as _typeHackModels from '@ngrx/store/src/models';
 /** @typehack */ import * as _typeHackStore from '@ngrx/store';
+import {getPathPrefixConfig} from '../../core/store';


Just ran a test sync and it looks like we'll need an explicit dep to get it to build.

Thanks for doing that. I was going to run the sync test after LGTM but you beat me to it!

psybuzz · 2020-12-04T22:12:46Z

tensorboard/http_api.md

+      "path_prefix": "/foo/bar"
+      "window_title": "Custom Name",
+    }
+
 ## `data/environment`

 Returns environment in which the TensorBoard app is running.


Now that we've copied the exp-agnostic fields from /data/environment into /data/config, the doc section for data/environment is stale.

In this PR, anything consuming window_title is either

getting it properly from the exp-agnostic endpoint /data/config

or is stuck on the legacy /data/environment, and needs to migrate so that we can make /data/environment exp-specific

When imagining the future TB that supports multiple experiments, I would assume the more appropriate place to get the data_location (and other experiment-specific info) is not /data/env or /data/config, but from one of the experiment-specific endpoints that already exist (/data/experiments or /data/experiment_runs or /data/runs). In this scenario, we don't need both /data/config AND /data/env, but only 1 of them to serve exp-agnostic prefix_path, window_title, so we may consider dropping one of them.

Firstly, do you agree that we'd eventually need [/data/config OR /data/env], but not both?

If so, it sounds like the progression after this PR would be to:

migrate consumers using /data/env for data_location to another endpoint

migrate consumers using /data/env for window_title to use /data/config

delete the /data/env endpoint

Secondly, perhaps it makes more sense to just pass path_prefix in the /data/environment in this PR, (app_routing_effects would block on getEnvironment instead of getConfig). That way, the plan would be simpler:

migrate consumers using /data/env for data_location to another endpoint

wdyt?

In the broad strokes, we are about aligned.

While /data/environment is not the most meaningful name, it is actually being used for experiment metadata in a worse way (not just data_location).

tensorboard/tensorboard/plugins/core/core_plugin.py

Lines 140 to 147 in 83ce2a2

if experiment_metadata is not None:

environment.update(

{

"experiment_name": experiment_metadata.experiment_name,

"experiment_description": experiment_metadata.experiment_description,

"creation_time": experiment_metadata.creation_time,

}

)

My ideal would be to rename /data/experiment/[:id]/metadata but "environment", in some sense, about the same thing and I would not advocate for the change unless others feel strongly about it.

I would really not want to roll all experiment metadata under experiment-specific endpoint that is meant to serve lists or runs (besides those endpoints don't exist in a meaningful way? /experiments return an empty array all the time)

One follow up work that is required is to remove window_title from /environment and its usage in the page title:

tensorboard/tensorboard/webapp/core/views/page_title_container.ts

Lines 39 to 43 in 83ce2a2

readonly title$ = this.store.pipe(

select(getEnvironment),

map((env) => env.window_title || 'TensorBoard'),

// (it's an empty string when the `--window_title` flag is not set)

distinctUntilChanged()

psybuzz · 2020-12-05T00:19:34Z

tensorboard/webapp/app_routing/effects/app_routing_effects.ts

+          filter((pathPrefix) => pathPrefix !== null),
+          take(1),
+          tap((pathPrefix) => {
+            this.pathPrefix = pathPrefix;


Would things be more maintainable if we didn't store pathPrefix on AppRoutingEffects, but made the Location's getPath, onPopState, replaceState methods aware of it? e.g.

this.location.setPathPrefix(pathPrefix);

I think Location's method surface is much smaller to check than making sure every usage of this.location.getPath() is covered in this file. In case some other file in app_routing needed to inject Location, it wouldn't need to do all this wrapping as well.

I thought about that but was a bit weary of putting business logic into what seems like a clear API, location!

According to our usages of the state, we need to be able to

read synchronously

discern was modified

Those one or two are separate APIs that really do not belong in the Location, imo.

tensorboard/webapp/core/store/core_reducers.ts

nfelt · 2020-12-05T00:56:23Z

I'm not crazy about baking the path_prefix string itself into the actual XHR responses of the app. Conceptually the functioning of the app should be independent of the path prefix it's "mounted" under - everything about the path_prefix should be able to be handled by normal WSGI middleware (as we started doing in #2733; this is also a task in #2573).

What William suggests above nicely avoids this dependency on the prefix itself:

Anwyay, what I’ve typically done for this is to include in each page’s
static HTML a relative path reference from that page to the root of the
application

We can use this aspect of the suggestion even if we don't do server-side HTML modification, since we can just change data/environment to return something that indicates the relative path of that particular endpoint to the root (I think it would suffice to either return the literal subpage path like experiment/1234/, an expression representing the path to the root like ../.., or even just an integer for the number of path segments we are below the root).

If we do that, we actually do preserve the property that data/environment is still page-specific, and we avoid introducing an extra XHR in the critical path.

stephanwlee · 2020-12-05T01:13:09Z

If I launch with --path_prefix /foo/bar at this patch and navigate to
/foo/bar/experiment/123/, it redirects me to just /foo/bar/, and
makes initial requests to /foo/bar/experiment/123/data/runs but then
later makes requests to just /foo/bar/data/runs. Is that working as
intended? (Like, it doesn’t matter for the OSS data providers, but it
doesn’t seem right to me.)

No, that is not what actually happens. If you navigate to /foo/bar/experiment/123, it should remain there (no redirection to /foo/bar/if your app is truly aware of /experiment/123/) and run fetch data from/foo/bar/experiment/123/data/*`. Are you referring to an actual behavior? If so, it is not intended and I would appreciate if you can kindly point me to the env that you've used to reproduce this.

Then, when index.js starts, it grabs body.dataset.initialRoot and
records normalize(${pathname}/${initialRoot}/) as the path prefix,
which works independent of the pathname, can be hosted statically, and
doesn’t add another RPC to the critical path.

sounds like what I had recommended in the "dehydrate" section in original comment (#4421 (comment)).
I avoided it for the reason you've mentioned.

What William suggests above nicely avoids this dependency on the prefix itself:

Anwyay, what I’ve typically done for this is to include in each page’s
static HTML a relative path reference from that page to the root of the
application

We can use this aspect of the suggestion even if we don't do server-side HTML modification, since we can just change data/environment to return something that indicates the relative path of that particular endpoint to the root (I think it would suffice to either return the literal subpage path like experiment/1234/, an expression representing the path to the root like ../.., or even just an integer for the number of path segments we are below the root).

Sorry, I don't understand the approach at all. /data/environment returning some information about path_prefix (directly or indirectly) sound about the same and they are on the critical path of application bootstrap no matter what. If you are protesting against the name "path_prefix", I can make some changes but I think it is the best option we've got since (1) we want to surface the app config information anyways and (2) semantically meaningful and straightforward.

Before I address any comments, I want to hear back from passbyers.

wchargin · 2020-12-05T01:17:46Z

No, that is not what actually happens. If you navigate to /foo/bar/experiment/123, it should remain there (no redirection to /foo/bar/if your app is truly aware of /experiment/123/) and run fetch data from/foo/bar/experiment/123/data/*`. Are you referring to an actual behavior? If so, it is not intended and I would appreciate if you can kindly point me to the env that you've used to reproduce this.

Yes, this is what I actually observed at d0a9f847c, running:

bazel run //tensorboard -- --logdir ~/tensorboard_data/mnist/ --path_prefix /foo/bar --bind_all

and navigating to /foo/bar/experiment/123/ in my browser. I see these
requests:

(I have tf-nightly-20201204-py3.8, but that shouldn’t matter.)

wchargin · 2020-12-05T01:19:21Z

Let me know if you need more info to repro.

wchargin · 2020-12-05T01:25:23Z

Sorry, I don't understand the approach at all. /data/environment returning some information about path_prefix (directly or indirectly) sound about the same

Suppose that we’re at /foo/bar/experiment/123 and request the path
prefix info. Say that in case (a), it returns /foo/bar/ and we
interpret that accordingly. In case (b), it returns experiment/123/,
or, equivalently, ../.., and we interpret that accordingly.

@nfelt and I are saying that case (b) is nicer than case (a) because
TensorBoard behaves the same way independent of the path prefix.
All the XHR responses are the same with any --path_prefix value.
For instance, you can run this server and simply scrape all the HTML and
XHRs, then save them to static files and point a static server at them.
In case (b), TensorBoard continues to work if you move that directory
somewhere else relative to the web server. In case (a), it doesn’t.

and they are on the critical path of application bootstrap no matter what.

Agreed, but /data/environment is already on the critical path. If we
add a new /data/config XHR, then that’s an extra XHR on the critical
path, right? This is independent of the (a)/(b) discussion above.

stephanwlee · 2020-12-05T01:36:54Z

@wchargin, Thanks, I can repro. It is very surprising to me that we serve index.html on the /foo/bar/experiment/123/ as opposed to /foo/bar/baz/. I will look into the plugins_listing and subsequent calls being made under /foo/bar/experiment/123/data separately (since it is not related to path_prefix).

@nfelt and I are saying that case (b) is nicer than case (a) because
TensorBoard behaves the same way independent of the path prefix.
All the XHR responses are the same with any --path_prefix value.
For instance, you can run this server and simply scrape all the HTML and
...

That benefit seems very hypothetical and marginal. Response changing given current route (/experiment/123/ vs. /experiment/) and handling such stateful response seems more painful.

Agreed, but /data/environment is already on the critical path. If we
add a new /data/config XHR, then that’s an extra XHR on the critical
path, right? This is independent of the (a)/(b) discussion above.

Is /data/environment really on the critical path? I think /data/plugins_listing and /data/runs are but I am not so sure about the environments. I mean, you still see the spinner while the environment is loading but as soon as plugins listing is loaded, the dashboard view is visible on the screen. Also, in case of multi-experiments world, /data/config is only relevant
on the initial load and it will not matter on the subsequent dashboard navigations.

wchargin · 2020-12-07T22:23:08Z

Is /data/environment really on the critical path? I think /data/plugins_listing and /data/runs are but I am not so sure about the environments.

Good point. I stand corrected.

lukasmasuch · 2020-12-09T16:41:09Z

As a tensorboard user, I would very much prefer the solution suggested by @wchargin (case b). Allowing the frontend to work under any relative path independent from the configured path-prefix - e.g. by using window.location.pathname - makes it a lot easier and flexible to integrate into many different setups. For me, it always was a big strength of the previous versions of tensorboard to not have this limitation. Requiring the frontend to use the configured path-prefix of the backend server might have unplanned consequences for many integration scenarios (especially if reverse proxies are involved).

Other complex tools, such as code-server, are also able to have their frontend run under a relative path. As far as I know, they are also using the window.location.pathname to construct the base URL (see here).

To add an example which, as far as I understood, would not work with the implemented solution:

Service 1: Starts tensorboard with prefix /tool/tensorboard
Service 2: Reverse proxy that redirects all requests coming in on /experiments to service 1 to the path /tool/tensorboard
User accesses tensorboard in the browser on path /experiments

psybuzz

Marking as "request changes", to await feedback on a new approach using server side provided path.

stephanwlee

@nfelt PTAL and see if this aligns with what you were thinking of.

@psybuzz do note that after index.html, our header appeared a bit different :\ I could not discern exactly what has caused the regression so I had to apply flex.

nfelt

Yeah this looks good, thanks. (I think we'll eventually want to avoid CorePlugin directly using path_prefix but I'm happy to clean that up later.)

tensorboard/webapp/app_routing/app_root.ts

tensorboard/webapp/app_routing/app_root_test.ts

tensorboard/plugins/core/core_plugin_test.py

psybuzz

Side note: I think we may need router_link_directive_container.ts to become aware of AppRoots as well. Currently, when an <a> has [routerLink]=['foo'], we guarantee that the "href" attribute is a full, correct URL so things like ctrl+click properly open "http://tb/foo" in a new tab.
With path_prefix, I would imagine those hrefs now point to incorrect appRoot-less URLs.

tensorboard/webapp/app_routing/app_root.ts

tensorboard/webapp/app_routing/effects/app_routing_effects.ts

nfelt

Python changes and tests LGTM, thanks. Will defer to Erik's remaining comments before approving though.

wchargin · 2020-12-17T07:13:10Z

tensorboard/plugins/core/core_plugin.py

+        # Technically, it is possible to flush parts using yields but http_utils
+        # try to measure gzip and measure content length if a truthy value is
+        # passed.
+        res = http_util.Respond(request, None, "text/html")
+        res.stream.write(
+            '<meta name="tb-relative-root" content="%s/" />' % (relpath)
+        )
+        res.stream.write(index_asset_bytes)
+        return res


I don’t quite understand why this change implies the accompanying change
to http_util.Respond. The alternative that comes to mind is:

meta_header = '<meta name="tb-relative-root" content="%s/" />' % (relpath) content = meta_header.encode("utf-8") + index_asset_bytes return http_util.Respond(request, content, "text/html")

Judging from the comment, it sounds like you want to avoid that because
it automatically gzips, since the content type is text/html. That
makes sense. But then can’t we just pass content_encoding="identity"
rather than adding new mechanisms? I think that that already does the
right thing:

>>> from tensorboard.backend import http_util >>> from werkzeug import test as wtest >>> from werkzeug import wrappers >>> def req(): ... headers = [("Accept-Encoding", "gzip")] ... environ = wtest.EnvironBuilder(headers=headers).get_environ() ... return wrappers.Request(environ) ... >>> res = http_util.Respond(req(), b"<html>\n", "text/html") >>> res.headers.get("Content-Encoding"), res.get_data() ('gzip', b'\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\xb3\xc9(\xc9\xcd\xb1\xe3\x02\x00\x994\xcb3\x07\x00\x00\x00') >>> res = http_util.Respond(req(), b"<html>\n", "text/html", content_encoding="identity") >>> res.headers.get("Content-Encoding"), res.get_data() ('identity', b'<html>\n')

Failing that, I might prefer to add a new no_gzip flag or something
rather than introducing (a) the edge case in http_util and (b) the
stream-writing code in this method.

stephanwlee · 2020-12-17T17:29:00Z

Tested the sync (cl/347955446) and tested against the internal version of TB.

psybuzz

The frontend code lgtm, thanks for checking internally.

I also did a quick test sync and saw 1 projector test fail, but I'm not 100% sure its related: cl/348085083

Approving % wchargin@'s approval on the content_encoding=identity topic.

wchargin

Python revisions look good to me. Ideally I would like html.escape
around the HTML interpolation; even though there’s probably an argument
that it’s safe in this case due to provenance of relpath, it’s nice
for it to be “clearly correct” rather than “merely correct”.

nfelt

Took a quick look and the updates and generally LGTM. +1 to William's point that it would be a bit better to escape the inserted relative path, just in case.

nfelt · 2020-12-18T02:28:08Z

tensorboard/webapp/app_routing/views/router_link_directive_container.ts

-      pathname: this.pathname,
-    });
+    return (
+      this.appRoot.slice(0, -1) +


Would it maybe make sense to expose getAbsPathnameWithAppRoot() somewhere to kind of share this logic? As it is, it's not terribly obvious here why we slice the last char off appRoot unless you can see the bigger picture. Maybe at least a comment would help.

Done. Moved it to AppRootProvider instead.

Problem: TensorBoard has a flag, path_prefix, which serves frontend and the data endpoints at your specified, if the flag is passed, pathname. While exact use case slips my mind, it is certainly used by dependents. Now, the problem is that new Angular based TensorBoard has a custom router for in-app navigation (we may add new routes for experiment list and others) it is unaware of the path prefix. Router works by matching set of predefined routes to current pathname and finds a match. However, the configuration is unaware (rightfully) of the path prefix and it results in zero match, which in turn cause navigation to the default path, "/". This totally breaks the assumption of the path prefix as all the request from the frontend will be sent to "/data/..." as opposed to "[path_prefix]/data/..." Remediation: Previously, there was no reason to propagate the prefix information to the frontend. Application only did hash changes and never required a navigation. With this change, we now: - server side render meta tag that depicts the relative path to the root of the application - bootstrap Angular app from the relative root path and uses that information for matching routes and navigation Tests: - manually tested with/without path_prefix - tested dynamic plugin with/without path_prefix (projector)

stephanwlee · 2021-01-06T06:11:32Z

@psybuzz made significant changes to the FE code to adopt the feedback. Please TAL one last time.

psybuzz · 2021-01-06T19:41:45Z

tensorboard/webapp/app_routing/location.ts

@@ -54,6 +56,10 @@ function isNavigation(

 @Injectable()
 export class Location implements LocationInterface {


One suggestion: perhaps we should add a comment to appRootProvider.getFullPathFromRouteOrNav(), noting that this.appRootProvider.getAbsPathnameWithAppRoot() is required to get the "full full" path.

Problem: TensorBoard has a flag, path_prefix, which serves frontend and the data endpoints at your specified, if the flag is passed, pathname. While exact use case slips my mind, it is certainly used by dependents. Now, the problem is that new Angular based TensorBoard has a custom router for in-app navigation (we may add new routes for experiment list and others) it is unaware of the path prefix. Router works by matching set of predefined routes to current pathname and finds a match. However, the configuration is unaware (rightfully) of the path prefix and it results in zero match, which in turn cause navigation to the default path, "/". This totally breaks the assumption of the path prefix as all the request from the frontend will be sent to "/data/..." as opposed to "[path_prefix]/data/..." Remediation: Previously, there was no reason to propagate the prefix information to the frontend. Application only did hash changes and never required a navigation. With this change, we now: - server side render `<meta>` tag that depicts the relative path to the root of the application. - bootstrap Angular app from the relative root path and uses that information for matching routes and navigation

Backport of #4423 to 2.4. Cf. #4547. --- Problem: TensorBoard has a flag, path_prefix, which serves frontend and the data endpoints at your specified, if the flag is passed, pathname. While exact use case slips my mind, it is certainly used by dependents. Now, the problem is that new Angular based TensorBoard has a custom router for in-app navigation (we may add new routes for experiment list and others) it is unaware of the path prefix. Router works by matching set of predefined routes to current pathname and finds a match. However, the configuration is unaware (rightfully) of the path prefix and it results in zero match, which in turn cause navigation to the default path, "/". This totally breaks the assumption of the path prefix as all the request from the frontend will be sent to "/data/..." as opposed to "[path_prefix]/data/..." Remediation: Previously, there was no reason to propagate the prefix information to the frontend. Application only did hash changes and never required a navigation. With this change, we now: - server side render meta tag that depicts the relative path to the root of the application - bootstrap Angular app from the relative root path and uses that information for matching routes and navigation Tests: - manually tested with/without path_prefix - tested dynamic plugin with/without path_prefix (projector) Co-authored-by: Stephan Lee <[email protected]>

google-cla bot added the cla: yes label Dec 4, 2020

stephanwlee requested a review from psybuzz December 4, 2020 06:04

psybuzz reviewed Dec 5, 2020

View reviewed changes

stephanwlee force-pushed the pp branch from 9b260e7 to e803b4e Compare December 5, 2020 07:39

stephanwlee requested a review from psybuzz December 5, 2020 07:40

psybuzz requested changes Dec 9, 2020

View reviewed changes

stephanwlee force-pushed the pp branch 3 times, most recently from 9b600a5 to ccf0685 Compare December 11, 2020 02:49

stephanwlee commented Dec 11, 2020

View reviewed changes

stephanwlee force-pushed the pp branch 2 times, most recently from 685e807 to 43341bc Compare December 11, 2020 05:20

nfelt reviewed Dec 12, 2020

View reviewed changes

tensorboard/webapp/app_routing/app_root.ts Outdated Show resolved Hide resolved

tensorboard/webapp/app_routing/app_root_test.ts Outdated Show resolved Hide resolved

tensorboard/plugins/core/core_plugin_test.py Outdated Show resolved Hide resolved

stephanwlee requested review from nfelt and psybuzz December 14, 2020 20:14

psybuzz reviewed Dec 15, 2020

View reviewed changes

ggosiang mentioned this pull request Dec 15, 2020

regression test for starting tensorboard InfuseAI/primehub#224

Merged

2 tasks

nfelt reviewed Dec 16, 2020

View reviewed changes

wchargin reviewed Dec 17, 2020

View reviewed changes

stephanwlee requested a review from psybuzz December 17, 2020 17:29

psybuzz approved these changes Dec 17, 2020

View reviewed changes

wchargin approved these changes Dec 18, 2020

View reviewed changes

nfelt approved these changes Dec 18, 2020

View reviewed changes

stephanwlee added 11 commits January 5, 2021 18:39

cr address

e83bd9b

whoops

680c94b

cr

27e6d98

wrong comment

f7af700

ts misc fix

402df5f

types

0c574d0

test injector

992f3ff

header undo

6895b08

undo header change since doctype html fixes things

82d6f68

refactor to move appRoot utils to the provider

c00cf58

stephanwlee force-pushed the pp branch from 4a4b6c2 to c00cf58 Compare January 6, 2021 06:10

stephanwlee requested a review from psybuzz January 6, 2021 06:11

psybuzz approved these changes Jan 6, 2021

View reviewed changes

cr

34cdf80

stephanwlee merged commit 0fe35b5 into tensorflow:master Jan 13, 2021

stephanwlee deleted the pp branch January 13, 2021 22:55

MrBago mentioned this pull request Jan 13, 2021

--path_prefix broken in 2.4.0 #4421

Closed

wchargin mentioned this pull request Jan 13, 2021

Patch release: 2.4.1 #4547

Closed

2 tasks

wchargin mentioned this pull request Jan 14, 2021

2.4: ng: support path_prefix at router level (#4423) #4549

Merged

wchargin mentioned this pull request Jan 14, 2021

TensorBoard 2.4.1 #4554

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ng: support path_prefix at router level #4423

ng: support path_prefix at router level #4423

stephanwlee commented Dec 4, 2020 •

edited

Loading

wchargin commented Dec 4, 2020

stephanwlee commented Dec 4, 2020

wchargin commented Dec 4, 2020

psybuzz left a comment

psybuzz Dec 4, 2020

stephanwlee Dec 5, 2020

psybuzz Dec 4, 2020

stephanwlee Dec 5, 2020 •

edited

Loading

psybuzz Dec 5, 2020

stephanwlee Dec 5, 2020

nfelt commented Dec 5, 2020

stephanwlee commented Dec 5, 2020 •

edited

Loading

wchargin commented Dec 5, 2020

wchargin commented Dec 5, 2020

wchargin commented Dec 5, 2020 •

edited

Loading

stephanwlee commented Dec 5, 2020 •

edited

Loading

wchargin commented Dec 7, 2020

lukasmasuch commented Dec 9, 2020 •

edited

Loading

psybuzz left a comment

stephanwlee left a comment

nfelt left a comment

psybuzz left a comment

nfelt left a comment •

edited

Loading

wchargin Dec 17, 2020 •

edited

Loading

stephanwlee commented Dec 17, 2020

psybuzz left a comment

wchargin left a comment

nfelt left a comment

nfelt Dec 18, 2020

stephanwlee Jan 6, 2021

stephanwlee commented Jan 6, 2021

psybuzz Jan 6, 2021

	if experiment_metadata is not None:
	environment.update(
	{
	"experiment_name": experiment_metadata.experiment_name,
	"experiment_description": experiment_metadata.experiment_description,
	"creation_time": experiment_metadata.creation_time,
	}
	)

	readonly title$ = this.store.pipe(
	select(getEnvironment),
	map((env) => env.window_title \|\| 'TensorBoard'),
	// (it's an empty string when the `--window_title` flag is not set)
	distinctUntilChanged()

		@@ -54,6 +56,10 @@ function isNavigation(

		@Injectable()
		export class Location implements LocationInterface {

ng: support path_prefix at router level #4423

ng: support path_prefix at router level #4423

Conversation

stephanwlee commented Dec 4, 2020 • edited Loading

wchargin commented Dec 4, 2020

stephanwlee commented Dec 4, 2020

wchargin commented Dec 4, 2020

psybuzz 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

stephanwlee Dec 5, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfelt commented Dec 5, 2020

stephanwlee commented Dec 5, 2020 • edited Loading

wchargin commented Dec 5, 2020

wchargin commented Dec 5, 2020

wchargin commented Dec 5, 2020 • edited Loading

stephanwlee commented Dec 5, 2020 • edited Loading

wchargin commented Dec 7, 2020

lukasmasuch commented Dec 9, 2020 • edited Loading

psybuzz left a comment

Choose a reason for hiding this comment

stephanwlee left a comment

Choose a reason for hiding this comment

nfelt left a comment

Choose a reason for hiding this comment

psybuzz left a comment

Choose a reason for hiding this comment

nfelt left a comment • edited Loading

Choose a reason for hiding this comment

wchargin Dec 17, 2020 • edited Loading

Choose a reason for hiding this comment

stephanwlee commented Dec 17, 2020

psybuzz left a comment

Choose a reason for hiding this comment

wchargin left a comment

Choose a reason for hiding this comment

nfelt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephanwlee commented Jan 6, 2021

Choose a reason for hiding this comment

stephanwlee commented Dec 4, 2020 •

edited

Loading

stephanwlee Dec 5, 2020 •

edited

Loading

stephanwlee commented Dec 5, 2020 •

edited

Loading

wchargin commented Dec 5, 2020 •

edited

Loading

stephanwlee commented Dec 5, 2020 •

edited

Loading

lukasmasuch commented Dec 9, 2020 •

edited

Loading

nfelt left a comment •

edited

Loading

wchargin Dec 17, 2020 •

edited

Loading