-
Notifications
You must be signed in to change notification settings - Fork 1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added .with_name method in FeatureView/OnDemandFeatureView classes for name aliasing. FeatureViewProjection will hold this information #1872
Changes from all commits
a10052f
ff0d9cf
f5c760f
a309fbb
0551a43
8a96dca
c8a72f4
21d224f
4ca2048
0dc06dc
5a091cf
476cbf3
d5cf2c5
91595cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,11 +11,12 @@ | |
@dataclass | ||
class FeatureViewProjection: | ||
name: str | ||
name_to_use: str | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about just |
||
features: List[Feature] | ||
|
||
def to_proto(self): | ||
feature_reference_proto = FeatureViewProjectionProto( | ||
feature_view_name=self.name | ||
feature_view_name=self.name, feature_view_name_to_use=self.name_to_use | ||
) | ||
for feature in self.features: | ||
feature_reference_proto.feature_columns.append(feature.to_proto()) | ||
|
@@ -24,7 +25,11 @@ def to_proto(self): | |
|
||
@staticmethod | ||
def from_proto(proto: FeatureViewProjectionProto): | ||
ref = FeatureViewProjection(name=proto.feature_view_name, features=[]) | ||
ref = FeatureViewProjection( | ||
name=proto.feature_view_name, | ||
name_to_use=proto.feature_view_name_to_use, | ||
features=[], | ||
) | ||
for feature_column in proto.feature_columns: | ||
ref.features.append(Feature.from_proto(feature_column)) | ||
|
||
|
@@ -33,5 +38,7 @@ def from_proto(proto: FeatureViewProjectionProto): | |
@staticmethod | ||
def from_definition(feature_grouping): | ||
return FeatureViewProjection( | ||
name=feature_grouping.name, features=feature_grouping.features | ||
name=feature_grouping.name, | ||
name_to_use=feature_grouping.name, | ||
features=feature_grouping.features, | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,4 @@ | ||
import copy | ||
import functools | ||
from types import MethodType | ||
from typing import Dict, List, Union, cast | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What isn't clear is why? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good |
||
|
||
Args: | ||
name: Name to assign to the OnDemandFeatureView copy. | ||
|
||
Returns: | ||
A copy of this OnDemandFeatureView with the name replaced with the 'name' input. | ||
""" | ||
odfv = OnDemandFeatureView( | ||
name=self.name, features=self.features, inputs=self.inputs, udf=self.udf | ||
) | ||
|
||
odfv.set_projection(copy.copy(self.projection)) | ||
woop marked this conversation as resolved.
Show resolved
Hide resolved
|
||
odfv.projection.name_to_use = name | ||
|
||
return odfv | ||
|
||
def to_proto(self) -> OnDemandFeatureViewProto: | ||
""" | ||
Converts an on demand feature view object to its protobuf representation. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 withname_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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok works for me. I'll make the change