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

Switch from monty to orjson for serialization #464

Merged
merged 5 commits into from Jul 9, 2021

Conversation

munrojm
Copy link
Member

@munrojm munrojm commented Jul 9, 2021

This PR switches to using orjson for custom serialization in ReadOnlyResource.

@codecov
Copy link

codecov bot commented Jul 9, 2021

Codecov Report

Merging #464 (5a67d79) into main (1348a1e) will decrease coverage by 0.16%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #464      +/-   ##
==========================================
- Coverage   88.42%   88.25%   -0.17%     
==========================================
  Files          40       40              
  Lines        2626     2631       +5     
==========================================
  Hits         2322     2322              
- Misses        304      309       +5     
Impacted Files Coverage Δ
src/maggma/api/resource/read_resource.py 98.64% <100.00%> (ø)
src/maggma/api/utils.py 97.05% <100.00%> (+0.23%) ⬆️
src/maggma/stores/mongolike.py 82.19% <0.00%> (-1.90%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8122d6b...5a67d79. Read the comment docs.

@munrojm munrojm requested a review from shyamd July 9, 2021 01:20
@lgtm-com
Copy link

lgtm-com bot commented Jul 9, 2021

This pull request introduces 1 alert when merging 678d3bb into 8122d6b - view on LGTM.com

new alerts:

  • 1 for Unused import

@munrojm munrojm changed the title Switch from monty or orsjon for serialization Switch from monty or orjon for serialization Jul 9, 2021
@munrojm munrojm changed the title Switch from monty or orjon for serialization Switch from monty or orjson for serialization Jul 9, 2021
@shyamd
Copy link
Contributor

shyamd commented Jul 9, 2021

Why orjson?

It seems silly, but can you add a test for custom_serialization. Maybe rename in object_id_serilaization_helper to make it clearer what it is doing?

Also, should the flag in ReadOnlyResource be called something different like disable_validation?

@munrojm
Copy link
Member Author

munrojm commented Jul 9, 2021

orjson is extremely fast and can handle datetime object natively while allowing for extensions for things like bson types.

I have renamed things as you have suggested and added the test.

@shyamd shyamd changed the title Switch from monty or orjson for serialization Switch from monty to orjson for serialization Jul 9, 2021
@shyamd shyamd merged commit f6cce12 into materialsproject:main Jul 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants