backend: fix --path_prefix validation
#2737
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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
Summary: Specifying a path prefix without leading slash has always been disallowed by the docs, but in TensorBoard 2.0.0 this just hits an inscrutable internal error when starting the server: ``` ValueError: invalid literal for int() with base 10: '6006my_path_prefix' ``` As of #2733, this is at least a correctly reported error, but it still has a stack trace. This commit makes it a normal, fast-failing usage error. The logic to strip trailing slashes was also duplicated, so `/foo` and `/foo/` and `/foo//` were all the same, but differed from `/foo///`. This commit removes one occurrence and makes the remaining one remove all trailing slashes instead of just one. Test Plan: Run with `--path_prefix /foo`, and note that TensorBoard exits with a single-line error. Run with `/foo`, `/foo/`, `/foo//`, and `/foo///`, and note that all four now have the same behavior. wchargin-branch: path-prefix-validation wchargin-source: 514c3bc317a1b90561f29f378919424b4284cc66
wchargin-branch: eid-middleware wchargin-source: e956be9e14a712e424594550d58ef337710a0a2b
wchargin-branch: eid-frontend wchargin-source: 1a5ef46116240bbeebdec17b2e50eac34ae5017d
wchargin-branch: eid-frontend wchargin-source: 1a5ef46116240bbeebdec17b2e50eac34ae5017d
wchargin-branch: path-prefix-validation wchargin-source: 633deb9e5d68c272c2fe90973881ba0331d30e47
stephanwlee
approved these changes
Oct 6, 2019
| flags.path_prefix = flags.path_prefix.rstrip('/') | ||
| if flags.path_prefix and not flags.path_prefix.startswith('/'): | ||
| raise FlagsError( | ||
| 'Path prefix must start with slash, but got: %r.' % flags.path_prefix) |
Contributor
There was a problem hiding this comment.
unless the test is already present, consider adding a test on this.
Contributor
Author
There was a problem hiding this comment.
Thanks for the suggestion. Done.
wchargin-branch: path-prefix-validation wchargin-source: 97cef390d2de7e0d1dc6e07b01af8a0713c78e76
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-branch: path-prefix-validation wchargin-source: 3e7107f01f52aed96fa288304f8c811d9714bc0d
wchargin-branch: eid-frontend wchargin-source: 93b5437804637f4c7a5f5a7dee02164d7c1200ae
wchargin-branch: eid-frontend wchargin-source: 48cd3bd4e1b732905adc66b2853ab8ef9ec16002
wchargin-branch: path-prefix-validation wchargin-source: 6dc6913017d9688608042c3ee8a30619eac6ca15
wchargin-branch: eid-frontend wchargin-source: 6782ff7bf792fb094e8e990ddfffe5b24ee264ff
wchargin-branch: path-prefix-validation wchargin-source: 74025383f7491ebdbff4a3fcaa6026f965b072fe
wchargin-branch: path-prefix-validation wchargin-source: f2ebc96feb4b894cb876bdbc8f56780156119757
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
Specifying a path prefix without leading slash has always been
disallowed by the docs, but in TensorBoard 2.0.0 this just hits an
inscrutable internal error when starting the server:
As of #2733, this is at least a correctly reported error, but it still
has a stack trace. This commit makes it a normal, fast-failing usage
error.
The logic to strip trailing slashes was also duplicated, so
/fooand/foo/and/foo//were all the same but differed from/foo///. Thiscommit removes one copy of the logic and makes the remaining copy strip
all trailing slashes instead of just one.
Test Plan:
Run with
--path_prefix /foo, and note that TensorBoard exits with asingle-line error. Run with
/foo,/foo/,/foo//, and/foo///,and note that all four now have the same behavior.
wchargin-branch: path-prefix-validation