🚸 Broaden name deduplication in HasType to search all contexts if no type is passed - #3850
🚸 Broaden name deduplication in HasType to search all contexts if no type is passed#3850ishitajain9717 wants to merge 11 commits into
HasType to search all contexts if no type is passed#3850Conversation
|
Great!
Can you condition this on |
|
Hi, |
69224bd to
1b60822
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3850 +/- ##
==========================================
- Coverage 91.55% 86.98% -4.58%
==========================================
Files 88 88
Lines 15747 15845 +98
==========================================
- Hits 14417 13782 -635
- Misses 1330 2063 +733
🚀 New features to boost your workflow:
|
|
Deployment URL: https://7dfd74f7.lamindb.pages.dev |
type= is not passedtype= is not passed
type= is not passedtype is passed
type is passedHasType to search all contexts if no type is passed
HasType to search all contexts if no type is passedHasType to search all contexts if no type is passed
|
|
||
| def suggest_records_with_similar_names( | ||
| record: SQLRecord, name_field: str, kwargs | ||
| record: SQLRecord, name_field: str, kwargs, type_val=UNSET |
There was a problem hiding this comment.
Why do we need to introduce type_val? The kwargs have type in them and you can detect whether it matches UNSET. Why is that not possible?
| # 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: |
There was a problem hiding this comment.
I would have expected something like this:
type = kwargs.get("type", UNSET)
if type is UNSET:
subset = record.__class__.filter()
elif type is None:
subset = record.__class__.filter(type__isnull=True)
else:
subset = record.__class__.filter(type=kwargs["type"])There was a problem hiding this comment.
There is a question whether we should use kwargs["type"] instead of kwargs.get() because we might now actually have a contract that guarantees the presence of type since it would never be popped? 🤔
| from .collection import Collection | ||
| from .transform import Transform | ||
|
|
||
| # ensure "type" is always present in kwargs for HasType models so that |
There was a problem hiding this comment.
Do you understand why this is necessary? My assumption is that every HasType model populates type=UNSET or whatever the user passes. How can it be that there are cases where you need this defensive line?
If this is AI generated: Can you remove it and see if the tests pass?
There was a problem hiding this comment.
I think the injection is needed for Project and Reference. Unlike Record, ULabel, Feature, and Schema, which explicitly pop and re-inject type (defaulting to UNSET) in their own constructors, Project and Reference have init methods that pass args, kwargs straight through to super(). So if a user calls Project(name="foo") without type, the key is simply absent from kwargs when BaseSQLRecord receives it.
Please suggest if it makes sense
There was a problem hiding this comment.
It's great that you found this!
Project and Reference have init methods that pass args, kwargs straight through to super().
Hmmm. If that's what's happening then we should see type: UNSET in the kwargs, dict, right? I think your reasoning might be incorrect here.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Maybe I missed something in the previous PR: I understood that we changed type = None to type = UNSET in the previous PR. This isn't an internal but a user-facing change. If it wasn't user-facing, how would a user learn about the difference of passing type = None versus not passing type?
I'm convinced that every HasType registry needs to expose type = UNSET to the user also in the docs; and of course, if the user doesn't pass type that needs to get passed into the downstream calling cascade. That should IMO be automatic since UNSET is the default value of the constructor.
| 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: |
There was a problem hiding this comment.
The logic here is incorrect: if "type" is not in kwargs the .get() statement returns UNSET and one enters kwargs.pop() even though there is no key "type"!
IMO the same rationale that would make https://github.com/laminlabs/lamindb/pull/3850/changes#r3782589607 easy would make this easy: replacing kwargs.get() with kwargs["type"] is UNSET.
There was a problem hiding this comment.
I agree, this is logically imprecise. I am correcting it in upcoming changes
| def test_name_lookup(): | ||
| 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 |
There was a problem hiding this comment.
Great idea to add the comments here! 🙏
| # same type → returns existing typed record | ||
| label2 = ln.Record(name="label 1", type=my_type) | ||
| assert label2 == label1 | ||
| # no type passed, only typed record exists → fallback returns the typed one |
There was a problem hiding this comment.
Great! This is the behavior change now.
| @@ -690,14 +690,17 @@ def test_feature_manager_raise_not_validated_values(): | |||
| def test_name_lookup(): | |||
There was a problem hiding this comment.
Aren't we now missing a test here where the user explicitly passes type=None?
|
Thanks, @ishitajain9717! This is now much better designed than yesterday! 🙏 Can you mark the comments that you resolved as resolved? Now only very small issues are left, for which I made new comments. |
| type = kwargs["type"] | ||
| if type is UNSET: | ||
| if type is None: | ||
| # explicit type=None → user wants a new root-level record, skip dedup |
There was a problem hiding this comment.
This is wrong, isn't it? If we pass type=None we need to deduplicate on the root level.
# explicit type=None → search root-level (type IS NULL)
subset = record.__class__.filter(type__isnull=True)
There was a problem hiding this comment.
I realised it immediately, and I was just correcting it 😅. I was actually reviewing changes
| label_new = ln.Record(name="label 1", type=None) | ||
| assert label_new != label1 | ||
| assert label_new._state.adding # not yet saved, truly a new record | ||
| # explicit type=None, even if a root-level record exists → still skips dedup |
Add fallback to search typed objects when root-level deduplication finds nothing. If no
typeis passed, deduplication first searches root-level objects as before. If nothing is found there, it falls back to searching across all types — preventing silent duplicate creation when a same-named objects exists anywhere in the hierarchy.Also adds a warning when
type=Noneis explicitly passed for non-type objects, since root-level creation should be intentional and rare.