Add save and load functionality to MSONable #670
Conversation
WalkthroughThe recent changes involve significant enhancements to JSON serialization capabilities in the Changes
Recent Review DetailsConfiguration used: .coderabbit.yaml Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Out of diff range and nitpick comments (1)
tests/test_json.py (1)
394-454: Thetest_save_loadmethod appropriately tests the new serialization functionalities. Consider adding more detailed comments within the test to explain the scenarios, especially the expected failures, to enhance readability and maintainability.
| from typing import Any, Dict | ||
| from uuid import UUID, uuid4 |
There was a problem hiding this comment.
Consider consolidating imports from the same module to maintain code cleanliness.
- from typing import Any, Dict
- from uuid import UUID, uuid4
+ from typing import Any, Dict
+ from uuid import UUID, uuid4Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| from typing import Any, Dict | |
| from uuid import UUID, uuid4 | |
| from typing import Any, Dict | |
| from uuid import UUID, uuid4 |
|
@matthewcarbone: Thanks for revisiting this! And don't worry about the breakage from last time; the tests passed in monty, so you did everything you were supposed to do. You can't be expected to control for things in other repos, but I appreciate your flexibility on this. This looks pretty safe to me! I did not find any issues with it breaking other repos this time around. 👍 For instance, see: |
|
@Andrew-S-Rosen Much appreciated, thanks for checking these things 👍
Thank you, but still I think I should've been a bit more careful last time. I simply didn't think about the case where people overwrite Anyways, I'll fix the linting, then we can proceed whenever/if you and @shyuep want to! |
|
@Andrew-S-Rosen it actually looks like the linting error is from a different file: To be completely honest, I'm not that familiar with mypy, but if you'd like I can try to debug this. |
|
Sounds good. I'll stay in a holding pattern for now. 👍 |
| enum_values=enum_values, | ||
| recursive_msonable=recursive_msonable, | ||
| ) | ||
| jsanitize(i, strict=strict, allow_bson=allow_bson, enum_values=enum_values) |
There was a problem hiding this comment.
I should have reviewed this better, but the recursive_msonable kwarg was removed in a few places. I'll make a patch.
In materialsvirtuallab#670, the `recursive_msonable` keyword argument was removed from the internal `jsanitize` calls. I have patched it back.
|
Well that was fast, hoping for no bugs this time 😁 |
|
Pinging @utf, @gpetretto, and @davidwaroquiers since this may be something that impacts jobflow/jobflow-remote development. |
This PR (re-)adds feature #653.
From #654:
One key difference from the (buggy) #654 is that the decoder was left unchanged. The encoder simply has a fallback when
saveis called, but otherwise behaves identically when the otherMSONablemethods are invoked.When
saveis called and an un-MSONableobject is encountered, it is swapped out for a unique identifier of the form:{"@object_reference" : unique_id}The object is then stored in a dictionary and pickled next to the saved json.
Any time
"@object_reference"is encountered during rehydration, it is replaced by the pickled result. I look for these object references by recursively searching the loaded json file (lists and dictionaries only) and replacing these references with their objects before passing tofrom_dict.Important
When I tried to implement this before, it came with some breaking bugs due to the fact that MSONable is used in other repositories in ways that weren't completely covered by the testing suite here. Here's an example. I was extra careful in this case to not touch any of the default behaviors, and to simply provide fallbacks where exceptions would otherwise be thrown, but that's never a guarantee!
@Andrew-S-Rosen let me know what you think!