Catch hanging figure payloads in parse step by etpinard · Pull Request #55 · plotly/orca

etpinard · 2018-02-13T22:16:08Z

A proof of concept for https://github.com/plotly/streambed/issues/9865#issuecomment-365322166 for more info.

- to catch figure payload that can cause image-exporter to hang

etpinard · 2018-02-19T16:56:23Z

@scjody is this PR still worth merging?

scjody · 2018-02-19T17:01:07Z

We need something like this before we can close https://github.com/plotly/streambed/issues/9865. Right now image requests are being delayed unnecessarily due to server restarts, and we need to run 12 servers at all times (whereas we should be able to scale down to 3 off-peak).

I think it's worth waiting until you've had the chance to analyze some more server-breaking plots though, since this work is based on only one. I expect to have time to collect some more next week by modifying the load_test tool in https://github.com/plotly/plotly.js/tree/imageserver-testing-hacks to send requests sequentially and check the health of the imageserver immediately afterwards. (Note that it needs to be run against one server, not against a cluster.)

etpinard · 2018-02-19T17:02:53Z

Thanks for the info @scjody 👌

etpinard · 2018-03-08T03:20:53Z

@scjody your recommendations from https://github.com/plotly/streambed/issues/9865#issuecomment-370514774 have now been addressed.

Ready to review.

scjody

This looks good to me. I feel like someone with more plotly.js experience like @alexcjohnson should review it too.

💃

alexcjohnson · 2018-03-08T18:39:37Z

src/component/plotly-graph/parse.js

+function maxPtsPerTraceType (type) {
+  switch (type) {
+    case 'scattergl':
+    case 'splom':


Not that we really have this info yet, but I'd have thought splom would need to be limited quite a bit lower, perhaps the same as parcoords provisionally but potentially even lower.

alexcjohnson · 2018-03-08T18:39:55Z

src/component/plotly-graph/parse.js

+function findMaxArrayLength (cont) {
+  const lengths = Object.keys(cont)
+    .filter(k => Array.isArray(cont[k]))
+    .map(k => cont[k].length)


Oh right. We can get a better estimate for those. Thanks for noticing 👍

alexcjohnson · 2018-03-08T18:42:15Z

src/component/plotly-graph/parse.js

+    case 'histogram2dcontour':
+    case 'box':
+    case 'violin':
+      return 1e5


Can these really only handle 1e5? They're not actually displaying every point, I'd have thought we could bump up to 1e6 perhaps.

I was thinking of box and violin for (box)points turned on. We could make a special case for those I guess.

Ah good call - most of the time outliers are a tiny fraction of total points, but not always... and certainly not if ...points='all'

alexcjohnson · 2018-03-08T18:45:10Z

src/component/plotly-graph/parse.js

+  if (Array.isArray(trace.dimensions)) {
+    return trace.dimensions
+      .map(findMaxArrayLength)
+      .reduce((a, v) => a + v)


Nicely done

alexcjohnson · 2018-03-08T18:50:27Z

src/component/plotly-graph/parse.js

+    // cap the number of point per trace type
+    if (numberOfPtsPerTraceType[type] > maxPtsPerTraceType(type)) {
+      return true
+    }


I'm imagining someone trying to hang our servers, sending in one trace of each type, each just under our limit... do we want to do something like:

budget += estimateDataLength(trace) / maxPtsPerTraceType(type); if(budget > 1) return true;

alexcjohnson · 2018-03-08T18:55:47Z

src/component/plotly-graph/parse.js

+    .filter(k => Array.isArray(cont[k]))
+    .map(k => cont[k].length)
+
+  return Math.max(...lengths)


Better fall back to 0 if there are no arrays - otherwise the user could start with an empty trace (Math.max() = -Infinity) and every trace after that would be ignored.

- dive into 2d arrays to estimate their lengths - bump box, violin, histogram and histogram2d max pt threshold to 1e6, and add special case for box and violin with sample points displayed - use budget so that multiple traces close to threshold won't hang exporter - make traces w/o array don't mess up count

etpinard · 2018-03-09T04:35:00Z

@alexcjohnson thanks for the feedback! I think all your comments have been addressed in 884afdf

alexcjohnson · 2018-03-09T14:00:04Z

src/component/plotly-graph/parse.js


-  return Math.max(...lengths)
+  const lengths = arrays.map(arr => {
+    const innerArrays = arr.filter(Array.isArray)


isn't this going to be super slow for big 1D arrays? Most if not all of our modules that support 1D or 2D differentiate just based on the first element.

good call -> done in 851b803

alexcjohnson · 2018-03-09T14:04:11Z

src/component/plotly-graph/parse.js

+    if (
+      type === 'box' &&
+      trace.boxpoints === 'all' &&
+      len > 5e4


In principle I guess the special cases should get included in the budget as well - otherwise you could include 199 x 5e4-point box traces...

Good idea -> 0372d82

alexcjohnson · 2018-03-09T14:05:39Z

src/component/plotly-graph/parse.js

+    }
+  })
+
+  return arrays.length ? Math.max(...lengths) : 0


🐄 not that it matters, but Math.max(0, ...lengths) works too

🤠 -> 851b803

alexcjohnson · 2018-03-09T17:00:35Z

Great, thanks! Still concerned about splom but we'll see once it exists 😄
💃

etpinard · 2018-03-09T17:03:17Z

Still concerned about splom but we'll see once it exists

I have a good feeling splom with handle 1e7 points in less than 15 seconds. Aim high 🚀

etpinard added 2 commits February 13, 2018 17:14

[proof of concept] add willFigureHang pass in plotly-graph parse

c4bbaea

- to catch figure payload that can cause image-exporter to hang

Merge branch 'master' into will-figure-hang

1eec3e3

etpinard added 4 commits March 7, 2018 21:39

Merge branch 'master' into will-figure-hang

86a46c5

add will-hang case (if # of traces > 200)

8097c03

fixup too-many-pts logic for parcoords and splom traces

cc362c1

add will-hang edge case (mesh3d + alphahull)

2916aee

scjody approved these changes Mar 8, 2018

View reviewed changes

alexcjohnson reviewed Mar 8, 2018

View reviewed changes

alexcjohnson reviewed Mar 9, 2018

View reviewed changes

etpinard added 2 commits March 9, 2018 11:55

only use first array item to check is array is 2d

851b803

bring 'edge' case max-pt checks into budget

0372d82

etpinard merged commit 948030a into master Mar 9, 2018

etpinard deleted the will-figure-hang branch March 9, 2018 17:03

etpinard mentioned this pull request Jun 26, 2018

Add 'table' safeMode specs #98

Merged

Uh oh!

Conversation

etpinard commented Feb 13, 2018

Uh oh!

etpinard commented Feb 19, 2018

Uh oh!

scjody commented Feb 19, 2018

Uh oh!

etpinard commented Feb 19, 2018

Uh oh!

etpinard commented Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scjody left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson Mar 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard commented Mar 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

etpinard Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcjohnson commented Mar 9, 2018

Uh oh!

etpinard commented Mar 9, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

etpinard commented Mar 8, 2018 •

edited

Loading

alexcjohnson Mar 8, 2018 •

edited

Loading

etpinard Mar 9, 2018 •

edited

Loading