Serve TensorBoard at /flags #5796
Conversation
| export interface NavigateToFlags { | ||
| routeKind: RouteKind.FLAGS; | ||
| routeParams: {}, | ||
| resetNamespacedState?: boolean; | ||
| } | ||
|
|
||
| export type ProgrammaticalNavigation = | ||
| | NavigateToExperiment | ||
| | NavigateToCompare | ||
| | NavigateToExperiments; | ||
| | NavigateToExperiments | ||
| | NavigateToFlags; |
There was a problem hiding this comment.
This is not technically necessary but it does make the typing more accurate.
There was a problem hiding this comment.
I don't have strong feelings about this either way. However, I would have left this out because we do not want to programmatically navigate.
I can definitely see this being useful one day if we decide to have a secret button that navigates here or something. But, I think we can add it then.
rileyajones
force-pushed
the
feature-flags
branch
from
d986c1f to
1edb1bb
Compare
| for app_path in APP_PATHS: | ||
| path_with_slash = ( | ||
| app_path | ||
| if app_path.endswith("/") | ||
| else app_path + "/" | ||
| ) | ||
| apps[path_with_slash + path] = functools.partial( | ||
| self._serve_index, content, app_path | ||
| ) | ||
| continue | ||
|
|
||
| gzipped_asset_bytes = _gzip(content) | ||
| wsgi_app = functools.partial( | ||
| self._serve_asset, path, gzipped_asset_bytes | ||
| ) | ||
| apps["/" + path] = wsgi_app | ||
| apps["/"] = apps["/index.html"] | ||
| for app_path in APP_PATHS: | ||
| path_with_slash = ( | ||
| app_path | ||
| if app_path.endswith("/") | ||
| else app_path + "/" | ||
| ) | ||
| apps[path_with_slash + path] = wsgi_app | ||
| for app_path in APP_PATHS: | ||
| if app_path.endswith("/"): | ||
| apps[app_path] = apps[app_path + "index.html"] | ||
| else: | ||
| apps[app_path] = apps[app_path + "/index.html"] | ||
| apps[app_path] = apps[app_path] |
There was a problem hiding this comment.
This DEFINITELY needs a unit test
There was a problem hiding this comment.
Unit test here would be great. I don't understand what line 143 does.
JamesHollyer
left a comment
JamesHollyer
left a comment
There was a problem hiding this comment.
This LGTM. However, I am not familiar with this code enough to sign off on it. I am going to add Brian as a reviewer so sign off on the python piece.
| for app_path in APP_PATHS: | ||
| path_with_slash = ( | ||
| app_path | ||
| if app_path.endswith("/") | ||
| else app_path + "/" | ||
| ) | ||
| apps[path_with_slash + path] = functools.partial( | ||
| self._serve_index, content, app_path | ||
| ) | ||
| continue | ||
|
|
||
| gzipped_asset_bytes = _gzip(content) | ||
| wsgi_app = functools.partial( | ||
| self._serve_asset, path, gzipped_asset_bytes | ||
| ) | ||
| apps["/" + path] = wsgi_app | ||
| apps["/"] = apps["/index.html"] | ||
| for app_path in APP_PATHS: | ||
| path_with_slash = ( | ||
| app_path | ||
| if app_path.endswith("/") | ||
| else app_path + "/" | ||
| ) | ||
| apps[path_with_slash + path] = wsgi_app | ||
| for app_path in APP_PATHS: | ||
| if app_path.endswith("/"): | ||
| apps[app_path] = apps[app_path + "index.html"] | ||
| else: | ||
| apps[app_path] = apps[app_path + "/index.html"] | ||
| apps[app_path] = apps[app_path] |
There was a problem hiding this comment.
Unit test here would be great. I don't understand what line 143 does.
| export interface NavigateToFlags { | ||
| routeKind: RouteKind.FLAGS; | ||
| routeParams: {}, | ||
| resetNamespacedState?: boolean; | ||
| } | ||
|
|
||
| export type ProgrammaticalNavigation = | ||
| | NavigateToExperiment | ||
| | NavigateToCompare | ||
| | NavigateToExperiments; | ||
| | NavigateToExperiments | ||
| | NavigateToFlags; |
There was a problem hiding this comment.
I don't have strong feelings about this either way. However, I would have left this out because we do not want to programmatically navigate.
I can definitely see this being useful one day if we decide to have a secret button that navigates here or something. But, I think we can add it then.
|
@JamesHollyer I spoke with @bmd3k and @nfelt about this on Friday and we decided to use a query param and a modal instead to prevent the python routing code from diverging. That is also why I didn't bother adding a test btw. |
2 participants
Motivation for features / changes
There is an existing page available at the route "/flags" on the frontend, however, the backend does support this route so it is functionally inaccessible.
Technical description of changes
Rather than just serving that app at "/" I added a list of routes for the app to be served at.
The meta tag content attribute is now generated using parameter which is closured during app startup.
get_resource_apps now generates resource paths for ALL supported routes.
Because of the logic in the
clean_pathfunction found inapplication.pywhich prevents paths from ending in "/" it is necessary to support routes which DO and DO NOT end with "/" slightly convoluting the logic.Screenshots of UI changes
Detailed steps to verify changes work correctly (as executed by you)
Start the application and manually navigate to the "/flags" route. Assert that the page is rendered.
Alternate designs / implementations considered