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

reproducible orca running image tests #3237

Closed
wants to merge 35 commits into from
Closed

Conversation

antoinerg
Copy link
Contributor

@antoinerg antoinerg commented Nov 12, 2018

This is a WIP proof of principle showcasing that Orca can yield reproducible output when running a software implementation of OpenGL. In this branch, test images generated on the CI are compared with baselines generated on my laptop with a tolerance of zero.

Because this is software OpenGL, a lot more CPU time is needed. On one core, running the test took roughly 20-30 minutes. By making use of 8 parallel containers, this time is cut down to ~5 minutes which is faster than the Jasmine tests we currently have.

To do:

  • Move the Dockerfile for "antoinerg/orca-reproducible:latest" into this repo and build the image on CircleCI
  • Reuse the nice code from @etpinard's PR Image tests using orca #2615
  • Replace bash scripts with node.js code
  • Update the dev commands npm run (test-image|baseline) and npm run docker and make sure they run in parallel
  • Orca should use the plotly.js in the build folder instead of using latest.
  • Remove images from test_images_diff that are exactly the same.
  • Add mapbox support
  • Add mathjax support

Accuracy issue:

@alexcjohnson
Copy link
Collaborator

Very nice work @antoinerg! Does the parallelism work locally, based on the number of cores on the local machine? 🙏

I see a step pushing artifacts, but I can't actually find them in the test results - are they there and I'm just not finding them?

There's a lot to look at in the image outputs, and we want to be extremely careful since it's updating every mock we have!

In the SVG mocks, the only substantial change I see is in the fonts: some have very different sizing and some don't seem to be recognized in orca at all. Here's onion skin halfway between the two:
screen shot 2018-11-13 at 9 35 14 am
I know fonts have always been a massive pain to get to match, but this seems farther off than is acceptable.

In WebGL the biggest thing I see is that all markers got substantially bigger (in both 2D and 3D). Onion skin again:
screen shot 2018-11-13 at 10 01 52 am
If I look at one of the mocks that puts GL2D and SVG side-by-side (for example gl2d_fill_trace_tozero_order), the old version seems more correct. Not really sure how to troubleshoot that though... some feature we're using for only marker sizing that behaves oddly in software OpenGL?

There's one issue in GL3D that seems to have been fixed here though! Some of our baselines don't correctly rotate tick labels. See for example gl3d_cone_rossler - on this branch and on my screen they're rotated, but in the master baseline they are not. Oddly enough some of the master baselines do have rotated labels, eg gl3d_chrisp-nan-1 which makes it even more puzzling, but if we can get the rest of these issues solved at least that one we can ignore 🎉

@antoinerg
Copy link
Contributor Author

antoinerg commented Nov 13, 2018

Very nice work @antoinerg!

Thank you @alexcjohnson !

Does the parallelism work locally, based on the number of cores on the local machine?

Although the script to do so is not here yet, yes it would work by running one container per core in parallel! FIY, running a bunch of Orcas in parallel in the same Docker container was not reliable in my experience. I will add a script to do so if we end up adopting Orca. Note that some gl3d mocks require quite a bit memory to render (maybe 1GB) so we may not want to run 8 at a time on a laptop.

I see a step pushing artifacts, but I can't actually find them in the test results - are they there and I'm just not finding them?

They are in there: https://circleci.com/gh/plotly/plotly.js/18671#artifacts/containers/0

In the SVG mocks, the only substantial change I see is in the fonts: some have very different sizing and some don't seem to be recognized in orca at all.

When I build the Docker image, I need to install all the fonts we would like to support. As of right now, I just copied the ones from Orca's Dockerfile: https://github.com/plotly/orca/blob/a0e7314a784802b4824b359aeff891e0d40be184/deployment/Dockerfile#L51-L77
Hopefully, I am just missing a few which explains the difference. Next step would be to check the OS and Electron's font rendering options.

If I look at one of the mocks that puts GL2D and SVG side-by-side (for example gl2d_fill_trace_tozero_order), the old version seems more correct. Not really sure how to troubleshoot that though... some feature we're using for only marker sizing that behaves oddly in software OpenGL?

Nice find 🔍 ! It's true that the markers are rendered bigger than they should as clearly shown in gl2d_fill_trace_tozero_order. I think you're right: the browser makes an OpenGL call that is rendered oddly by this software implementation of OpenGL. Either we're using an old badly supported GL call (like we do here gl-vis/gl-error3d#5) or the software renderer needs to be updated/improved.

Thanks for the review! I guess the interim conclusion is that the current solution is exactly reproducible but not yet accurate 🤔

@antoinerg
Copy link
Contributor Author

About fonts, the Droid fonts do not get downloaded when building Orca's Dockerfile so I opened an issue over there plotly/orca#146. When building orca-reproducible I will instead copy the fonts we had in the old image-server.

@antoinerg
Copy link
Contributor Author

antoinerg commented Nov 14, 2018

I am hopeful we can fix the issue with fonts or at least get close enough. I think I'm only missing "Courier New" as you can see in this onion skin halfway between the two:
fonts_improvement

The updated baselines are in commit 9779c46!

@antoinerg
Copy link
Contributor Author

@alexcjohnson

I found out the reason why the markers are rendered bigger in WebGL!

  • The size of markers in regl-scatter2d is set by this line:
gl_PointSize = 2. * size * pixelRatio;
  • The pixelRatio above is forced to be 2.5 in Orca on this line instead of its previous value of 2:
  plotGlPixelRatio: 2.5,

By forcing it to be 2, the markers are now correctly sized:
gl2d_fill_trace_tozero_order

I will push a new set of baselines with this change!

@alexcjohnson
Copy link
Collaborator

gl_PointSize = 2. * size * pixelRatio;

Oh wow, that's a problem, nice find! But the fix is not to force orca to use the default value. pixelRatio should not affect anything about the plot except the quality of antialiasing. We should be able to make a regular in-browser plot using plotGlPixelRatio: 5 or something and, other than potentially running out of memory, everything should look the same just smoother.

So was pixelRatio already included in size or something? Should we just change that line to gl_PointSize = 4. * size; or something like that?

@antoinerg
Copy link
Contributor Author

antoinerg commented Nov 15, 2018

So was pixelRatio already included in size or something?

No I don't think it's already included in variable size

Should we just change that line to gl_PointSize = 4. * size; or something like that?

I am under the impression that we've always been rendering plots with pixelRatio=2 so yes I would be tempted to change that line to gl_PointSize = 4. * size; as well as the few other lines that have pixelRatio in them.

We should be able to make a regular in-browser plot using plotGlPixelRatio: 5 or something and, other than potentially running out of memory, everything should look the same just smoother.

Ok thanks for the explanation! I will test this out and open an issue if it doesn't behave as such.

But the fix is not to force orca to use the default value.

I did even worse in my last commit: I forced the value to be 2 in plotly.js itself. This was just for testing however. We should fix the aforementioned issue! At the very least this PR will have allow us to uncover a bug 🪲 🔍!

Thank you @alexcjohnson

@archmoj
Copy link
Contributor

archmoj commented Apr 10, 2019

gl_PointSize = 2. * size * pixelRatio;

Oh wow, that's a problem, nice find! But the fix is not to force orca to use the default value. pixelRatio should not affect anything about the plot except the quality of antialiasing. We should be able to make a regular in-browser plot using plotGlPixelRatio: 5 or something and, other than potentially running out of memory, everything should look the same just smoother.

So was pixelRatio already included in size or something? Should we just change that line to gl_PointSize = 4. * size; or something like that?

@etpinard That's exactly what I still think we should do here: gl-vis/regl-scatter2d#20.

@archmoj
Copy link
Contributor

archmoj commented Aug 21, 2020

@antoinerg regarding gl2d marker sizes are fixed in #5093. FYI - I simply checked the item in the PR description.

@archmoj
Copy link
Contributor

archmoj commented Jun 25, 2021

Closing now that #5724 merged.

@archmoj archmoj closed this Jun 25, 2021
@archmoj archmoj deleted the orca-reproducible branch June 25, 2021 14:05
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.

4 participants