Hide FeatureViewProjections from user interface & have FeatureViews carry FVProjections that carries the modified info of the FeatureView #1899

mavysavydav · 2021-09-22T22:45:14Z

DEPENDENT ON - #1899

What this PR does / why we need it: #1907

Which issue(s) this PR fixes: #1907

Fixes #1907

Does this PR introduce a user-facing change?:

When users do my_feature_view[[feature_a, ...]], a FeatureView will be returned rather than a FeatureViewProjection. This doesn't really impact anything from a usage perspective and users don't need to make any adjustments.  Everything will work the same as before.

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

feast-ci-bot · 2021-09-22T22:45:23Z

Hi @mavysavydav. Thanks for your PR.

I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Signed-off-by: Cody Lin <codyl@twitter.com>

codecov-commenter · 2021-09-23T19:58:20Z

Codecov Report

Merging #1899 (31cd6a8) into master (5d0f37b) will decrease coverage by 22.82%.
The diff coverage is 39.06%.

@@             Coverage Diff             @@
##           master    #1899       +/-   ##
===========================================
- Coverage   82.95%   60.13%   -22.83%     
===========================================
  Files          96       96               
  Lines        7359     7490      +131     
===========================================
- Hits         6105     4504     -1601     
- Misses       1254     2986     +1732

Flag	Coverage Δ
integrationtests	`?`
unittests	`60.13% <39.06%> (-0.64%)`	⬇️

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

Impacted Files	Coverage Δ
sdk/python/feast/cli.py	`39.27% <0.00%> (ø)`
.../integration/online_store/test_universal_online.py	`14.58% <ø> (-85.42%)`	⬇️
sdk/python/feast/on_demand_feature_view.py	`32.25% <25.00%> (-57.22%)`	⬇️
sdk/python/feast/feature_service.py	`61.76% <36.36%> (-13.58%)`	⬇️
sdk/python/feast/feature_view.py	`78.12% <38.46%> (-8.46%)`	⬇️
sdk/python/feast/feature_store.py	`75.94% <46.15%> (-17.87%)`	⬇️
sdk/python/feast/feature_view_projection.py	`63.63% <100.00%> (-36.37%)`	⬇️
.../integration/online_store/test_online_retrieval.py	`17.39% <0.00%> (-82.61%)`	⬇️
sdk/python/tests/utils/online_read_write_test.py	`18.18% <0.00%> (-81.82%)`	⬇️
... and 54 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 5d0f37b...31cd6a8. Read the comment docs.

Signed-off-by: Cody Lin <codyl@twitter.com>

sdk/python/feast/feature_store.py

mavysavydav · 2021-09-30T22:14:52Z

/retest

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

woop · 2021-10-01T02:09:19Z

What happened to setup.py?

sdk/python/feast/feature_store.py

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

sdk/python/feast/feature_service.py

woop · 2021-10-04T22:07:42Z

sdk/python/feast/feature_service.py

            tags=dict(feature_service_proto.spec.tags),
            description=(
                feature_service_proto.spec.description
                if feature_service_proto.spec.description != ""
                else None
            ),
        )
+        fs.feature_view_projections.extend(


I'm curious why you moved this code out of the constructor. Don't feel strongly about it though.

woop · 2021-10-04T22:18:37Z

sdk/python/feast/feature_view.py

+
+    def set_projection(self, feature_view_projection: FeatureViewProjection) -> None:
+        """
+        Setter for the projection object held by this FeatureView. Performs checks to ensure


Nitpick: These comments assume the user knows exactly what a projection is. I think it's important to define what a projection is.

sdk/python/feast/feature_view.py

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

feast-ci-bot · 2021-10-05T02:38:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, mavysavydav, woop

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,woop]

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

woop · 2021-10-05T02:39:02Z

/lgtm

mavysavydav added 3 commits September 22, 2021 15:33

Revised FeatureService class and proto

0e802f4

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

wip transitioning feast off of FVProjections

f17b075

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

corrected a lot of errors and tests failures

e24d04a

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

mavysavydav requested review from achals, adchia, felixwang9817, tsotnet, woop and a team as code owners September 22, 2021 22:45

feast-ci-bot added do-not-merge/release-note-label-needed needs-kind needs-ok-to-test labels Sep 22, 2021

feast-ci-bot added the size/L label Sep 22, 2021

mavysavydav changed the title ~~[draft in my own repo] Remove fv projections~~ [draft, not ready for review] Remove FeatureViewProjections & handle its responsibility with FeatureView and FeatureService Sep 22, 2021

mavysavydav removed request for achals, adchia, felixwang9817, tsotnet, woop and a team September 22, 2021 23:23

mavysavydav marked this pull request as draft September 23, 2021 03:08

feast-ci-bot added the do-not-merge/work-in-progress label Sep 23, 2021

fix unit test failure and most lint issues

ffe284a

Signed-off-by: Cody Lin <codyl@twitter.com>

remove debugging print statement

37b6d87

Signed-off-by: Cody Lin <codyl@twitter.com>

codyjlin reviewed Sep 23, 2021

View reviewed changes

sdk/python/feast/feature_store.py Outdated Show resolved Hide resolved

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

mavysavydav changed the title ~~[draft, not ready for review] Remove FeatureViewProjections & handle its responsibility with FeatureView and FeatureService~~ Remove FeatureViewProjections & handle its responsibility with FeatureView and FeatureService Sep 23, 2021

mavysavydav force-pushed the RemoveFVProjections2 branch from c4a042c to e69ad89 Compare September 30, 2021 22:13

fixed merge conflicts

bf8eb94

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

woop reviewed Oct 1, 2021

View reviewed changes

sdk/python/feast/feature_store.py Outdated Show resolved Hide resolved

woop reviewed Oct 1, 2021

View reviewed changes

sdk/python/feast/feature_store.py Outdated Show resolved Hide resolved

Brought back FVProjections without exposing it ever to user-facing API

009edf4

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

mavysavydav force-pushed the RemoveFVProjections2 branch from a5aab1a to 67c7d47 Compare October 4, 2021 18:06

Updated OnDemandFV to this new paradigm. Other misc updates

d65326d

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

mavysavydav force-pushed the RemoveFVProjections2 branch 2 times, most recently from 4f810d8 to 292f9de Compare October 4, 2021 18:38

mavysavydav changed the title ~~Remove FeatureViewProjections & handle its responsibility with FeatureView and FeatureService~~ Hide FeatureViewProjections from user interface & have FeatureViews carry FVProjections that carries the modified info of the FeatureView Oct 4, 2021

feast-ci-bot added release-note and removed release-note-none labels Oct 4, 2021

mavysavydav force-pushed the RemoveFVProjections2 branch 2 times, most recently from e2c3a53 to bad7e63 Compare October 4, 2021 19:22

corrections based on integration test failures

659374a

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

mavysavydav force-pushed the RemoveFVProjections2 branch from bad7e63 to 659374a Compare October 4, 2021 19:41

woop reviewed Oct 4, 2021

View reviewed changes

sdk/python/feast/feature_service.py Outdated Show resolved Hide resolved

woop reviewed Oct 4, 2021

View reviewed changes

sdk/python/feast/feature_view.py Outdated Show resolved Hide resolved

mavysavydav force-pushed the RemoveFVProjections2 branch from abc655b to ef0d75c Compare October 4, 2021 23:03

corrections based on CR comments

31cd6a8

Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

mavysavydav force-pushed the RemoveFVProjections2 branch from ef0d75c to 31cd6a8 Compare October 4, 2021 23:35

woop approved these changes Oct 5, 2021

View reviewed changes

feast-ci-bot added the lgtm label Oct 5, 2021

feast-ci-bot merged commit b1ccf8d into feast-dev:master Oct 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide FeatureViewProjections from user interface & have FeatureViews carry FVProjections that carries the modified info of the FeatureView #1899

Hide FeatureViewProjections from user interface & have FeatureViews carry FVProjections that carries the modified info of the FeatureView #1899

mavysavydav commented Sep 22, 2021 •

edited

Loading

feast-ci-bot commented Sep 22, 2021

codecov-commenter commented Sep 23, 2021 •

edited

Loading

mavysavydav commented Sep 30, 2021

woop commented Oct 1, 2021

woop Oct 4, 2021

woop Oct 4, 2021

feast-ci-bot commented Oct 5, 2021

woop commented Oct 5, 2021

Hide FeatureViewProjections from user interface & have FeatureViews carry FVProjections that carries the modified info of the FeatureView #1899

Hide FeatureViewProjections from user interface & have FeatureViews carry FVProjections that carries the modified info of the FeatureView #1899

Conversation

mavysavydav commented Sep 22, 2021 • edited Loading

feast-ci-bot commented Sep 22, 2021

codecov-commenter commented Sep 23, 2021 • edited Loading

Codecov Report

mavysavydav commented Sep 30, 2021

woop commented Oct 1, 2021

woop Oct 4, 2021

Choose a reason for hiding this comment

woop Oct 4, 2021

Choose a reason for hiding this comment

feast-ci-bot commented Oct 5, 2021

woop commented Oct 5, 2021

mavysavydav commented Sep 22, 2021 •

edited

Loading

codecov-commenter commented Sep 23, 2021 •

edited

Loading