Skip to content

Conversation

@wchargin
Copy link
Contributor

@wchargin wchargin commented Oct 6, 2019

Summary:
This commit changes the frontend experiment ID plumbing from using the
query string to using a path component: experiment URLs are now of the
form http://localhost:6006/experiment/123/. This supersedes the
short-term changes from #2580, #2613, and #2627.

Test Plan:
Apply the below patch to the multiplexer data provider, then launch
TensorBoard with --generic_data=true and verify that the correct
experiment ID is printed for each call, at / and at /experiment/123.
Verify also that no request has an experiment query parameter. Patch:

diff --git a/tensorboard/backend/event_processing/data_provider.py b/tensorboard/backend/event_processing/data_provider.py
index 0b354aa8..1c00b930 100644
--- a/tensorboard/backend/event_processing/data_provider.py
+++ b/tensorboard/backend/event_processing/data_provider.py
@@ -59,2 +59,3 @@ class MultiplexerDataProvider(provider.DataProvider):
   def data_location(self, experiment_id):
+    logger.warn("data_location(%r)", experiment_id)
     del experiment_id   # ignored
@@ -63,2 +64,3 @@ class MultiplexerDataProvider(provider.DataProvider):
   def list_runs(self, experiment_id):
+    logger.warn("list_runs(%r)", experiment_id)
     del experiment_id  # ignored for now
@@ -74,2 +76,3 @@ class MultiplexerDataProvider(provider.DataProvider):
   def list_scalars(self, experiment_id, plugin_name, run_tag_filter=None):
+    logger.warn("list_scalars(%r, ...)", experiment_id)
     del experiment_id  # ignored for now
@@ -105,2 +108,3 @@ class MultiplexerDataProvider(provider.DataProvider):
   ):
+    logger.warn("read_scalars(%r, ...)", experiment_id)
     # TODO(@wchargin): Downsampling not implemented, as the multiplexer

wchargin-branch: eid-frontend

Summary:
Currently, we apply the `_handle_errors` middleware as a decorator
directly on `__call__`. We plan to add middleware that is parameterized
on instance variables, so this pattern will no longer suffice. This
commit simply refactors the existing code to apply middleware at
initialization time.

Test Plan:
It suffices to run `bazel test //tensorboard/backend:application_test`.

wchargin-branch: app-middleware
wchargin-source: 77c4f793de8b64d88bd10212db5a0191995b29ca
Summary:
This way, we can reason about and test it in isolation as we add more
middleware to the application.

Note that when using `--path_prefix`, loading the main TensorBoard page
without a trailing slash on the URL has long been broken (#1117). This
commit does not fix that, but changes the exact failure mode: rather
than 404ing, we now load a broken TensorBoard (relative URLs don’t
resolve). A follow-up commit will fix this properly.

Test Plan:
Verify that TensorBoard still works fully, with `--path_prefix` and
without it.

wchargin-branch: path-prefix-middleware
wchargin-source: 3e07d2b681f12c4bab310bef2afc066590ae5cb7
Summary:
When using a `--path_prefix`, TensorBoard has historically required a
trailing slash after that prefix: e.g., one visits `localhost:6006/foo/`
rather than `localhost:6006/foo`. (See, for instance, #1176.) Different
versions of TensorBoard have different failure modes when the non-slash
path is loaded; currently, the TensorBoard shell loads, but the frontend
computes relative URLs incorrectly. This commit adds a redirect from the
empty path to `/` to avoid the problem.

This logic could be inlined into the `PathPrefixMiddleware`, but we’ll
soon have other middlewares with similar needs, so providing this as its
own middleware avoids duplicating the functionality.

Test Plan:
Launch TensorBoard with `--path_prefix /foo` (or `--path_prefix /foo/`;
the two are equivalent) and navigate to `/foo/` and `/foo` in a browser.
Note that they now both work and resolve to `/foo/`, while prior to this
commit navigating to `/foo` yielded a broken TensorBoard.

wchargin-branch: empty-path-redirect
wchargin-source: 300682590a9a44f1b37d16168b457b4f5898463c
Summary:
This commit teaches TensorBoard to recognize URLs like `/experiment/123`
as specifying experiment IDs. If these two path components are omitted,
the experiment ID is the empty string, for backward compatibility. The
experiment ID is intercepted by middleware and stripped from the URL
before the application handles it, so no existing logic needs to change.
The ID is added to the WSGI environment and may be extracted via a new
public function, `plugin_util.experiment_id`.

This commit does not yet replace the existing query parameter wiring, so
this is not a user-facing change (except that more URLs are now valid).

This functionality is compatible with `--path_prefix`.

Note that the `/data/plugins_listing` implementation does not need to
change: it correctly specifies experiment IDs in iframe loading
mechanisms because we already use the general-purpose `SCRIPT_NAME` key
specified by WSGI for this purpose.

Test Plan:
Unit tests and a small integration test in `:application_test` included.
For manual testing, modify `application.py`’s `_route_request` method to
print out the application-level path and experiment ID:

```python
    # at top of `_route_request`:
    from tensorboard import plugin_util
    eid = plugin_util.experiment_id(environ)
    logger.warn("%r [%r]" % (environ.get("PATH_INFO"), eid))
```

Then, check that the scalars plugin and the dynamically loaded projector
plugin both work, and that the paths and experiment IDs are as expected,
in the following configurations:

  - with `--path_prefix=/foo`:
      - at `/foo` (should redirect to add slash)
      - at `/foo/`
      - at `/foo/experiment/123` (should redirect to add slash)
      - at `/foo/experiment/123/`
  - with no path prefix:
      - at `/`
      - at `/experiment/123` (should redirect to add slash)
      - at `/experiment/123/`

wchargin-branch: eid-middleware
wchargin-source: 21f505c418f05a90ed434a9ca86b41959a39040f
Summary:
This commit changes the frontend experiment ID plumbing from using the
query string to using a path component: experiment URLs are now of the
form `http://localhost:6006/experiment/123/`. This supersedes the
short-term changes from #2580, #2613, and #2627.

Test Plan:
Apply the below patch to the multiplexer data provider, then launch
TensorBoard with `--generic_data=true` and verify that the correct
experiment ID is printed for each call, at `/` and at `/experiment/123`.
Verify also that no request has an `experiment` query parameter. Patch:

```diff
diff --git a/tensorboard/backend/event_processing/data_provider.py b/tensorboard/backend/event_processing/data_provider.py
index 0b354aa..1c00b930 100644

wchargin-source: c5b1a893a67cb82286f9de0f5b2a0638dc372a54
--- a/tensorboard/backend/event_processing/data_provider.py
+++ b/tensorboard/backend/event_processing/data_provider.py
@@ -57,10 +57,12 @@ class MultiplexerDataProvider(provider.DataProvider):
       return None

   def data_location(self, experiment_id):
+    logger.warn("data_location(%r)", experiment_id)
     del experiment_id   # ignored
     return str(self._logdir)

   def list_runs(self, experiment_id):
+    logger.warn("list_runs(%r)", experiment_id)
     del experiment_id  # ignored for now
     return [
         provider.Run(
@@ -72,6 +74,7 @@ class MultiplexerDataProvider(provider.DataProvider):
     ]

   def list_scalars(self, experiment_id, plugin_name, run_tag_filter=None):
+    logger.warn("list_scalars(%r, ...)", experiment_id)
     del experiment_id  # ignored for now
     run_tag_content = self._multiplexer.PluginRunToTagToContent(plugin_name)
     result = {}
@@ -103,6 +106,7 @@ class MultiplexerDataProvider(provider.DataProvider):
   def read_scalars(
       self, experiment_id, plugin_name, downsample=None, run_tag_filter=None
   ):
+    logger.warn("read_scalars(%r, ...)", experiment_id)
     # TODO(@wchargin): Downsampling not implemented, as the multiplexer
     # is already downsampled. We could downsample on top of the existing
     # sampling, which would be nice for testing.
```

wchargin-branch: eid-frontend
wchargin-branch: eid-middleware
wchargin-source: e956be9e14a712e424594550d58ef337710a0a2b
wchargin-branch: eid-frontend
wchargin-source: 1a5ef46116240bbeebdec17b2e50eac34ae5017d
wchargin-branch: eid-frontend
wchargin-source: 1a5ef46116240bbeebdec17b2e50eac34ae5017d
Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

I think I am missing one critical information. Does this change require the document to be served at localhost:6006/experiment/[:eid]/?

  1. I think code makes sense.
  2. currently TB FE is not that experiment aware. When it increasingly is aware of the experiment, I think it is inevitable that we will introduce experiment param back to the router.
  3. there should be separate router implementation from different implementations of the server.

Because (1) and (2) is a future work, this change LGTM.

@wchargin
Copy link
Contributor Author

wchargin commented Oct 6, 2019

I’m not sure that I fully understand the question. The frontend already
assumes that it’s served from /, because otherwise the relative URLs
will resolve incorrectly. After this change, we still support loading
from / (with empty experiment ID) and additionally support loading
from /experiment/EID/. Does this answer your question?

If you’re asking about caching of the large HTML blob of the document
itself: It’s true that this won’t have a shared cache across two
experiments, but that was also true for the query parameter version. In
either case, we can improve the caching by loading the bundle (or parts
of it) dynamically on the frontend.

When it increasingly is aware of the experiment, I think it is
inevitable that we will introduce experiment param back to the
router.

I agree that this is plausible, especially because you can’t just route
to ../456/ to change the experiment ID, because when you’re at /
instead of /experiment/123/ this would do the wrong thing.

wchargin-branch: empty-path-redirect
wchargin-source: 5cb309fa62fd9efac68ac52b8a57fcef67a2fcb5

# Conflicts:
#	tensorboard/backend/BUILD
#	tensorboard/backend/application.py
wchargin-branch: empty-path-redirect
wchargin-source: 5cb309fa62fd9efac68ac52b8a57fcef67a2fcb5
wchargin-branch: empty-path-redirect
wchargin-source: fa2c0afa5ecc3882d03ea695c6370edaa77399a2
wchargin-branch: eid-middleware
wchargin-source: f581a5c84d81b331ffcae4c0ef7dfbc32da9986e
wchargin-branch: eid-middleware
wchargin-source: f581a5c84d81b331ffcae4c0ef7dfbc32da9986e
wchargin-branch: eid-middleware
wchargin-source: 1883d4c6fa7cf726cd9fc159b325232080e856db

# Conflicts:
#	tensorboard/backend/BUILD
#	tensorboard/backend/application.py
wchargin-branch: eid-middleware
wchargin-source: 1883d4c6fa7cf726cd9fc159b325232080e856db
wchargin-branch: eid-frontend
wchargin-source: c6e0646979c2596e5272430ccef6bbb78049e76a
@wchargin wchargin changed the base branch from wchargin-eid-middleware to master October 8, 2019 01:04
wchargin-branch: eid-frontend
wchargin-source: 93b5437804637f4c7a5f5a7dee02164d7c1200ae
wchargin-branch: eid-frontend
wchargin-source: 48cd3bd4e1b732905adc66b2853ab8ef9ec16002
wchargin-branch: eid-frontend
wchargin-source: 6782ff7bf792fb094e8e990ddfffe5b24ee264ff
@wchargin wchargin merged commit cbc3494 into master Oct 8, 2019
@wchargin wchargin deleted the wchargin-eid-frontend branch October 8, 2019 05:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants