Skip to content
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

Merged
merged 14 commits into from
Oct 7, 2021
Merged
3 changes: 3 additions & 0 deletions protos/feast/core/FeatureViewProjection.proto
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

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?

Copy link
Collaborator Author

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.

Copy link
Member

@woop woop Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

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


// The features of the feature view that are a part of the feature reference.
repeated FeatureSpecV2 feature_columns = 2;
}
5 changes: 3 additions & 2 deletions sdk/python/feast/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,9 @@ 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."
"To resolve this collision, please ensure that the feature views or their own features "
"have different names. If you're intentionally joining the same feature view twice on "
"different sets of entities, please rename one of the feature views with '.with_name'."
)
else:
collisions = [ref.split(":")[1] for ref in feature_refs_collisions]
Expand Down
22 changes: 11 additions & 11 deletions sdk/python/feast/feature_store.py
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ def _get_features(
)
for projection in feature_service_from_registry.feature_view_projections:
_feature_refs.extend(
[f"{projection.name}:{f.name}" for f in projection.features]
[f"{projection.name_to_use}:{f.name}" for f in projection.features]
)
else:
assert isinstance(_features, list)
Expand Down Expand Up @@ -903,7 +903,11 @@ def get_online_features(
GetOnlineFeaturesResponse(field_values=result_rows)
)
return self._augment_response_with_on_demand_transforms(
_feature_refs, full_feature_names, initial_response, result_rows
_feature_refs,
all_on_demand_feature_views,
full_feature_names,
initial_response,
result_rows,
)

def _populate_result_rows_from_feature_view(
Expand Down Expand Up @@ -933,7 +937,7 @@ def _populate_result_rows_from_feature_view(
if feature_data is None:
for feature_name in requested_features:
feature_ref = (
f"{table.name}__{feature_name}"
f"{table.projection.name_to_use}__{feature_name}"
if full_feature_names
else feature_name
)
Expand All @@ -943,7 +947,7 @@ def _populate_result_rows_from_feature_view(
else:
for feature_name in feature_data:
feature_ref = (
f"{table.name}__{feature_name}"
f"{table.projection.name_to_use}__{feature_name}"
if full_feature_names
else feature_name
)
Expand All @@ -970,16 +974,12 @@ def _get_needed_request_data_features(self, grouped_odfv_refs) -> Set[str]:
def _augment_response_with_on_demand_transforms(
self,
feature_refs: List[str],
odfvs: List[OnDemandFeatureView],
full_feature_names: bool,
initial_response: OnlineResponse,
result_rows: List[GetOnlineFeaturesResponse.FieldValues],
) -> OnlineResponse:
all_on_demand_feature_views = {
view.name: view
for view in self._registry.list_on_demand_feature_views(
project=self.project, allow_cache=True
)
}
all_on_demand_feature_views = {view.name: view for view in odfvs}
all_odfv_feature_names = all_on_demand_feature_views.keys()

if len(all_on_demand_feature_views) == 0:
Expand Down Expand Up @@ -1007,7 +1007,7 @@ def _augment_response_with_on_demand_transforms(

for transformed_feature in selected_subset:
transformed_feature_name = (
f"{odfv.name}__{transformed_feature}"
f"{odfv.projection.name_to_use}__{transformed_feature}"
if full_feature_names
else transformed_feature
)
Expand Down
28 changes: 28 additions & 0 deletions sdk/python/feast/feature_view.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
import copy
import re
import warnings
from datetime import datetime, timedelta
Expand Down Expand Up @@ -204,6 +205,33 @@ def is_valid(self):
if not self.entities:
raise ValueError("Feature view has no entities.")

def with_name(self, name: str):
"""
Produces a copy of this FeatureView with the passed name.
mavysavydav marked this conversation as resolved.
Show resolved Hide resolved

Args:
name: Name to assign to the FeatureView copy.

Returns:
A copy of this FeatureView with the name replaced with the 'name' input.
"""
fv = FeatureView(
name=self.name,
entities=self.entities,
ttl=self.ttl,
input=self.input,
batch_source=self.batch_source,
stream_source=self.stream_source,
features=self.features,
tags=self.tags,
online=self.online,
)

fv.set_projection(copy.copy(self.projection))
fv.projection.name_to_use = name

return fv

def to_proto(self) -> FeatureViewProto:
"""
Converts a feature view object to its protobuf representation.
Expand Down
13 changes: 10 additions & 3 deletions sdk/python/feast/feature_view_projection.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,12 @@
@dataclass
class FeatureViewProjection:
name: str
name_to_use: str
Copy link
Member

@woop woop Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about just alias or name_alias?

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())
Expand All @@ -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))

Expand All @@ -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,
)
2 changes: 1 addition & 1 deletion sdk/python/feast/infra/offline_stores/offline_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ def get_feature_view_query_context(
created_timestamp_column = feature_view.input.created_timestamp_column

context = FeatureViewQueryContext(
name=feature_view.name,
name=feature_view.projection.name_to_use,
ttl=ttl_seconds,
entities=join_keys,
features=features,
Expand Down
16 changes: 6 additions & 10 deletions sdk/python/feast/infra/provider.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,18 +186,14 @@ def _get_requested_feature_views_to_features_dict(
feature_from_ref = ref_parts[1]

found = False
for feature_view_from_registry in feature_views:
if feature_view_from_registry.name == feature_view_from_ref:
for fv in feature_views:
if fv.projection.name_to_use == feature_view_from_ref:
found = True
feature_views_to_feature_map[feature_view_from_registry].append(
feature_from_ref
)
for odfv_from_registry in on_demand_feature_views:
if odfv_from_registry.name == feature_view_from_ref:
feature_views_to_feature_map[fv].append(feature_from_ref)
for odfv in on_demand_feature_views:
if odfv.projection.name_to_use == feature_view_from_ref:
found = True
on_demand_feature_views_to_feature_map[odfv_from_registry].append(
feature_from_ref
)
on_demand_feature_views_to_feature_map[odfv].append(feature_from_ref)

if not found:
raise ValueError(f"Could not find feature view from reference {ref}")
Expand Down
20 changes: 20 additions & 0 deletions sdk/python/feast/on_demand_feature_view.py
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
Expand Down Expand Up @@ -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.
Copy link
Member

@woop woop Oct 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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.
Expand Down
25 changes: 13 additions & 12 deletions sdk/python/tests/unit/test_feature_validation.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,13 @@ def test_feature_name_collision_on_historical_retrieval():
full_feature_names=False,
)

expected_error_message = (
"Duplicate features named avg_daily_trips found.\n"
"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."
)
expected_error_message = (
woop marked this conversation as resolved.
Show resolved Hide resolved
"Duplicate features named avg_daily_trips found.\n"
"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."
)

assert str(error.value) == expected_error_message
assert str(error.value) == expected_error_message

# check when feature names collide and 'full_feature_names=True'
with pytest.raises(FeatureNameCollisionError) as error:
Expand All @@ -43,9 +43,10 @@ def test_feature_name_collision_on_historical_retrieval():
full_feature_names=True,
)

expected_error_message = (
"Duplicate features named driver_stats__avg_daily_trips found.\n"
"To resolve this collision, please ensure that the features in question "
"have different names."
)
assert str(error.value) == expected_error_message
expected_error_message = (
"Duplicate features named driver_stats__avg_daily_trips found.\n"
"To resolve this collision, please ensure that the feature views or their own features "
"have different names. If you're intentionally joining the same feature view twice on "
"different sets of entities, please rename one of the feature views with '.with_name'."
)
assert str(error.value) == expected_error_message