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

Improve reliability, performance and feature coverage of the layout #8084

Closed
12 of 16 tasks
mattpap opened this issue Jul 17, 2018 · 19 comments
Closed
12 of 16 tasks

Improve reliability, performance and feature coverage of the layout #8084

mattpap opened this issue Jul 17, 2018 · 19 comments

Comments

@mattpap
Copy link
Contributor

mattpap commented Jul 17, 2018

This is a meta issue addressing specific issues with the layout (tag: layout).

  • words_and_plots.py
    blank until browser resize, DOM text on top of iris plot after

  • models/glyphs.py
    tab bars truncated

  • dash
    range plot on top of (covering) the line plot

  • gapminder
    widgets on top of scatter plot
    plot can overlap top of text below
    Play/Pause does not work

  • movies
    FIXED_SIZING_MODE warning
    huge gap between top text and bokeh content, until browser resize, then snaps to place

  • OHLC
    would be nice if sliders and button were vertically aligned

  • spectrogram (issue Add support for spans to grid layout #8710)

    • slider label overlaps text above
    • would be nice if right plot top/bottom border aligned with tops/bottoms of plots to the left
  • integration/glyphs/hex_tile (issue Range-based aspect management doesn't cooperate with layout #8711)

    Last column of plots has too narrow left panel (minus sign is cut). This isn't strictly an issue with the new layout, but a problem with the range-based aspect management scheme, which doesn't work well with old layout either, causing infinite update loops (e.g. plotting/file/aspect).

  • add GridPlot model and make handling of a toolbar an implementation detail

  • finalize implementation of flow layout (HTMLBox, Div, some other widgets)

  • add support for tabs scrolling (or some other mechanism, e.g. a dropdown)

@mattpap mattpap added this to the 1.0 milestone Jul 17, 2018
@mattpap mattpap added this to In progress in Must have tasks for 1.0 Jul 17, 2018
@mattpap mattpap added this to Triage in Focus: Layout Aug 25, 2018
@mattpap mattpap moved this from Triage to In progress in Focus: Layout Sep 10, 2018
@bryevdv bryevdv modified the milestones: 1.0, 1.1 Oct 1, 2018
@mattpap mattpap removed this from In progress in Must have tasks for 1.0 Nov 13, 2018
@mattpap

This comment has been minimized.

@mattpap

This comment has been minimized.

@bryevdv
Copy link
Member

bryevdv commented Feb 12, 2019

@mattpap @philippjfr OK I have consolidated the remaining open issues into the top. We need to prioritize these. For reference, I would like to release 1.1 next week -- monday or tuesday. The critical things I am aware of are:

are there others? Please edit the issue at the top to highlight or call out anything that should be regarded as critical for 1.1

@philippjfr
Copy link
Contributor

philippjfr commented Feb 12, 2019

Just to provide a bit more color for the performance issue, I've recorded some screencasts comparing the before and after.

Here is a screencast of the sample app with 1.0.4 and here is the performance profiling output for two events:

screen shot 2019-02-12 at 6 29 36 pm

Here is a screencast of the sample app with current master and here is the performance profiling output for two events:

screen shot 2019-02-12 at 6 16 56 pm

You can see that it goes from 200 ms in 1.0.4 (which feels instantaneous) to about 1 second on master (with a noticeable lag). It's slightly better when the profiler is not enabled.

Finally here is the stack output from the profiler:

screen shot 2019-02-12 at 6 37 19 pm

@mattpap
Copy link
Contributor Author

mattpap commented Feb 12, 2019

I would like to release 1.1 next week -- monday or tuesday.

That's unrealistic in my opinion. I would say last week of February (in two weeks) would be better.

@bryevdv
Copy link
Member

bryevdv commented Feb 12, 2019

ok, I think @philippjfr wanted more time as well, but I would say it is important to have 1.1 before the end of the month.

@bryevdv
Copy link
Member

bryevdv commented Feb 12, 2019

FYI regarding the issue above, removing some of the redundant/extraneous nesting did result in a significant improvement. But @philippjfr will post more info later.

@mattpap
Copy link
Contributor Author

mattpap commented Feb 12, 2019

I already investigated this in the PR. De-nesting will help, but the problem is with super slow measurement procedure of arbitrary HTML components. When replaced by a constant, speed improves a factor of 10-20, which translates to layout under 50ms in my VM.

@philippjfr
Copy link
Contributor

philippjfr commented Feb 13, 2019

I've been able to reduced the nesting in my example by one level (and updated the example code) by eliminating the use of widget boxes in panel. That indeed makes a fair difference but it would of course still be great if we could speed up the measurement procedure.

@philippjfr
Copy link
Contributor

Should I be opening new issues for other issues I've encountered? If I display the app example in a notebook and navigate away from and back to the notebook tab this is what happens to the layout:

screen shot 2019-02-13 at 1 32 07 pm

@mattpap
Copy link
Contributor Author

mattpap commented Feb 13, 2019

@philippjfr, yes, this one in particular deserves individual treatment.

@birdsarah
Copy link
Member

The critical things I am aware of are .....

I would include the lack of axis alignment (#8643) as a critical issue. If everyone runs 1.1 and all their plots have moved I would imagine it will be regarded as a serious regression. The whole point of the original layout PR was that things were aligned. You could already lay things out.

@bryevdv
Copy link
Member

bryevdv commented Feb 13, 2019

You could already lay things out.

Yes and no :) There were lots of ways to break the original layout before the solver existed. And after it was added there were easily obtainable combinations of stretch/scale modes that led to bad results, or just nothing rendering at all. Also the solver overhead made it impossible to have more than ~10 plots on a page. I've gotten explicit feedback from users that fixing these bugs and limitations is far more pressing, especially when you consider typical dashboards don't seem to rely on this capability. A random GIS image search for "dashboard" mostly yields a lot of individual stretchy plots in a individual cells:

screen shot 2019-02-13 at 08 32 35

screen shot 2019-02-13 at 08 41 21

Just this kind of simple thing (which has no fancy alignment) is what people are clamoring to be able to do, and to be frank, what they are sometimes choosing to use other tools for, for lack of. In hindsight I'd say I would say that in prioritizing the alignment capability, I optimized for the wrong feature up front (that decision is entirely on me).

So where are we now? I think it's possible to restore the the axis alignment, and that we should work towards that. AFAIK @mattpap has concrete ideas about a grid-span approach. But I don't think we should wait months to release if that's how long it takes with part time resourcing. Or in other words that ultimately, if it really comes down to it, the user pain points that have actually come up in real experience are acute enough that fixing them would justify this one step back for now.

Would like others thoughts, though.

@birdsarah
Copy link
Member

I'm going to agree with you while disagreeing very strongly :-D

  1. You buried the lead, alignment is largely working! That was not clear from your comment or from responses to my issue. Check out this tortured version of the gridplot example which, aside from the left title looks great

screenshot from 2019-02-14 00-36-59

That's with stretch_both, but all sizing modes look good (except scale_both but not in a way that's unovercome-able)

I am guessing that the issue is alignment when a plot spans multiple columns of a grid. I am happy to agree that's a secondary consideration.


  1. When alignment is really important

2a) I have spent a lot of the last 6 months looking at grids of plots. If they all have, for example, the same x-axis but different quantities on the y-axis you want to be able to scan for patterns. I don't think that my experience is uncommon. I think that science-y use cases should hold equal weight to dashboard-y use cases. As I said earlier, the fact that grids work make me less concerned.

2b) In the equal weighting of dashboardy-to-sciency...to offer some counter examples, matplotlib and ggplot provide options for alignment, a quick search yielded these two tutorials covering how to get things lined up: https://www.python-course.eu/matplotlib_multiple_figures.php, https://www.andrewheiss.com/blog/2017/08/10/exploring-minards-1812-plot-with-ggplot2/.

  1. It seems dangerous to compare a rate of complaints about things that need to be fixed, with an absence of complaints about a thing that works

@birdsarah
Copy link
Member

@mattpap is that left-title-when-no-axis issue a known issue. Let me know if not and I'll open an issue.

@philippjfr
Copy link
Contributor

philippjfr commented Feb 21, 2019

So I decided to have a look at the performance issues myself and immediately got quite confused by the profiler output. According to this it looks like the sized utility is making a bunch of calls to the inline function which computes the size:

screen shot 2019-02-21 at 7 37 54 pm

But if I look at the sized utility function it should only ever run that anonymous function once. So unless I don't know how to read the profiler output this makes zero sense. Any idea what's going on there @mattpap?

Edit: I must be misunderstanding the output since I've confirmed they are all separate calls. Probably better for you to just ignore me since you have a much better understanding.

@mattpap
Copy link
Contributor Author

mattpap commented Feb 21, 2019

@philippjfr, in grids and nested layouts measures are taken in batches, and this is nicely visible in this profile. A single widget will have its measurement function called multiple times with different viewports, to optimize its size. Some of those measurements could be eliminated or at least cached (in future development). However, the major source of slowdown is nicely shown in violet color. This is CSS layout triggered by widgets' measurement function. Unfortunately I significantly underestimated how inefficient the approach is. But no worries, I'm hoping to have a PR fixing this tomorrow.

@mattpap
Copy link
Contributor Author

mattpap commented Mar 4, 2019

Avoid slider widgets cutting off the handle and text

@philippjfr, is this sill an issue? Slider now allocates more space to accommodate for the handle, so it should look better.

@philippjfr
Copy link
Contributor

It seems to be fine now, and one can always increase the margin if necessary.

@mattpap mattpap closed this as completed Mar 5, 2019
@mattpap mattpap moved this from In progress to Done in Focus: Layout Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

No branches or pull requests

4 participants