-
Notifications
You must be signed in to change notification settings - Fork 36
🚸 Broaden name deduplication in HasType to search all contexts if no type is passed
#3850
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 9 commits
2c0725c
784951d
e8745e5
1b60822
4f7a15c
c92a784
bc43b13
b917409
03d36ca
6a01541
9033d3a
b5a25b8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -451,7 +451,7 @@ def init_self_from_db( | |
|
|
||
| def update_attributes(record: SQLRecord, attributes: dict[str, str]): | ||
| for key, value in attributes.items(): | ||
| if getattr(record, key) != value and value is not None: | ||
| if value is not None and value is not UNSET and getattr(record, key) != value: | ||
| if key not in {"uid", "_dtype_str", "otype", "hash"}: | ||
| logger.warning(f"updated {key} from {getattr(record, key)} to {value}") | ||
| setattr(record, key, value) | ||
|
|
@@ -624,10 +624,18 @@ def suggest_records_with_similar_names( | |
| # the below needs to be .first() because there might be multiple records with the same | ||
| # name field in case the record is versioned (e.g. for Transform key) | ||
| if isinstance(record, HasType): | ||
| if kwargs.get("type", None) is None: | ||
| # "type" is always present in kwargs at this point (Solution A contract) | ||
| type = kwargs["type"] | ||
| if type is UNSET: | ||
| # user passed nothing → search all type contexts | ||
| # catches typed records with same name, fixes silent dup bug | ||
| subset = record.__class__.filter() | ||
| elif type is None: | ||
| # explicit type=None → search root-level (type IS NULL) | ||
| subset = record.__class__.filter(type__isnull=True) | ||
| else: | ||
| subset = record.__class__.filter(type=kwargs["type"]) | ||
| # specific type → search within that type context only | ||
| subset = record.__class__.filter(type=type) | ||
| else: | ||
| subset = record.__class__ | ||
| exact_match = subset.filter(**{name_field: kwargs[name_field]}).first() | ||
|
|
@@ -1162,10 +1170,6 @@ class Meta: | |
|
|
||
| def __init__(self, *args, **kwargs): | ||
| skip_validation = kwargs.pop("_skip_validation", False) | ||
| # strip sentinel before validate_fields and Django's Model.__init__ see it | ||
| # `is` never calls __eq__, so FeaturePredicate objects are safe | ||
| if isinstance(self, HasType) and kwargs.get("type", UNSET) is UNSET: | ||
| kwargs.pop("type", None) | ||
| if not args: | ||
|
|
||
| def resolve_fk_or_id(field_name: str) -> bool: | ||
|
|
@@ -1214,13 +1218,21 @@ def resolve_fk_or_id(field_name: str) -> bool: | |
| # the current one), so the record is created on that branch. | ||
| kwargs["created_on"] = kwargs["branch"] | ||
| if skip_validation: | ||
| # strip UNSET just before Django sees kwargs — FK descriptors reject non-model values | ||
| if isinstance(self, HasType) and kwargs.get("type", UNSET) is UNSET: | ||
|
ishitajain9717 marked this conversation as resolved.
Outdated
|
||
| kwargs.pop("type", None) | ||
| super().__init__(**kwargs) | ||
| else: | ||
| from ..core._settings import settings | ||
| from .can_curate import CanCurate | ||
| from .collection import Collection | ||
| from .transform import Transform | ||
|
|
||
| # ensure "type" is always present in kwargs for HasType models so that | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you understand why this is necessary? My assumption is that every HasType model populates If this is AI generated: Can you remove it and see if the tests pass?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the injection is needed for
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's great that you found this!
Hmmm. If that's what's happening then we should see
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right that we should always see type: UNSET in kwargs — that's the contract. But the problem is that UNSET is an internal sentinel. So "type" only ends up in kwargs if the model's init explicitly injects it. Do you agree?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe I missed something in the previous PR: I understood that we changed I'm convinced that every
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree with that, this should be a user facing change, and should also be mentioned in the docs specifically. I would make the change now |
||
| # suggest_records_with_similar_names can use kwargs["type"] unconditionally | ||
| if isinstance(self, HasType) and "type" not in kwargs: | ||
| kwargs["type"] = UNSET | ||
|
|
||
| validate_fields(self, kwargs) | ||
|
|
||
| # do not search for names if an id is passed; this is important | ||
|
|
@@ -1270,6 +1282,9 @@ def resolve_fk_or_id(field_name: str) -> bool: | |
| # track original values after replacing with the existing record | ||
| self._populate_tracked_fields() | ||
| return None | ||
| # strip UNSET just before Django sees kwargs — FK descriptors reject non-model values | ||
| if isinstance(self, HasType) and kwargs.get("type", UNSET) is UNSET: | ||
| kwargs.pop("type", None) | ||
| super().__init__(**kwargs) | ||
| if isinstance(self, ValidateFields): | ||
| # this will trigger validation against django validators | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -690,14 +690,17 @@ def test_feature_manager_raise_not_validated_values(): | |
| def test_name_lookup(): | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aren't we now missing a test here where the user explicitly passes |
||
| my_type = ln.Record(name="MyType", is_type=True).save() | ||
| label1 = ln.Record(name="label 1", type=my_type).save() | ||
| # same type → returns existing typed record | ||
|
ishitajain9717 marked this conversation as resolved.
|
||
| label2 = ln.Record(name="label 1", type=my_type) | ||
| assert label2 == label1 | ||
| # no type passed, only typed record exists → fallback returns the typed one | ||
|
ishitajain9717 marked this conversation as resolved.
|
||
| label2 = ln.Record(name="label 1") | ||
| assert label2 != label1 | ||
| label2.save() | ||
| label3 = ln.Record(name="label 1") | ||
| assert label3 == label2 | ||
| label2.delete(permanent=True) | ||
| assert label2 == label1 | ||
| # root-level record exists → no-type search finds root first before typed | ||
| root_label = ln.Record(name="root label 1").save() | ||
| label3 = ln.Record(name="root label 1") | ||
| assert label3 == root_label | ||
| root_label.delete(permanent=True) | ||
| label1.delete(permanent=True) | ||
| my_type.delete(permanent=True) | ||
|
|
||
|
|
@@ -1194,7 +1197,7 @@ def test_record_features_add_remove_values(): | |
|
|
||
| # test passing ISO-format date string for date | ||
|
|
||
| test_record2 = ln.Record(name="test_record").save() | ||
| test_record2 = ln.Record(name="test_record_2").save() | ||
| # we could also test different ways of formatting but don't yet do that | ||
| # in to_dataframe() we enforce ISO format already | ||
| feature_date = ln.Feature.get(name="feature_date") | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have expected something like this:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a question whether we should use
kwargs["type"]instead ofkwargs.get()because we might now actually have a contract that guarantees the presence oftypesince it would never be popped? 🤔