Visitar URL original
Added .with_name method in FeatureView/OnDemandFeatureView classes for name aliasing. FeatureViewProjection will hold this information by mavysavydav · Pull Request #1872 · feast-dev/feast · GitHub
Skip to content

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

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.

Sign up for GitHub

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

Merged
feast-ci-bot merged 14 commits into from
Oct 7, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a10052f
Added .with_name method in FV with test case
mavysavydav Sep 16, 2021
ff0d9cf
Correctly sort imports
mavysavydav Sep 16, 2021
f5c760f
correct lint errors
mavysavydav Sep 16, 2021
a309fbb
Modified FeatureNameCollisionError msg
mavysavydav Sep 24, 2021
0551a43
fixed error msg test
mavysavydav Sep 24, 2021
8a96dca
resolve merge conflicts
mavysavydav Sep 24, 2021
c8a72f4
updated code to preserve original FV name in base_name field
mavysavydav Sep 21, 2021
21d224f
Updated proto conversion methods and fixed failures
mavysavydav Sep 24, 2021
4ca2048
fixed proto numberings
mavysavydav Sep 30, 2021
0dc06dc
Merged with master
mavysavydav Oct 5, 2021
5a091cf
wip
mavysavydav Oct 6, 2021
476cbf3
Cleaned code up
mavysavydav Oct 6, 2021
d5cf2c5
added copy method in FVProjections
mavysavydav Oct 6, 2021
91595cf
use python's copy lib rather than creating new copy method.
mavysavydav Oct 7, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions protos/feast/core/FeatureViewProjection.proto
View file
Open in desktop
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;

@woop woop Oct 7, 2021

Copy link
Copy Markdown
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?

@mavysavydav mavysavydav Oct 7, 2021

Copy link
Copy Markdown
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.

@woop woop Oct 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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.

@mavysavydav mavysavydav Oct 7, 2021

Copy link
Copy Markdown
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.
Comment thread
mavysavydav marked this conversation as resolved.
Outdated

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
View file
Open in desktop
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

@woop woop Oct 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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.

@woop woop Oct 7, 2021 •
edited
Loading

Copy link
Copy Markdown
Member

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

@mavysavydav mavysavydav Oct 7, 2021

Copy link
Copy Markdown
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))
Comment thread
woop marked this conversation as 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
View file
Open in desktop
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 = (
Comment thread
woop marked this conversation as 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