Use FeatureViewProjection instead of FeatureView in ODFV #2186

judahrand · 2022-01-04T14:32:47Z

Signed-off-by: Judah Rand 17158624+judahrand@users.noreply.github.com

What this PR does / why we need it:
Before this PR if an ODFV feature is requested then all Features from FeatureView dependencies must be retrieved. This is not performant for FeatureViews which are very wide but with few required features.

This change allows the following spelling to require only a subset of features:

# Define a request data source which encodes features / information only 
# available at request time (e.g. part of the user initiated HTTP request)
input_request = RequestDataSource(
    name="vals_to_add",
    schema={
        "val_to_add": ValueType.INT64,
        "val_to_add_2": ValueType.INT64
    }
)

# Use the input data and feature view features to create new features
@on_demand_feature_view(
   inputs={
       'driver_hourly_stats': driver_hourly_stats_view[['conv_rate']],
       'vals_to_add': input_request
   },
   features=[
     Feature(name='conv_rate_plus_val1', dtype=ValueType.DOUBLE),
     Feature(name='conv_rate_plus_val2', dtype=ValueType.DOUBLE)
   ]
)
def transformed_conv_rate(features_df: pd.DataFrame) -> pd.DataFrame:
    df = pd.DataFrame()
    df['conv_rate_plus_val1'] = (features_df['conv_rate'] + features_df['val_to_add'])
    df['conv_rate_plus_val2'] = (features_df['conv_rate'] + features_df['val_to_add_2'])
    return df

This can dramatically reduce serving latency of ODFVs in some cases.

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

- Use `FeatureViewProjections` to determine OnDemandFeatureView dependencies.
- Fix case where changes to OnDemandFeatureView would not be applied to FeatureStore.
- Reduce serving latency when features from both FeatureViews and OnDemandFeatureViews are requested.
- Improve performance of `get_online_features`.

codecov-commenter · 2022-01-04T14:34:20Z

Codecov Report

Merging #2186 (153ca13) into master (4c32d75) will decrease coverage by 0.10%.
The diff coverage is 91.22%.

@@            Coverage Diff             @@
##           master    #2186      +/-   ##
==========================================
- Coverage   84.55%   84.44%   -0.11%     
==========================================
  Files         104      104              
  Lines        8286     8255      -31     
==========================================
- Hits         7006     6971      -35     
- Misses       1280     1284       +4

Flag	Coverage Δ
integrationtests	`74.20% <90.35%> (-0.41%)`	⬇️
unittests	`58.66% <64.03%> (-0.26%)`	⬇️

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

Impacted Files	Coverage Δ
sdk/python/feast/cli.py	`38.65% <0.00%> (+0.11%)`	⬆️
sdk/python/feast/feature_view.py	`90.78% <ø> (ø)`
sdk/python/feast/on_demand_feature_view.py	`95.20% <83.33%> (-4.05%)`	⬇️
sdk/python/feast/feature_store.py	`91.51% <96.05%> (+0.02%)`	⬆️
sdk/python/feast/infra/passthrough_provider.py	`100.00% <100.00%> (ø)`
sdk/python/feast/infra/provider.py	`90.09% <100.00%> (ø)`
sdk/python/feast/online_response.py	`94.73% <100.00%> (+7.41%)`	⬆️
.../integration/online_store/test_online_retrieval.py	`100.00% <100.00%> (ø)`
sdk/python/feast/type_map.py	`60.93% <0.00%> (-6.25%)`	⬇️
... and 2 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 4c32d75...153ca13. Read the comment docs.

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

achals · 2022-01-04T17:19:44Z

protos/feast/core/OnDemandFeatureView.proto

    map<string, OnDemandInput> inputs = 4;

+    // Map of projections for the inputs of this feature view.
+    // Keys must also be present in `inputs`
+    map<string, FeatureViewProjection> projections = 6;


Can't we add FeatureViewProjection in the OnDemandInput oneof? And remove the feature view if you so wish?

I ended up avoiding doing this because this retrieves the entities of the backing FeatureViews.

feast/sdk/python/feast/cli.py

Line 287 in 7d4369f

for backing_fv in feature_view.inputs.values():

This can't be done if a FeatureView Projection is store instead of the FeatureView.

Hmmm should we look up the entites expicitly via the feature view name, if it's just in that method?

I'd be worried to have two fields in the proto which have this weird dependency between each other, and would prefer a single field - and it feels like we should be able to get away with it.

👍 I'll take another pass at doing that tomorrow.

This ended up being quite a bit more involved.

The changes in this PR definitely reduce the number of calls to the OnlineStore in the simple case of Features being requested from 'an ODFV which depends on a projection of a FV' and from the unprojected version of that FV (1 call rather than 2 and only the required features).

However, when multiple different Projections are used in a FeatureService I'm not 100% we won't end up making multiple calls to the OnlineStore for each Projection + the projection used by the ODFV.

I'm also not 100% we catch the case where 2 ODFVs used different projections of the same FV.

But I do think this approach of working out what we need before starting to request things from the OnlineStore is better than it was.

…ions in ODFVs. Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

…ection Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

judahrand · 2022-01-06T18:26:51Z

@achals I've tacked on some additional changes here which improve the performance of get_online_features quite significantly for large numbers of entity_rows. I'm seeing 2-3x speed ups for parts of the serialization of EntityKeyProtos and adding the entity_rows to the results_rows.

It looks like there may still be a few issues but hopefully will get them solved quickly.

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

judahrand · 2022-01-06T23:05:26Z

@achals All the tests other than the ones that are failing on master are now fixed. It looks like I've actually fixed a handful of bugs which required some alternations to the tests as you can see. This is because the customer entity is actually defined as ValueType.STRING but the tests treated it as if it were ValueType.INT64.

Do you think you could give this another review and look at merging?

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

achals · 2022-01-07T16:00:19Z

Nit, release notes are a bit unclear:

- Update ODFV in UDF code changes.
- Reduce number of calls to OnlineStore ODFVs are used.

judahrand · 2022-01-07T16:05:19Z

Nit, release notes are a bit unclear:

- Update ODFV in UDF code changes.
- Reduce number of calls to OnlineStore ODFVs are used.

Reworded those two.

achals · 2022-01-07T15:52:08Z

sdk/python/feast/on_demand_feature_view.py

+    def __eq__(self, other):
+        if not super().__eq__(other):
+            return False
+
+        if (
+            not self.input_feature_view_projections
+            == other.input_feature_view_projections
+        ):
+            return False
+
+        if not self.input_request_data_sources == other.input_request_data_sources:
+            return False
+
+        if not self.udf.__code__.co_code == other.udf.__code__.co_code:
+            return False
+
+        return True
+
+    def __hash__(self):
+        return super().__hash__()


Curious why these needed to be added?

By default only the name and features properties are checked. This seemed obviously wrong to me... You could change the inputs and UDF but keep the outputs and name the same.

feast/sdk/python/feast/base_feature_view.py

Lines 105 to 117 in 30f7bba

def __eq__(self, other):

if not isinstance(other, BaseFeatureView):

raise TypeError(

"Comparisons should only involve BaseFeatureView class objects."

)

if self.name != other.name:

return False

if sorted(self.features) != sorted(other.features):

return False

return True

Then is __eq__ is overridden __hash__ also has to be! Annoying but true.
https://docs.python.org/3/reference/datamodel.html#object.__hash__

yeah I remember that __hash__ has to be overridden too - okay cool makes sense.

achals · 2022-01-07T15:58:18Z

sdk/python/feast/on_demand_feature_view.py

+            elif on_demand_input.WhichOneof("input") == "feature_view_projection":
+                inputs[input_name] = FeatureViewProjection.from_proto(
+                    on_demand_input.feature_view_projection


Does this mean that fv inputs will be converted from a feature view to a projection when feast apply runs for existing feature repos?

I wonder what the output of feast plan looks like in that case, if anything has changed. If the change does appear, we probably should keep that in mind and update the release notes to have users expect it.

Yes, I believe they should be updated the next time that feast apply is run. The diff is 'large' but makes sense. I'd also suggest maybe this isn't a big deal as the ODFVs are still labelled as Alpha (and I think will need some iteration as calling either to_dict or to_df for them isn't terribly performant).

Yeah fair I don't really want to block this change on the perceived change, but maybe it should be something we call out in the release notes (or the blog post about the release).

achals · 2022-01-07T15:58:31Z

sdk/python/feast/online_response.py

@@ -99,65 +88,3 @@ def to_df(self) -> pd.DataFrame:
        """

        return pd.DataFrame(self.to_dict())
-
-
-def _infer_online_entity_rows(


love seeing red

achals

/lgtm

feast-ci-bot · 2022-01-07T16:27:49Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

judahrand requested a review from a team as a code owner January 4, 2022 14:32

judahrand requested review from tsotnet and removed request for a team January 4, 2022 14:32

feast-ci-bot added do-not-merge/release-note-label-needed needs-kind labels Jan 4, 2022

judahrand added ok-to-test and removed needs-kind labels Jan 4, 2022

feast-ci-bot added the size/M label Jan 4, 2022

judahrand added kind/feature New feature or request and removed size/M labels Jan 4, 2022

github-actions bot assigned tsotnet Jan 4, 2022

judahrand force-pushed the odfv-feature-projection branch from 87a0469 to bad65c1 Compare January 4, 2022 14:34

feast-ci-bot added the size/M label Jan 4, 2022

judahrand added ok-to-test and removed ok-to-test labels Jan 4, 2022

judahrand force-pushed the odfv-feature-projection branch from bad65c1 to 5bfd7fd Compare January 4, 2022 14:39

feast-ci-bot added release-note and removed do-not-merge/release-note-label-needed labels Jan 4, 2022

judahrand added ok-to-test and removed ok-to-test labels Jan 4, 2022

judahrand force-pushed the odfv-feature-projection branch 2 times, most recently from cb1cd2c to 5e45b47 Compare January 4, 2022 15:48

Use FeatureViewProjection instead of FeatureView in ODFV

7d40161

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

judahrand force-pushed the odfv-feature-projection branch from 5e45b47 to 7d40161 Compare January 4, 2022 15:51

achals requested changes Jan 4, 2022

View reviewed changes

achals self-assigned this Jan 4, 2022

judahrand requested a review from achals January 4, 2022 18:50

judahrand added 2 commits January 4, 2022 19:19

Merge branch 'master' into odfv-feature-projection

f4d5a7f

Refactor get_online_features to allow for use of FeatureViewProject…

7d516af

…ions in ODFVs. Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

Refactor get_online_features to improve performance

792b3cc

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

feast-ci-bot added size/XL and removed size/L labels Jan 6, 2022

judahrand added 2 commits January 6, 2022 18:08

Merge remote-tracking branch 'upstream/master' into odfv-feature-proj…

23713a5

…ection Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

Fix linting error

56d3049

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

Refactor _set_table_entity_keys for clarity and performance

39cb7b3

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

judahrand force-pushed the odfv-feature-projection branch from 28a29b9 to 39cb7b3 Compare January 6, 2022 21:04

judahrand mentioned this pull request Jan 6, 2022

Refactor _get_table_entity_keys for small performance benefit and readability. #2192

Closed

judahrand force-pushed the odfv-feature-projection branch 2 times, most recently from 30cdd2d to 6ef9773 Compare January 6, 2022 22:28

Customer IDs should be ValueType.STRING

d5d5c24

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

judahrand force-pushed the odfv-feature-projection branch from 6ef9773 to 2548800 Compare January 6, 2022 22:53

Use Entity ValueType information in _convert_arrow_to_proto

1d20807

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

judahrand force-pushed the odfv-feature-projection branch from 2548800 to 1d20807 Compare January 6, 2022 22:56

judahrand added 2 commits January 6, 2022 23:18

Only call _augment_response_with_on_demand_transforms if needed

1900a48

Signed-off-by: Judah Rand <17158624+judahrand@users.noreply.github.com>

Merge branch 'master' into odfv-feature-projection

f9384b9

This was referenced Jan 7, 2022

Python FeatureServer inefficiently converts entity rows #2201

Closed

Python FeatureServer optimization #2202

Merged

Merge branch 'master' into odfv-feature-projection

153ca13

achals reviewed Jan 7, 2022

View reviewed changes

achals approved these changes Jan 7, 2022

View reviewed changes

feast-ci-bot added the lgtm label Jan 7, 2022

feast-ci-bot added the approved label Jan 7, 2022

feast-ci-bot merged commit f969e53 into feast-dev:master Jan 7, 2022

judahrand deleted the odfv-feature-projection branch January 19, 2022 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use FeatureViewProjection instead of FeatureView in ODFV #2186

Use FeatureViewProjection instead of FeatureView in ODFV #2186

judahrand commented Jan 4, 2022 •

edited

Loading

codecov-commenter commented Jan 4, 2022 •

edited

Loading

achals Jan 4, 2022

judahrand Jan 4, 2022 •

edited

Loading

achals Jan 4, 2022

achals Jan 4, 2022

judahrand Jan 4, 2022

judahrand Jan 5, 2022

judahrand commented Jan 6, 2022 •

edited

Loading

judahrand commented Jan 6, 2022

achals commented Jan 7, 2022

judahrand commented Jan 7, 2022

achals Jan 7, 2022

judahrand Jan 7, 2022

achals Jan 7, 2022

achals Jan 7, 2022

judahrand Jan 7, 2022

achals Jan 7, 2022

achals Jan 7, 2022

achals left a comment

feast-ci-bot commented Jan 7, 2022

	def __eq__(self, other):
	if not isinstance(other, BaseFeatureView):
	raise TypeError(
	"Comparisons should only involve BaseFeatureView class objects."
	)

	if self.name != other.name:
	return False

	if sorted(self.features) != sorted(other.features):
	return False

	return True

Use FeatureViewProjection instead of FeatureView in ODFV #2186

Use FeatureViewProjection instead of FeatureView in ODFV #2186

Conversation

judahrand commented Jan 4, 2022 • edited Loading

codecov-commenter commented Jan 4, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

judahrand Jan 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

judahrand commented Jan 6, 2022 • edited Loading

judahrand commented Jan 6, 2022

achals commented Jan 7, 2022

judahrand commented Jan 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achals left a comment

Choose a reason for hiding this comment

feast-ci-bot commented Jan 7, 2022

judahrand commented Jan 4, 2022 •

edited

Loading

codecov-commenter commented Jan 4, 2022 •

edited

Loading

judahrand Jan 4, 2022 •

edited

Loading

judahrand commented Jan 6, 2022 •

edited

Loading