Added .with_name method in FeatureView/OnDemandFeatureView classes for name aliasing. FeatureViewProjection will hold this information #1872

mavysavydav · 2021-09-16T16:00:30Z

Signed-off-by: David Y Liu [email protected]

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

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

Added ".with_name" public method to FeatureView class as part of the shadow entities mapping implementation which will enable users to join a FeatureView to entity data more than once and therefore there may be feature name conflicts. For more context, see the shadow entities mapping RFC here - https://docs.google.com/document/d/1TsCwKf3nVXTAfL0f8i26jnCgHA3bRd4dKQ8QdM87vIA/edit#.

feast-ci-bot · 2021-09-16T16:00:40Z

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.

codecov-commenter · 2021-09-16T16:04:54Z

Codecov Report

Merging #1872 (91595cf) into master (b1ccf8d) will decrease coverage by 0.12%.
The diff coverage is 65.51%.

@@            Coverage Diff             @@
##           master    #1872      +/-   ##
==========================================
- Coverage   82.34%   82.22%   -0.13%     
==========================================
  Files          96       96              
  Lines        7490     7512      +22     
==========================================
+ Hits         6168     6177       +9     
- Misses       1322     1335      +13

Flag	Coverage Δ
integrationtests	`73.84% <58.62%> (-0.08%)`	⬇️
unittests	`60.06% <44.82%> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
sdk/python/feast/errors.py	`70.07% <ø> (ø)`
...python/feast/infra/offline_stores/offline_utils.py	`90.10% <ø> (ø)`
sdk/python/feast/feature_view.py	`84.33% <33.33%> (-1.92%)`	⬇️
sdk/python/feast/on_demand_feature_view.py	`86.15% <33.33%> (-2.56%)`	⬇️
sdk/python/tests/unit/test_feature_validation.py	`83.33% <50.00%> (-16.67%)`	⬇️
sdk/python/feast/feature_store.py	`94.36% <100.00%> (+0.02%)`	⬆️
sdk/python/feast/feature_view_projection.py	`100.00% <100.00%> (ø)`
sdk/python/feast/infra/provider.py	`90.67% <100.00%> (ø)`
sdk/python/feast/cli.py	`38.79% <0.00%> (-0.49%)`	⬇️
... and 4 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 b1ccf8d...91595cf. Read the comment docs.

Signed-off-by: David Y Liu <[email protected]>

sdk/python/feast/feature_view.py

woop · 2021-09-16T19:10:31Z

Btw @mavysavydav your thoughts on #1868 (comment) would be appreciated since it may slightly change this PR.

codyjlin · 2021-09-20T22:43:47Z

We may want to add another case to the FeatureNameCollisionError error message to reflect that they might also need to use .with_name if they're trying to use join on multiple featureviews with the same name (i.e. in the shadow entities case) since full_feature_names=True won't be enough.

feast/sdk/python/feast/errors.py

Lines 144 to 157 in e98d65a

    
           class FeatureNameCollisionError(Exception): 
        
               def __init__(self, feature_refs_collisions: List[str], full_feature_names: bool): 
        
                   if full_feature_names: 
        
                       collisions = [ref.replace(":", "__") for ref in feature_refs_collisions] 
        
                       error_message = ( 
        
                           "To resolve this collision, please ensure that the features in question " 
        
                           "have different names." 
        
                       ) 
        
                   else: 
        
                       collisions = [ref.split(":")[1] for ref in feature_refs_collisions] 
        
                       error_message = ( 
        
                           "To resolve this collision, either use the full feature name by setting " 
        
                           "'full_feature_names=True', or ensure that the features in question have different names." 
        
                       )

mavysavydav · 2021-09-24T17:12:24Z

Cool, added the FeatureNameCollisionError msg modification

Signed-off-by: David Y Liu <[email protected]>

mavysavydav · 2021-10-06T19:12:47Z

/retest

Signed-off-by: David Y Liu <[email protected]>

achals

/lgtm

feast-ci-bot · 2021-10-07T00:20:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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-07T01:00:17Z

protos/feast/core/FeatureViewProjection.proto

@@ -14,6 +14,9 @@ message FeatureViewProjection {
  // The feature view name
  string feature_view_name = 1;

+  // Alias for feature view name
+  string feature_view_name_to_use = 3;


I dont think this is an appropriate name. "Name to use" is confusing if you dont understand the context. What if it's unset, would we use a blank string?

What about feature_view_name_alias?

name_alias seemed like an obvious choice, but it didn't seem to fit for the following reason. My thinking was that name_to_use defaults to the name and it always has a value. Either the alias or the original name. So when projection.name_to_use is used, the name that will be used is returned, whereas with projection.name_alias, one might think -- does it have an alias set? If not, should I do a check? To me, name_to_use is less conventional than a term like an alias, but it seems clear in meaning "the name that will be used". I can see how ppl may find it unfamiliar though and maybe hesitate to assume that it means exactly as it's worded especially if they don't know the use case. Happy to change it.

So when projection.name_to_use is used, the name that will be used is returned, whereas with projection.name_alias, one might think -- does it have an alias set? If not, should I do a check?

I think this is where I see it differently. The caller shouldn't have to check which one is set. They should just ask for name. We should be able to place the conditional logic within a Python property, right?

Of course, we do need a different name in that case though. We can't just have name refer to both the original and the name that has been aliased.

So perhaps

field: name
field: name_alias

method: get_name()
or
method: name_to_use()

In the latter case I am slightly more comfortable with name_to_use since at a storage level we are still keeping the fields separate and clear. My beef with name_to_use is really that it's not a descriptive name. It doesnt convey its meaning. Shouldnt all the code we write be usable? Ideally we'd have something more specific imho, but I dont have any good ideas except above.

ok works for me. I'll make the change

woop · 2021-10-07T01:01:12Z

sdk/python/feast/feature_view_projection.py

@@ -11,11 +11,12 @@
 @dataclass
 class FeatureViewProjection:
    name: str
+    name_to_use: str


What about just alias or name_alias?

woop · 2021-10-07T01:04:01Z

sdk/python/feast/on_demand_feature_view.py

@@ -68,6 +69,25 @@ def __init__(
    def __hash__(self) -> int:
        return hash((id(self), self.name))

+    def with_name(self, name: str):
+        """
+        Produces a copy of this OnDemandFeatureView with the passed name.


What isn't clear is why?
What about
Renames this feature view by setting an alias for the feature view name. This rename operation is only used as part of query operations and will not modify the underlying feature view.?

Sounds good

sdk/python/feast/on_demand_feature_view.py

…r name aliasing. FeatureViewProjection will hold this information (feast-dev#1872) * Added .with_name method in FV with test case Signed-off-by: David Y Liu <[email protected]> * Correctly sort imports Signed-off-by: David Y Liu <[email protected]> * correct lint errors Signed-off-by: David Y Liu <[email protected]> * Modified FeatureNameCollisionError msg Signed-off-by: David Y Liu <[email protected]> * fixed error msg test Signed-off-by: David Y Liu <[email protected]> * updated code to preserve original FV name in base_name field Signed-off-by: David Y Liu <[email protected]> * Updated proto conversion methods and fixed failures Signed-off-by: David Y Liu <[email protected]> * fixed proto numberings Signed-off-by: David Y Liu <[email protected]> * wip Signed-off-by: David Y Liu <[email protected]> * Cleaned code up Signed-off-by: David Y Liu <[email protected]> * added copy method in FVProjections Signed-off-by: David Y Liu <[email protected]> * use python's copy lib rather than creating new copy method. Signed-off-by: David Y Liu <[email protected]>

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

feast-ci-bot added release-note needs-kind needs-ok-to-test labels Sep 16, 2021

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

achals added ok-to-test and removed needs-ok-to-test labels Sep 16, 2021

mavysavydav added the kind/feature New feature or request label Sep 16, 2021

feast-ci-bot removed the needs-kind label Sep 16, 2021

mavysavydav added 3 commits September 16, 2021 09:45

Added .with_name method in FV with test case

a10052f

Signed-off-by: David Y Liu <[email protected]>

Correctly sort imports

ff0d9cf

Signed-off-by: David Y Liu <[email protected]>

correct lint errors

f5c760f

Signed-off-by: David Y Liu <[email protected]>

woop reviewed Sep 16, 2021

View reviewed changes

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

felixwang9817 assigned woop Sep 16, 2021

codyjlin mentioned this pull request Sep 22, 2021

Implement entity aliasing for feature retrieval #1868

Merged

mavysavydav force-pushed the 1867 branch from 634aa6a to 4924bd3 Compare September 23, 2021 21:18

mavysavydav added 2 commits September 24, 2021 10:32

Modified FeatureNameCollisionError msg

a309fbb

Signed-off-by: David Y Liu <[email protected]>

fixed error msg test

0551a43

Signed-off-by: David Y Liu <[email protected]>

mavysavydav force-pushed the 1867 branch from 9ecaee2 to 0551a43 Compare September 24, 2021 17:34

resolve merge conflicts

8a96dca

Signed-off-by: David Y Liu <[email protected]>

Updated proto conversion methods and fixed failures

21d224f

Signed-off-by: David Y Liu <[email protected]>

feast-ci-bot added size/L and removed size/M labels Sep 30, 2021

fixed proto numberings

4ca2048

Signed-off-by: David Y Liu <[email protected]>

feast-ci-bot added size/M and removed size/L labels Sep 30, 2021

Merged with master

0dc06dc

Signed-off-by: David Y Liu <[email protected]>

feast-ci-bot added size/L and removed size/M labels Oct 6, 2021

wip

5a091cf

Signed-off-by: David Y Liu <[email protected]>

mavysavydav force-pushed the 1867 branch from 69fa719 to 5a091cf Compare October 6, 2021 18:57

mavysavydav added 2 commits October 6, 2021 15:51

Cleaned code up

476cbf3

Signed-off-by: David Y Liu <[email protected]>

added copy method in FVProjections

d5cf2c5

Signed-off-by: David Y Liu <[email protected]>

mavysavydav force-pushed the 1867 branch from 32015d6 to d5cf2c5 Compare October 6, 2021 23:50

use python's copy lib rather than creating new copy method.

91595cf

Signed-off-by: David Y Liu <[email protected]>

mavysavydav force-pushed the 1867 branch from 15eae7e to 91595cf Compare October 7, 2021 00:06

achals approved these changes Oct 7, 2021

View reviewed changes

feast-ci-bot assigned achals Oct 7, 2021

feast-ci-bot added lgtm approved labels Oct 7, 2021

mavysavydav changed the title ~~Added .with_name method in FeatureView class~~ Added .with_name method in FeatureView/OnDemandFeatureView classes for name aliasing. FeatureViewProjection will hold this information Oct 7, 2021

feast-ci-bot merged commit cc95c75 into feast-dev:master Oct 7, 2021

woop reviewed Oct 7, 2021

View reviewed changes

sdk/python/feast/on_demand_feature_view.py Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added .with_name method in FeatureView/OnDemandFeatureView classes for name aliasing. FeatureViewProjection will hold this information #1872

Added .with_name method in FeatureView/OnDemandFeatureView classes for name aliasing. FeatureViewProjection will hold this information #1872

mavysavydav commented Sep 16, 2021

feast-ci-bot commented Sep 16, 2021

codecov-commenter commented Sep 16, 2021 •

edited

Loading

woop commented Sep 16, 2021

codyjlin commented Sep 20, 2021

mavysavydav commented Sep 24, 2021 •

edited

Loading

mavysavydav commented Oct 6, 2021

achals left a comment

feast-ci-bot commented Oct 7, 2021

woop Oct 7, 2021

mavysavydav Oct 7, 2021

woop Oct 7, 2021 •

edited

Loading

mavysavydav Oct 7, 2021

woop Oct 7, 2021 •

edited

Loading

woop Oct 7, 2021 •

edited

Loading

mavysavydav Oct 7, 2021

Added .with_name method in FeatureView/OnDemandFeatureView classes for name aliasing. FeatureViewProjection will hold this information #1872

Added .with_name method in FeatureView/OnDemandFeatureView classes for name aliasing. FeatureViewProjection will hold this information #1872

Conversation

mavysavydav commented Sep 16, 2021

feast-ci-bot commented Sep 16, 2021

codecov-commenter commented Sep 16, 2021 • edited Loading

Codecov Report

woop commented Sep 16, 2021

codyjlin commented Sep 20, 2021

mavysavydav commented Sep 24, 2021 • edited Loading

mavysavydav commented Oct 6, 2021

achals left a comment

Choose a reason for hiding this comment

feast-ci-bot commented Oct 7, 2021

woop Oct 7, 2021

Choose a reason for hiding this comment

mavysavydav Oct 7, 2021

Choose a reason for hiding this comment

woop Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

mavysavydav Oct 7, 2021

Choose a reason for hiding this comment

woop Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

woop Oct 7, 2021 • edited Loading

Choose a reason for hiding this comment

mavysavydav Oct 7, 2021

Choose a reason for hiding this comment

codecov-commenter commented Sep 16, 2021 •

edited

Loading

mavysavydav commented Sep 24, 2021 •

edited

Loading

woop Oct 7, 2021 •

edited

Loading

woop Oct 7, 2021 •

edited

Loading

woop Oct 7, 2021 •

edited

Loading