fix: verify registered models against the stored spec on deserialization - #1877
Open
rahul188 wants to merge 2 commits into
Open
fix: verify registered models against the stored spec on deserialization#1877rahul188 wants to merge 2 commits into
rahul188 wants to merge 2 commits into
Conversation
SignalSchema._deserialize_custom_type returned any registered model whose name and version matched, so two generated models sharing a name (e.g. both created by explode() on a column named json) shadowed each other across datasets: whichever registered first was used to read every same-named dataset, producing KeyError, ValidationError, or silently misread values. A registered model now resolves a custom type only when its field spec, including nested custom types, equals the spec stored in the dataset schema. On mismatch the model is recreated from the stored spec under a spec-fingerprinted alias, so the registered model keeps its name and repeated reads reuse the recreated class. Fixes datachain-ai#1872
Author
|
Hi 👋 Gentle nudge on this PR — it's mergeable and CI is passing, just waiting on a review. Would really appreciate a look whenever you have bandwidth, and I'm glad to make any changes you'd like. Thanks for your time and for maintaining this project! 🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
SignalSchema._deserialize_custom_typeresolves custom types byname@versionalone:Since
ModelStoreis process-global and all generated models are@v1, two datasets whose inferred models share a name (bothexplode()d from a column namedjson, two parquet files with anitemslist-of-struct column, …) shadow each other: whichever model registers first is used to read every same-named dataset for the rest of the process. Depending on how the shapes overlap that surfaces as aKeyError(missing DB column), a PydanticValidationError, or a silent misread. Repro from #1872:Fix
Following the approach sketched in the issue:
_serialize_custom_modelmachinery and comparesfieldsper custom type, so a divergence in a nested model (identical top-level field strings, differentInner@v1shapes) is caught too.{name}_{fingerprint[:10]}, the same shapeto_partialuses for partials) instead of clobbering the registered model's name inModelStore. Repeated reads of the same dataset hit the alias cache and reuse the recreated class.The fingerprint covers the transitive closure of the type's spec (fields, bases, hidden fields of every reachable custom type), so two colliding specs that differ only in a nested type or base still get distinct aliases.
Testing
test_deserialize_same_name_different_shape_not_shadowed— the cross-dataset collision, plus re-read identity (same class object on repeated deserialization of both specs)test_deserialize_nested_shape_divergence_not_shadowed— identical top-level specs, diverging nested modeltest_deserialize_prefers_registered_model_when_spec_matches— round-trip still returns the registered class itself, not a recreationtest_explode_same_column_name_across_datasets— the issue's end-to-end repro (explode→save→read_datasetboth ways)All four were run against unmodified
mainfirst: the three collision tests fail there (the e2e test withKeyError: 'json_expl__a'), and the registered-model test passes on both, confirming no regression of round-trip identity. Fulltests/unit/lib/test_signal_schema.py(120) andtests/unit/lib/test_datachain.py(349) pass;ruff check,ruff format --check, andmypyare clean on the changed files.Fixes #1872