Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix compatibility with TB >= 2.2 #63

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

cliffwoolley
Copy link

tensorflow/tensorboard#3539 in TensorBoard 2.2 dropped the legacy DB mode, so the unused optional db_* parameters to base_plugin.TBContext() should be dropped, as in tensorflow/tensorboard@628b78f#diff-9f7ffee070d3d2429db0b7178d355ec6L229-L241

Add handler for POST requests to TensorboardHandler, providing support
for hparams plugin.
Expand xsrf_cookie exceptions, normally only applied to GET and HEAD
requests in the IPythonHandler, to POST requests in TensorboardHandler.

Provides support for hparams plugin, which uses POST to retrieve
experiment information but can't be trivially extended to include xsrf
information in these POST requests. Mirrors existing IPythonHandler
behavior, falling back to Referer header rather than form parameters.
@cliffwoolley
Copy link
Author

/cc @wchargin

@leonardschneider
Copy link

Why is this not merged already? Bumped into this issue.

Provides for better compatibility with TB 2.2, 2.3, 2.4; also better future-proofness

Signed-off-by: Cliff Woolley <jwoolley@nvidia.com>
Signed-off-by: Cliff Woolley <jwoolley@nvidia.com>
Signed-off-by: Cliff Woolley <jwoolley@nvidia.com>
Examples of dynamic plugins include the Projector plugin as well as NVIDIA's DLProf plugin

Signed-off-by: Cliff Woolley <jwoolley@nvidia.com>
@cliffwoolley cliffwoolley changed the title Drop deprecated (optional) db_* params for TB 2.2 compatibility Fix compatibility with TB 2.3 and 2.4 Feb 5, 2021
@cliffwoolley cliffwoolley changed the title Fix compatibility with TB 2.3 and 2.4 Fix compatibility with TB 2.2, 2.3, 2.4 Feb 5, 2021
@cliffwoolley
Copy link
Author

I've updated this PR from last year to also include TB 2.3 and TB 2.4 now.

Plus the newer, simplified implementation should allow better future-proofness as well:
Instead of pulling out a huge chunks of the entry function to TensorBoard (which by the way no longer exists in TB 2.4) and reimplementing it mostly just to catch the returned object to add to our tensorboard_manager; instead we now just wrap the unmodified TB function from the outside and add to the tensorboard_manager that way. Also we no longer attempt to implement any special multiplexing functionality since TB handles all that on its own and the threading being handled in two places was causing bugs.

Even with this change, there is a missing XSRF (cross-site request forgery) header issue
with this portion of TensorBoard 2.4 functionality; see tensorflow/tensorboard#4685 .

Jupyter can be configured to disable XSRF checking as a short-term workaround, but this
is less than desirable.

Signed-off-by: Cliff Woolley <jwoolley@nvidia.com>
Signed-off-by: Cliff Woolley <jwoolley@nvidia.com>
Extends XSRF header exceptions to POST requests per discussion in tensorflow/tensorboard#4685 (comment)

Signed-off-by: Cliff Woolley <jwoolley@nvidia.com>
@rvernaeshv
Copy link

Thanks for this fix... It helps me a lot

@wexder
Copy link

wexder commented Apr 6, 2021

@lspvic can we merge this?

@cliffwoolley
Copy link
Author

This seems to be working with TB 2.5 as well, fwiw.

cliffwoolley referenced this pull request in cliffwoolley/jupyter_tensorboard Jun 8, 2021
Merges and adapts changes from lspvic#54
to fix TensorBoard 2.4.x scalar card compatibility

Signed-off-by: Cliff Woolley <jwoolley@nvidia.com>
@billshitg
Copy link

just tried this with TB 2.7 and it worked very well. amazing! thanks for the fix. why has it not been merged already?

@cliffwoolley cliffwoolley changed the title Fix compatibility with TB 2.2, 2.3, 2.4 Fix compatibility with TB >= 2.2 Dec 12, 2021
@videodanchik
Copy link

@lspvic Could you please merge this feature to master and bump up the library version?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants