Change Validations to be Generic #75
Conversation
| def validate_model_serialized_for_multiprocessing(cls, serialized_model: str) -> str: | ||
| """Validate a model serialized via Pydantic's .json() method, and return a list of JSON serialized issues. | ||
|
|
||
| This method exists because our validations are forked into parallel processes via | ||
| multiprocessing.ProcessPoolExecutor, and passing a model or validation results object can result in | ||
| idiosyncratic behavior and inscrutable errors due to interactions between pickling and pydantic objects. | ||
| """ | ||
| return SemanticManifestValidationResults.from_issues_sequence( | ||
| cls.validate_model(SemanticManifest.parse_raw(serialized_model)) | ||
| ).json() |
There was a problem hiding this comment.
If I remember correctly this sometimes but not always threw errors. Like I had persistent failures in CI but not locally or vice-versa when this popped up. It's possible pydantic fixed something, or that we cut out the bits that were causing problems (like the special input handing in our old WhereClauseConstraint objects), but it's also possible we just haven't stumbled in a bad configuration yet.
I think it's fine to remove it here, especially given the scope of this PR, but I wouldn't be surprised if we had to put it back. The best practice here is to pass serialized data across the process boundary rather than passing data objects and relying on pickling's best-effort serialization. There's a lot to be said for the key protocols having serialize() and deserialize() or similar methods, which would allow us to avoid the pickling as well.
There was a problem hiding this comment.
I got a reproducible failure case, and the issue actually was due to the way that Pydantic handles pickling if there is a dataclass embedded within a Pydantic object. Pickling of the definition Pydantic objects is fine, but pickling of SemanticManifestValidationResults is not, so I changed it to only JSON-serialize that class.
Resolves #20
Description
This PR changes the signature of the classes used for validation to be generic so that other implementations of the
SemanticManifestcan use the same framework with type-safety.Checklist
changie newto create a changelog entry