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

JSON encoding engine instrumentation #3012

Closed
wants to merge 18 commits into from

Conversation

jonmmease
Copy link
Contributor

This PR should not be merged!

This PR is instrumented for testing the new JSON encoding engine being introduced in #2955 (comment). Idea from @nicolaskruchten in #2955 (comment).

With this branch, every call to to_plotly_json performs JSON encoding with all three engines, then decodes the result and checks that identical dictionaries are produced. When they are not identical, and exception is raised.

Locally, nearly all of the documentation examples work. The two exceptions are imshow and ml-tsne-umap-projections. Both of these fail due to difference in how the orjson encoding handles floating-point numbers with precision less than 64 bits.

When encoding a float32 numpy array, the legacy and json encoders will convert this to a list of regular Python 64-bit floats before encoding them. This results extra digits in the JSON representation. In contrast, the orjson encoder will only write out the number of digits corresponding to the original 32-bit values.

@jonmmease jonmmease marked this pull request as draft January 8, 2021 21:02
@nicolaskruchten
Copy link
Contributor

awesome! To get a bit more mileage out of our extant tests which don't call fig.show() maybe add a JSON call to the end of px._core.make_figure() or something? Maybe consider adding it to all the update_* methods so that we check a bunch of intermediate representations as well? Super-slow, I know, but doing it just a couple of times would do ;)

@jonmmease
Copy link
Contributor Author

Yeah, those are good ideas. I'll add that sometime over the next couple of days

@jonmmease
Copy link
Contributor Author

@nicolaskruchten, I added the extra instrumentation points in be81b16 and everything looks the same as far as I can tell. See what you think!

@nicolaskruchten
Copy link
Contributor

OK, are you just running these locally or...? CI is failing because orjson isn't installed so I assume your goal wasn't to get CI to go green here, right?

@jonmmease
Copy link
Contributor Author

Yeah, I was running locally. And on CI I was mostly looking at the docs. I had the orjson stuff working on CI, but maybe that's in the other PR. I'll take a look.

@jonmmease
Copy link
Contributor Author

Merged in #3022, and a couple CI fixes and now all existing tests are passing with the instrumented version.

@archmoj archmoj deleted the orjson_encoding_instrumentation branch November 23, 2021 23:34
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.

2 participants