Compare Python objects instead of proto objects #2227

felixwang9817 · 2022-01-20T00:49:18Z

Signed-off-by: Felix Wang wangfelix98@gmail.com

What this PR does / why we need it: This PR switches Registry.diff_between and other helper methods to compare registry objects as Python objects instead of proto objects.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

NONE

codecov-commenter · 2022-01-21T20:05:35Z

Codecov Report

Merging #2227 (4da806e) into master (62fae05) will increase coverage by 0.28%.
The diff coverage is 90.22%.

@@            Coverage Diff             @@
##           master    #2227      +/-   ##
==========================================
+ Coverage   84.92%   85.21%   +0.28%     
==========================================
  Files         105      106       +1     
  Lines        8498     8568      +70     
==========================================
+ Hits         7217     7301      +84     
+ Misses       1281     1267      -14

Flag	Coverage Δ
integrationtests	`72.61% <52.29%> (-0.78%)`	⬇️
unittests	`60.25% <89.08%> (+0.53%)`	⬆️

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

Impacted Files	Coverage Δ
sdk/python/feast/repo_operations.py	`52.21% <40.00%> (+3.82%)`	⬆️
sdk/python/feast/feature_service.py	`82.35% <85.71%> (-0.99%)`	⬇️
sdk/python/feast/diff/FcoDiff.py	`98.91% <98.03%> (+9.82%)`	⬆️
sdk/python/feast/feature_store.py	`89.42% <100.00%> (-0.42%)`	⬇️
sdk/python/feast/registry.py	`83.50% <100.00%> (-0.11%)`	⬇️
sdk/python/feast/repo_contents.py	`100.00% <100.00%> (ø)`
...ts/integration/feature_repos/repo_configuration.py	`94.28% <100.00%> (+0.05%)`	⬆️
sdk/python/tests/unit/diff/test_fco_diff.py	`100.00% <100.00%> (ø)`

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 62fae05...4da806e. Read the comment docs.

adchia · 2022-01-21T20:44:09Z

sdk/python/feast/registry.py

+            ),
+            feature_services=set(self.list_feature_services(project=project)),
+        )
+
    # TODO(achals): This method needs to be filled out and used in the feast plan/apply methods.
    @staticmethod
    def diff_between(


this method seems to logically belong as a method within RegistryDiff or something. You no longer are passing in registry protos at least

my comment still loosely applies. We'll want to eventually do inferencing here as well to resolve the RepoContents to their exact registry form (e.g. including schema, inferred features in a FV, and feature projections in a feature service). A "diff" method to me seems to suggest a much simpler implementation than that.

Would also see this as belong in the same file as where we do inferencing, which I think technically is in feature_store.py. That file is getting a bit large too though. We probably could use some sort of RepoContents -> Registry converter that does this inference logic for feast plan and feast apply

yup this is fair - I moved this method to FcoDiff.py

sdk/python/feast/registry.py

sdk/python/feast/diff/FcoDiff.py

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

sdk/python/feast/feature_store.py

adchia · 2022-01-24T00:52:21Z

sdk/python/feast/registry.py

-        current_registry: RegistryProto, new_registry: RegistryProto
-    ) -> RegistryDiff:
-        diff = RegistryDiff()
+    def extract_objects_for_keep_delete_update_add(


this seems oddly placed here now. (also type annotations please)

I think the only place you call this is in repo_operations and don't see any reason why other classes would ever call this, so maybe throw this there instead?

note that this is also called in diff_between, but still a fair point - it doesn't quite belong with the Registry, so I moved it to FcoDiff.py

adchia · 2022-01-24T00:56:28Z

sdk/python/feast/registry.py

+            ),
+            feature_services=set(self.list_feature_services(project=project)),
+        )
+
    # TODO(achals): This method needs to be filled out and used in the feast plan/apply methods.
    @staticmethod
    def diff_between(


my comment still loosely applies. We'll want to eventually do inferencing here as well to resolve the RepoContents to their exact registry form (e.g. including schema, inferred features in a FV, and feature projections in a feature service). A "diff" method to me seems to suggest a much simpler implementation than that.

Would also see this as belong in the same file as where we do inferencing, which I think technically is in feature_store.py. That file is getting a bit large too though. We probably could use some sort of RepoContents -> Registry converter that does this inference logic for feast plan and feast apply

adchia · 2022-01-24T00:57:20Z

sdk/python/feast/registry.py

@@ -65,6 +66,16 @@
    "": "LocalRegistryStore",
 }

+REGISTRY_OBJECT_TYPE_TO_STR = {


any reason why we can't use the original type name and need to remove the underscores?

we want to convert plural -> single, so removing underscores doesn't quite do it

I see.. this is because we use them directly in feast plan output to the console.

Mind making a comment explaining why this mapping exists? Also, it may make more sense to store this map closer to where it's used (i.e. near some feast plan file or in what today is repo_operations)

adchia · 2022-01-24T01:03:38Z

sdk/python/feast/registry.py

            )

+            objs_to_keep[object_type] = to_keep


What's the purpose of having these mapped by object type? Seems like the caller of this immediately flattens it back into one big list. That also seems to introduce this need to look at REGISTRY_OBJECT_TYPES

we map these by object type because diff_between uses extract_objects_for_keep_delete_update_add, and diff_between needs to know the keep/delete/update/add split by object type in order to compare objects of the same type

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

felixwang9817 · 2022-01-24T19:11:21Z

/retest

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

sdk/python/feast/diff/FcoDiff.py

adchia · 2022-01-25T20:56:41Z

sdk/python/feast/registry.py

@@ -65,6 +66,16 @@
    "": "LocalRegistryStore",
 }

+REGISTRY_OBJECT_TYPE_TO_STR = {


I see.. this is because we use them directly in feast plan output to the console.

Mind making a comment explaining why this mapping exists? Also, it may make more sense to store this map closer to where it's used (i.e. near some feast plan file or in what today is repo_operations)

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

adchia

/lgtm

sdk/python/feast/diff/FcoDiff.py

feast-ci-bot · 2022-01-25T23:09:39Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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~~ [adchia,felixwang9817]

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

felixwang9817 requested a review from adchia January 20, 2022 00:49

felixwang9817 assigned adchia Jan 20, 2022

felixwang9817 requested a review from a team as a code owner January 20, 2022 00:49

feast-ci-bot added release-note-none approved labels Jan 20, 2022

felixwang9817 removed the request for review from a team January 20, 2022 00:49

feast-ci-bot added needs-kind size/L labels Jan 20, 2022

github-actions bot assigned woop Jan 20, 2022

felixwang9817 added kind/feature New feature or request ok-to-test and removed size/L needs-kind labels Jan 20, 2022

felixwang9817 unassigned woop Jan 20, 2022

feast-ci-bot added the size/L label Jan 20, 2022

felixwang9817 added ok-to-test and removed ok-to-test labels Jan 20, 2022

feast-ci-bot added size/XL and removed size/L labels Jan 21, 2022

felixwang9817 added ok-to-test and removed ok-to-test labels Jan 21, 2022

adchia reviewed Jan 21, 2022

View reviewed changes

woop reviewed Jan 21, 2022

View reviewed changes

sdk/python/feast/diff/FcoDiff.py Show resolved Hide resolved

felixwang9817 added 6 commits January 21, 2022 19:04

Compare Python objects instead of proto objects

a5b62b3

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Remove unnecessary helper method

26ee03d

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Fix docstring test

b27df77

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Add docstring to RepoContents

c2e3e82

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Lint

f5fa131

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Update usage test

2104830

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

felixwang9817 added 5 commits January 21, 2022 19:04

Set cache ttl to 1 second in tests for local feature server tests

e661e01

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Add FCO test

b29c4cb

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Add properties to feature service

812689d

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Lint

e66d656

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Remove logic that converts Registry to RepoContents

e076a6b

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

adchia reviewed Jan 24, 2022

View reviewed changes

felixwang9817 added 2 commits January 24, 2022 10:09

Always initialize registry

c19fc42

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Move diffing methods from Registry into FcoDiff.py

ab2c9cb

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

felixwang9817 force-pushed the feast_plan_compare_python_objects branch from d2e700d to ab2c9cb Compare January 24, 2022 18:40

felixwang9817 added 2 commits January 24, 2022 10:45

Fix unit test

d94e918

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Put registry initialization back in repo_operations.py

8e0b5dd

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

Fix usage test

2e8961f

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

adchia approved these changes Jan 25, 2022

View reviewed changes

Switch from hardcoded names to enum

4da806e

Signed-off-by: Felix Wang <wangfelix98@gmail.com>

adchia approved these changes Jan 25, 2022

View reviewed changes

sdk/python/feast/diff/FcoDiff.py Show resolved Hide resolved

sdk/python/feast/diff/FcoDiff.py Show resolved Hide resolved

sdk/python/feast/diff/FcoDiff.py Show resolved Hide resolved

feast-ci-bot added the lgtm label Jan 25, 2022

feast-ci-bot merged commit 3dcec6d into feast-dev:master Jan 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compare Python objects instead of proto objects #2227

Compare Python objects instead of proto objects #2227

felixwang9817 commented Jan 20, 2022

codecov-commenter commented Jan 21, 2022 •

edited

Loading

adchia Jan 21, 2022

adchia Jan 24, 2022

felixwang9817 Jan 24, 2022

adchia Jan 24, 2022

felixwang9817 Jan 24, 2022

adchia Jan 24, 2022

adchia Jan 24, 2022

felixwang9817 Jan 24, 2022

adchia Jan 25, 2022

adchia Jan 24, 2022

felixwang9817 Jan 24, 2022

felixwang9817 commented Jan 24, 2022

adchia Jan 25, 2022

adchia left a comment

feast-ci-bot commented Jan 25, 2022

Compare Python objects instead of proto objects #2227

Compare Python objects instead of proto objects #2227

Conversation

felixwang9817 commented Jan 20, 2022

codecov-commenter commented Jan 21, 2022 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixwang9817 commented Jan 24, 2022

Choose a reason for hiding this comment

adchia left a comment

Choose a reason for hiding this comment

feast-ci-bot commented Jan 25, 2022

codecov-commenter commented Jan 21, 2022 •

edited

Loading