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

[UI] TFMA visualization loads slowly #3114

Closed
Bobgy opened this issue Feb 19, 2020 · 13 comments
Closed

[UI] TFMA visualization loads slowly #3114

Bobgy opened this issue Feb 19, 2020 · 13 comments
Assignees
Labels

Comments

@Bobgy
Copy link
Contributor

Bobgy commented Feb 19, 2020

Part of #2283

What steps did you take:

  1. visit KFP UI -> run details page -> node details
  2. use custom visualization to run tfma visualization

What happened:

visualization loads slowly with some too many redirect errors like the following:
Screen Shot 2020-02-19 at 3 04 34 PM

What did you expect to happen:

There shouldn't be too many redirect errors, but scripts should be loaded from cdn directly.

Environment:

How did you deploy Kubeflow Pipelines (KFP)?

hosted

Anything else you would like to add:

[Miscellaneous information that will assist in solving the issue.]

/kind bug
/area frontend

@Bobgy
Copy link
Contributor Author

Bobgy commented Feb 19, 2020

I'm found root cause for the infinite redirection is:

  1. requirejs loads modules from local path first, so it tries to load tfma_widget_js and @jupyter/html-manager from https://281b9edc3093660f-dot-asia-east1.pipelines.googleusercontent.com/ (hosted kfp base path)
  2. however, the script runs in a sandboxed iframe, so it cannot use credentials to bypass base path proxy authentication
  3. therefore, base path redirects to google login page
  4. however, the login page request somehow can actually send credentials, so it just successfully verifies credentials and redirects back to kfp host base path
  5. no surprise, kfp host base path still cannot authenticate, so this creates a loop of redirection.

However, ... all of these shouldn't happen in the first place, because the js files should be loaded from external cdn directly.

@Bobgy
Copy link
Contributor Author

Bobgy commented Feb 19, 2020

A second problem is that, this @jupyter/html-manager should already load if user code uses the following

from ipywidgets.embed import embed_minimal_html

analysis_path = 'gs://<TFMA_OUTPUT_DIRECTORY>'
result = tfma.load_eval_result(output_path=analysis_path)
slicing_metrics_view = tfma.view.render_slicing_metrics(result)
embed_minimal_html('tfma_export.html', views=[slicing_metrics_view], title='Slicing Metrics')

@Bobgy
Copy link
Contributor Author

Bobgy commented Feb 19, 2020

The unnecessary html-manager comes from

which I believe doesn't load in current use cases.

The full template looks like it should be able to let users skip the step of calling embed_minimal_html, but it should configure a correct resources.ipywidgets_base_url like unpkg.com, to load html-manager

@ajchili Can you confirm if this is true?

This issue itself is kind of hard to fix, but if we remove usage in the full template, we can already save half of the time wasted on infinite redirection.

@Bobgy Bobgy changed the title TFMA visualization loads slowly [UI] TFMA visualization loads slowly Feb 19, 2020
@Bobgy Bobgy self-assigned this Feb 19, 2020
@Bobgy
Copy link
Contributor Author

Bobgy commented Feb 19, 2020

I'm going to verify if removing the addWidgetsRenderer can still allow current supported visualizations for TFDV and TFMA. We can add better support for this later.

@Bobgy
Copy link
Contributor Author

Bobgy commented Feb 19, 2020

Verified tfma and tfdv visualization still works without addWidgetsRenderer, and saves about 2 seconds from loading.

@Bobgy
Copy link
Contributor Author

Bobgy commented Feb 20, 2020

Filed an upstream feature request for @jupyter/html-manager to fix this issue: jupyter-widgets/ipywidgets#2786

@Bobgy Bobgy added priority/p1 status/triaged Whether the issue has been explicitly triaged labels Feb 27, 2020
@Bobgy
Copy link
Contributor Author

Bobgy commented Mar 10, 2020

My upstream PR is merged, I will wait for ipywidgets 8.0 release to switch to use my new feature

@Bobgy
Copy link
Contributor Author

Bobgy commented May 8, 2020

#3712 is pretty much all we can do now, integrating with new (not yet released) ipywidgets 8.0 can be post 1.0

@stale
Copy link

stale bot commented Aug 6, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the lifecycle/stale The issue / pull request is stale, any activities remove this label. label Aug 6, 2020
@Bobgy
Copy link
Contributor Author

Bobgy commented Aug 6, 2020

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen and removed lifecycle/stale The issue / pull request is stale, any activities remove this label. labels Aug 6, 2020
@rimolive
Copy link
Member

Closing this issue. No activity for more than a year.

@rimolive
Copy link
Member

rimolive commented Apr 9, 2024

/close

Copy link

@rimolive: Closing this issue.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

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

No branches or pull requests

3 participants