Fix issue with feature views being detected as duplicated when imported #1905

achals · 2021-09-23T17:52:33Z

Signed-off-by: Achal Shah achals@gmail.com

What this PR does / why we need it:

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

none

Signed-off-by: Achal Shah <achals@gmail.com>

codecov-commenter · 2021-09-23T17:55:51Z

Codecov Report

Merging #1905 (c25ac21) into master (75f67ec) will increase coverage by 20.69%.
The diff coverage is 64.44%.

@@             Coverage Diff             @@
##           master    #1905       +/-   ##
===========================================
+ Coverage   62.30%   82.99%   +20.69%     
===========================================
  Files          96       96               
  Lines        7353     7357        +4     
===========================================
+ Hits         4581     6106     +1525     
+ Misses       2772     1251     -1521

Flag	Coverage Δ
integrationtests	`74.78% <62.22%> (?)`
unittests	`60.66% <62.22%> (-1.64%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
sdk/python/feast/repo_operations.py	`42.46% <0.00%> (-1.76%)`	⬇️
sdk/python/feast/feature_table.py	`45.27% <80.00%> (+1.21%)`	⬆️
sdk/python/feast/entity.py	`74.80% <100.00%> (+0.81%)`	⬆️
sdk/python/feast/feature_service.py	`76.62% <100.00%> (+3.38%)`	⬆️
sdk/python/feast/feature_store.py	`93.80% <100.00%> (+16.00%)`	⬆️
sdk/python/feast/feature_view.py	`86.92% <100.00%> (+1.69%)`	⬆️
sdk/python/feast/importer.py	`73.68% <100.00%> (+9.39%)`	⬆️
sdk/python/feast/on_demand_feature_view.py	`89.65% <100.00%> (+56.61%)`	⬆️
sdk/python/tests/unit/test_entity.py	`100.00% <100.00%> (ø)`
sdk/python/feast/online_response.py	`86.79% <0.00%> (-5.71%)`	⬇️
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 75f67ec...c25ac21. Read the comment docs.

MattDelac

Nice !
I tried couple of the inspect functions like getsourcefile() in the other PR but could not make it work.
It makes sense to track where the objects are created 👍

MattDelac · 2021-09-24T11:52:47Z

sdk/python/feast/repo_operations.py

@@ -107,15 +107,20 @@ def parse_repo(repo_root: Path) -> ParsedRepo:
        for attr_name in dir(module):
            obj = getattr(module, attr_name)
            if isinstance(obj, FeatureTable):
-                res.feature_tables.append(obj)
+                if obj.defined_in is not None and obj.defined_in == module.__file__:


Can module.file be None ?

we should maybe also raise exceptions in case defined_in is None

Done. And no, module.file cannot be None

MattDelac · 2021-09-24T11:55:29Z

sdk/python/feast/entity.py

+        stack = inspect.stack()
+        # Get two levels up from current, to ignore usage.py
+        previous_stack_frame = stack[2]
+        self.defined_in = previous_stack_frame.filename


Should we abstract that in an helper function and also test it in a test file ?
Don't think we need to test each class but at least the helper function

What do you think ?

A helper class probably makes sense since there's some state tracking being done. wdyt @achals ?

A helper class

yeah even better 👍

sdk/python/feast/infra/online_stores/redis.py

woop · 2021-09-26T18:08:17Z

Just to play devil's advocate here. Tracking the file names does add complexity to our objects. Is there any reason we cant import all the objects and use built in functionality __hash__() and __eq__ to ensure that they are the same if their names are the same, then we can dedupe/remove the additional ones?

achals · 2021-09-27T16:57:06Z

Just to play devil's advocate here. Tracking the file names does add complexity to our objects. Is there any reason we cant import all the objects and use built in functionality __hash__() and __eq__ to ensure that they are the same if their names are the same, then we can dedupe/remove the additional ones?

IMO updating __hash__ and __eq__ is more fragile since we may add/remove fields without updating those methods, and end up in problematic situations. I think the source file is a more robust solution and less likely to break with changes.

Signed-off-by: Achal Shah <achals@gmail.com>

MattDelac · 2021-09-28T17:11:14Z

sdk/python/feast/feature_table.py

+        stack = inspect.stack()
+        # Get two levels up from current, to ignore usage.py
+        previous_stack_frame = stack[2]
+        self.defined_in = previous_stack_frame.filename


Sounds like missing one get_calling_file_name() here

oh yeah missed this

MattDelac · 2021-09-28T17:11:40Z

sdk/python/feast/infra/online_stores/redis.py

@@ -188,6 +188,7 @@ def online_read(

        for entity_key in entity_keys:
            redis_key_bin = _redis_key(project, entity_key)
+            print(redis_key_bin)


To be removed ?

Signed-off-by: Achal Shah <achals@gmail.com>

adchia

/lgtm

feast-ci-bot · 2021-09-28T18:16:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, adchia

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [achals,adchia]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Achal Shah <achals@gmail.com>

feast-ci-bot · 2021-09-28T18:20:37Z

New changes are detected. LGTM label has been removed.

MattDelac · 2021-09-29T08:19:51Z

👋
I just tried it on our repo and this got shipped with a bug. The problem is how we save the information defined_in.

Look at how my feast apply behaves

module.__file__ is correct but obj.defined_in is not.
The entity "customer" is, indeed, defined in pano/entities/customer.py

woop · 2021-09-29T15:08:47Z

Whether or not there is a bug here, I am not 100% convinced by the approach taken here. There are many reasons to support object equality, deduplication just being one of them. Reaching into the filesystem seems like it will be a lot more prone to errors since its OS/env dependent.

achals · 2021-09-29T17:32:29Z

Whether or not there is a bug here, I am not 100% convinced by the approach taken here. There are many reasons to support object equality, deduplication just being one of them. Reaching into the filesystem seems like it will be a lot more prone to errors since its OS/env dependent.

I'm coming around to this view point after this bug as well. It's pretty easy to get wrong. I'm going to undo this change and use equality based deduping.

Signed-off-by: Achal Shah <achals@gmail.com>

* Use set when parsing repos to prevent duplicates Signed-off-by: Achal Shah <achals@gmail.com> * Undo #1905 changes Signed-off-by: Achal Shah <achals@gmail.com> * use id() in addition to name to differentiate different objects with the same name Signed-off-by: Achal Shah <achals@gmail.com> * add a test Signed-off-by: Achal Shah <achals@gmail.com> * add one more test Signed-off-by: Achal Shah <achals@gmail.com> * remove debugging Signed-off-by: Achal Shah <achals@gmail.com>

Fix issue with feature views being detected as duplicated when imported

87c415d

Signed-off-by: Achal Shah <achals@gmail.com>

achals requested review from adchia, felixwang9817, tsotnet, woop and a team as code owners September 23, 2021 17:52

feast-ci-bot added do-not-merge/release-note-label-needed approved needs-kind labels Sep 23, 2021

achals requested a review from MattDelac September 23, 2021 17:52

feast-ci-bot added the size/M label Sep 23, 2021

achals assigned adchia Sep 23, 2021

feast-ci-bot added release-note-none and removed do-not-merge/release-note-label-needed labels Sep 23, 2021

achals added do-not-merge/release-note-label-needed kind/bug and removed release-note-none labels Sep 23, 2021

feast-ci-bot added release-note-none and removed needs-kind do-not-merge/release-note-label-needed labels Sep 23, 2021

achals mentioned this pull request Sep 23, 2021

Fix feast apply with imported modules #1896

Closed

MattDelac reviewed Sep 24, 2021

View reviewed changes

woop reviewed Sep 26, 2021

View reviewed changes

sdk/python/feast/infra/online_stores/redis.py Outdated Show resolved Hide resolved

Refactor into method

28132fe

Signed-off-by: Achal Shah <achals@gmail.com>

MattDelac reviewed Sep 28, 2021

View reviewed changes

achals added 2 commits September 28, 2021 10:39

missed a spot

96659e3

Signed-off-by: Achal Shah <achals@gmail.com>

Simplify

98f4450

Signed-off-by: Achal Shah <achals@gmail.com>

adchia approved these changes Sep 28, 2021

View reviewed changes

feast-ci-bot added the lgtm label Sep 28, 2021

Fix for real

c25ac21

Signed-off-by: Achal Shah <achals@gmail.com>

feast-ci-bot removed the lgtm label Sep 28, 2021

achals added the lgtm label Sep 28, 2021

feast-ci-bot merged commit 1aedb8a into feast-dev:master Sep 28, 2021

achals mentioned this pull request Sep 29, 2021

Use set when parsing repos to prevent duplicates #1913

Merged

achals added a commit to achals/feast that referenced this pull request Sep 29, 2021

Undo feast-dev#1905 changes

cb0bb8c

Signed-off-by: Achal Shah <achals@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with feature views being detected as duplicated when imported #1905

Fix issue with feature views being detected as duplicated when imported #1905

achals commented Sep 23, 2021 •

edited

Loading

codecov-commenter commented Sep 23, 2021 •

edited

Loading

MattDelac left a comment

MattDelac Sep 24, 2021

woop Sep 26, 2021

achals Sep 28, 2021

MattDelac Sep 24, 2021

adchia Sep 28, 2021

MattDelac Sep 28, 2021

achals Sep 28, 2021

woop commented Sep 26, 2021

achals commented Sep 27, 2021

MattDelac Sep 28, 2021

achals Sep 28, 2021

MattDelac Sep 28, 2021

adchia left a comment

feast-ci-bot commented Sep 28, 2021

feast-ci-bot commented Sep 28, 2021

MattDelac commented Sep 29, 2021 •

edited

Loading

woop commented Sep 29, 2021

achals commented Sep 29, 2021

Fix issue with feature views being detected as duplicated when imported #1905

Fix issue with feature views being detected as duplicated when imported #1905

Conversation

achals commented Sep 23, 2021 • edited Loading

codecov-commenter commented Sep 23, 2021 • edited Loading

Codecov Report

MattDelac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woop commented Sep 26, 2021

achals commented Sep 27, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adchia left a comment

Choose a reason for hiding this comment

feast-ci-bot commented Sep 28, 2021

feast-ci-bot commented Sep 28, 2021

MattDelac commented Sep 29, 2021 • edited Loading

woop commented Sep 29, 2021

achals commented Sep 29, 2021

achals commented Sep 23, 2021 •

edited

Loading

codecov-commenter commented Sep 23, 2021 •

edited

Loading

MattDelac commented Sep 29, 2021 •

edited

Loading