QCFractal icon indicating copy to clipboard operation
QCFractal copied to clipboard

[WIP] Conceptual split of *_json into *_json, *_dict methods on Collections

Open dotsdl opened this issue 3 years ago • 3 comments

Description

Addresses #591

Pydantic models support direct JSON serialization; we take advantage of this here to support use of e.g. sets in a model.

The to_json and from_json methods also technically dealt with dicts, not JSON strings; this is a distinction that matters since not all dicts are valid JSON constructs. We resolve that distinction here.

To do:

  1. Need explicit roundtripping tests for *_json and *_dict methods yet.

Changelog description

Collections feature to_dict,from_dict and to_json,from_json methods that are conceptually clearer, roundtrip with model fidelity.

Status

  • [ ] Code base linted
  • [ ] Ready to go

dotsdl avatar Nov 04 '20 18:11 dotsdl

Codecov Report

Merging #643 into master will increase coverage by 0.21%. The diff coverage is 40.00%.

codecov[bot] avatar Nov 04 '20 18:11 codecov[bot]

Perhaps its better to mirror the raw pydantic .json() and parse_file/raw methods here? Did OpenFF settle on a specific pattern?

Thank you for the review @dgasmith!

I would favor what I consider the clearer to_* and from_* semantics that are used in e.g. pandas, openforcefield, xarray, etc. That appears to be a pattern pretty widely used in the scientific Python stack where data formats abound in variety, whereas pydantic appears more web-services oriented in which most things are represented in json or json-compatible serialization formats (my impression).

dotsdl avatar Dec 29 '20 18:12 dotsdl

Good to have confirmation, this is my base stance as well. I often cut out pydantic functionality these days from being easily discoverable:

    def __dir__(self) -> List[str]:
        # Hide many Pydantic base functions to skip confusion in notebooks
        hidden_funcs = {
            "json",
            "parse_obj",
            "parse_raw",
            "parse_file",
            "from_orm",
            "construct",
            "schema_json",
            "schema",
            "validate",
            "update_forward_refs",
            "to_string",
        }
        return sorted(set(dir(super())) - hidden_funcs)

Note there is also movement to have pydantic model methods moved to an internal namespace and since these objects are metaclasses, the ability to simply turn off the base functionality entirely. This gives a more composable surfaces and allows you to ditch the more webserver-esq abstractions.

dgasmith avatar Dec 29 '20 19:12 dgasmith

I think this was largely addressed in the previous version, and hopefully addressed in v0.50.

Likely to become even more moot with pydantic v2 (for example, PR #736)

bennybp avatar Sep 14 '23 15:09 bennybp