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

SVG Plot viewer perf fix #6913

Closed
DonJayamanne opened this issue Jul 29, 2021 · 5 comments
Closed

SVG Plot viewer perf fix #6913

DonJayamanne opened this issue Jul 29, 2021 · 5 comments
Assignees
Labels
perf Performance issues plot-viewer
Milestone

Comments

@DonJayamanne
Copy link
Contributor

  • Stop generating SVG
  • Display plot viewer with PNGs
  • Ensure exporting notebooks to PDF works even without SVG
@IanMatthewHuff
Copy link
Member

IanMatthewHuff commented Aug 11, 2021

I was out for some of the discussion on this, so I wanted to bump the item for a quick recap.

Current state:

  • Customers (via issues / request) want the plot viewer
  • The current method of generating "hidden" SVGs under the surface is crummy, it can slow down execution and bulk up files in a way that customers don't expect
  • Customers might not care about the "advanced" plot features as opposed to just the ability to view plots off to the side in a separate view from the notebook

Proposed solution(s):

  1. Do nothing, not good. We create a hidden perf issue that hits many customers and is difficult to diagnose (as the SVG is not visible).
  2. Turn off plot viewer by default, customers need to opt in to the setting. Bad as it requires customers to opt into a setting, many will not find it and still has the issue of not knowing which plots will cause massive SVGs.
  3. Same as 2, but leave the button in as a discoverability point, user can click the plot viewer button and if there is not the hidden SVG then we can prompt to turn on the setting (which will warn of the perf cost).
  4. I'm iffy on the technical feasibility here (as the SVG generation is out of our control) but we could look for a way to turn on the setting, but bail out when we are generating SVGs of over a certain size. Like our max output size for streaming output. Then users could opt in to just those plots if they are sure they want them.
  5. Don't generate the hidden SVGs anymore, and instead just use images. If we are working with the current basic plotviewer UI we would need to change the react components to work with images instead of SVGs only like the current implementation. Downside here would be we then wouldn't be supporting SVG plots anymore, which a user might also want in the plot viewer.
  6. Don't generate hidden SVGs. Using the current UI, support both images and SVGs. We could either disable the SVG only features (zoom ect..) or show them only for SVGs. The current UI would need a fair bit of adjustment to support this.
  7. Don't generate hidden SVGs. Ditch the current UI and look at moving over to a more general image viewing control that supports both SVGs and Images. Good time to examine fluent controls as the current plot viewer is not egregious, but also not really slick or VS Code native looking.

@IanMatthewHuff
Copy link
Member

One question here to examine is what do customers want from the plot viewer. We have feedback that they want it, but do they really care about zoom, pan, ect? Does just a basic image list work for 90% of users? Also are we worried enough about the perf issues here that we implement 1 - 3 quickly while we work on a bigger solution.

@IanMatthewHuff
Copy link
Member

Per Rich another thing could be seeing if the current SVG control will support images referenced from SVG. That way for a quick and dirty fix we can use the existing UI control to host images without UI changes and without generating SVGs.

@greazer
Copy link
Member

greazer commented Aug 11, 2021

Not sure if this is covered in the above. It's like a combination of 5 and 6. The setting would be only whether SVG's should be generated. By default it would just be basic images. If the user wants more fidelity, they'll need to turn on the switch and understand the consequences.

@IanMatthewHuff
Copy link
Member

Yeah, that sounds like a useful side feature. We can support images or SVGs. But we could still have an opt-in feature to generate SVGs alongside plot images automatically. I don't think that any of the solutions here would preclude that, and it's a good idea.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
perf Performance issues plot-viewer
Projects
None yet
Development

No branches or pull requests

3 participants