Added .with_name method in FeatureView/OnDemandFeatureView classes for name aliasing. FeatureViewProjection will hold this information by mavysavydav · Pull Request #1872 · feast-dev/feast

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

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

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.

Details

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 <davidyliuliu@gmail.com>

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 <davidyliuliu@gmail.com>

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

/retest

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

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

Details

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

  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

 @dataclass
 class FeatureViewProjection:
    name: str
+    name_to_use: str


What about just alias or name_alias?

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


+    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

…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 <davidyliuliu@gmail.com> * Correctly sort imports Signed-off-by: David Y Liu <davidyliuliu@gmail.com> * correct lint errors Signed-off-by: David Y Liu <davidyliuliu@gmail.com> * Modified FeatureNameCollisionError msg Signed-off-by: David Y Liu <davidyliuliu@gmail.com> * fixed error msg test Signed-off-by: David Y Liu <davidyliuliu@gmail.com> * updated code to preserve original FV name in base_name field Signed-off-by: David Y Liu <davidyliuliu@gmail.com> * Updated proto conversion methods and fixed failures Signed-off-by: David Y Liu <davidyliuliu@gmail.com> * fixed proto numberings Signed-off-by: David Y Liu <davidyliuliu@gmail.com> * wip Signed-off-by: David Y Liu <davidyliuliu@gmail.com> * Cleaned code up Signed-off-by: David Y Liu <davidyliuliu@gmail.com> * added copy method in FVProjections Signed-off-by: David Y Liu <davidyliuliu@gmail.com> * use python's copy lib rather than creating new copy method. Signed-off-by: David Y Liu <davidyliuliu@gmail.com>

mavysavydav requested review from a team, achals, adchia, felixwang9817, tsotnet and woop 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 <davidyliuliu@gmail.com>

Correctly sort imports

ff0d9cf

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

correct lint errors

f5c760f

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

woop reviewed Sep 16, 2021

View reviewed changes

Comment thread sdk/python/feast/feature_view.py Outdated

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 <davidyliuliu@gmail.com>

fixed error msg test

0551a43

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

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 <davidyliuliu@gmail.com>

Updated proto conversion methods and fixed failures

21d224f

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

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

fixed proto numberings

4ca2048

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

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

Merged with master

0dc06dc

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

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

wip

5a091cf

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

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 <davidyliuliu@gmail.com>

added copy method in FVProjections

d5cf2c5

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

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 <davidyliuliu@gmail.com>

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

Comment thread sdk/python/feast/on_demand_feature_view.py

Uh oh!

Conversation

mavysavydav commented Sep 16, 2021

Uh oh!

feast-ci-bot commented Sep 16, 2021

Uh oh!

codecov-commenter commented Sep 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

woop commented Sep 16, 2021

Uh oh!

codyjlin commented Sep 20, 2021

Uh oh!

mavysavydav commented Sep 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mavysavydav commented Oct 6, 2021

Uh oh!

achals left a comment

Choose a reason for hiding this comment

Uh oh!

feast-ci-bot commented Oct 7, 2021

Uh oh!

woop Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

mavysavydav Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

woop Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavysavydav Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

woop Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

woop Oct 7, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavysavydav Oct 7, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

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