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

Integration of orjson for (much) faster JSON encoding and decoding #358

Merged
merged 35 commits into from Mar 12, 2022

Conversation

munrojm
Copy link
Contributor

@munrojm munrojm commented Mar 8, 2022

Summary

This PR switches monty from using the standard JSON encode and decode functions, to using the equivalent in the orjson package (see https://github.com/ijl/orjson). This greatly speeds up both MontyDecoder and MontyEncoder.

One important note is that I elected to keep the fact that MontyDecoder/Encoder subclasses json.JSONDecoder and json.JSONEncoder respectively. I think this effectively communicates that both custom classes can still be used with json.dumps/loads as the passed cls argument.

I will also point out that this change was motivated by slowness issues in the new Materials Project API from decoding large objects.

@shyuep
Copy link
Contributor

shyuep commented Mar 8, 2022

I don't have a problem with this, but it should be made an optional alternative rather than a requirement. In other words, the code should check if orjson is installed and use that if it is, and if not, fall back to standard json.

@munrojm
Copy link
Contributor Author

munrojm commented Mar 8, 2022

I don't have a problem with this, but it should be made an optional alternative rather than a requirement. In other words, the code should check if orjson is installed and use that if it is, and if not, fall back to standard json.

Sounds good, I will make that change.

@munrojm munrojm changed the title Integration of orjson for (much) faster JSON encoding and decoding [WIP] Integration of orjson for (much) faster JSON encoding and decoding Mar 8, 2022
@munrojm
Copy link
Contributor Author

munrojm commented Mar 8, 2022

I want to write some more tests before it is merged. I've marked it as WIP and will indicate again when it is ready.

@munrojm munrojm changed the title [WIP] Integration of orjson for (much) faster JSON encoding and decoding Integration of orjson for (much) faster JSON encoding and decoding Mar 11, 2022
@munrojm
Copy link
Contributor Author

munrojm commented Mar 11, 2022

Re-writing a new encode function with recursive pre-processing before passing to orjson turned out to be slower than the existing solution. It is better to use the native orjson API with the MontyEncoder.default method for encoding, which I have added to the docstring. I have kept the optional orjson decoding in, as it is trivial to incorporate without any issues.

Additionally, I have fixed some of the CI issues with pre-commit and pylint. This should be good to go now.

@shyuep shyuep merged commit e4c705b into materialsvirtuallab:master Mar 12, 2022
11 checks passed
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.

None yet

2 participants