Enable recursive_msonable in jsanitize calls

[Andrew-S-Rosen]

Summary

In several places throughout maggma, the monty.json.jsanitize function is called to (as the name suggests) sanitize the data prior to dumping it in the Store. This is all fine and dandy, but MSONable objects are (seemingly?) converted to str. With recursive_msonable=True, all MSONable objects are converted to JSONable dictionary form via .as_dict().

@rkingsbury, @munrojm: I'm assuming there's something I'm missing here. Apologies because I don't have a MongoDB set up anymore so can't test directly, but surely we want MSONable objects to be represented in .as_dict() form in the data store, right? Is that happening some other way?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 81.52%. Comparing base (d83341e) to head (844b309).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #930   +/-   ##
=======================================
  Coverage   81.52%   81.52%           
=======================================
  Files          46       46           
  Lines        3940     3940           
=======================================
  Hits         3212     3212           
  Misses        728      728           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[rkingsbury]

This makes sense to me @Andrew-S-Rosen . I'm guessing it has not come up before because the docs passed into Store operations are usually already "dict-ified"?

For context I'm linking here the current source of jsanitize. If I understand correctly, when jsanitize is passed an object that it does not know what to do with (i.e., something not hard coded), then the behavior is defined by the following combinations:

1. strict=False, recursive_msonable=False (default): convert the object to string
2. strict=False, recursive_msonable=True: try to use the object's as_dict(). If that fails, convert to string
3. strict=True: try to use the object's as_dict(). If that fails, convert to string raise an error. The value of recursive_msonable doesn't matter here.

So yeah, I don't see any reason not to enable recursive_msonable @munrojm , do you see any issues with this? If not, I'll merge.

[Andrew-S-Rosen]

I'm guessing it has not come up before because the docs passed into Store operations are usually already "dict-ified"?

Ah, I see. I suppose that makes sense. I have always just passed it a dict and tried to let maggma take care of the rest.

For context I'm linking here the current source of jsanitize. If I understand correctly, when jsanitize is passed an object that it does not know what to do with (i.e., something not hard coded), then the behavior is defined by the following combinations:

@rkingsbury: I believe your understanding is almost correct. For strict = True, jsanitize will raise an AttributeError if no .as_dict() method is present on an object that is not otherwise JSONable. It is really only used for validation. You are correct about the other options. I should note that I had added in the recursive_msonable keyword argument ~2 years ago, so the code in maggma probably predated this.

So yeah, I don't see any reason not to enable recursive_msonable @munrojm , do you see any issues with this? If not, I'll merge.

Great. Would love to get @munrojm's confirmation. 👍

[rkingsbury]

@rkingsbury: I believe your understanding is almost correct. For strict = True, jsanitize will raise an AttributeError if no .as_dict() method is present on an object that is not otherwise JSONable. It is really only used for validation. You are correct about the other options. I should note that I had added in the recursive_msonable keyword argument ~2 years ago, so the code in maggma probably predated this.

Ah yes, I see now. Thanks for the clarification (I updated my previous post to strikethrough the incorrect info)

[orionarcher]

This would also be useful for my use case!

[munrojm]

Sorry, this is just getting on my radar again. No issues on my end, this all sounds good @rkingsbury.

[rkingsbury]

Thanks @munrojm . I'm trying to merge but GitHub is acting weird - have you ever seen this?

Anyway, feel free to merge if you are able to before I sort this out.

[Andrew-S-Rosen]

I have seen this happen. I usually just wait a few minutes and try again.

[rkingsbury]

I have seen this happen. I usually just wait a few minutes and try again.

After multiple tries over ~ 2 hours, it's still happening. Maybe someone else can try?

[Andrew-S-Rosen]

I don't have permissions, but perhaps you can just do a straight merge into main from the command line or Gitkraken or whatever since we know tests pass?

[rkingsbury]

Thanks @Andrew-S-Rosen !