replace GetOnlineFeaturesResponse with GetOnlineFeaturesResponseV2 in… #2214

tsotnet · 2022-01-14T17:37:15Z

… Python

Signed-off-by: Tsotne Tabidze tsotne@tecton.ai

What this PR does / why we need it: In Java server we started using GetOnlineFeaturesResponseV2 instead of GetOnlineFeaturesResponse as a response proto type. This PR changes Python codebase to also use GetOnlineFeaturesResponseV2. We also get rid of the previous GetOnlineFeaturesResponse definition completely and rename GetOnlineFeaturesResponseV2 to GetOnlineFeaturesResponse. This is a backwards compatible change for the SDK. Namely, we aren't changing the results of feature_store.get_online_features(...).{to_dict(),to_df()} calls. However, the Python HTTP server's response will be different. This is an example of a response:

{
  "metadata": {
    "feature_names": [
      "driver_id",
      "conv_rate",
      "acc_rate",
      "avg_daily_trips",
      "conv_rate_plus_val1",
      "conv_rate_plus_val2"
    ]
  },
  "results": [
    {
      "values": [
        1001,
        0.7037263512611389,
        0.8724706768989563,
        308,
        1.703726351261139,
        10.703726351261139
      ],
      "statuses": [
        "PRESENT",
        "PRESENT",
        "PRESENT",
        "PRESENT",
        "PRESENT",
        "PRESENT"
      ],
      "event_timestamps": [
        "1970-01-01T00:00:00Z",
        "2021-12-31T23:00:00Z",
        "2021-12-31T23:00:00Z",
        "2021-12-31T23:00:00Z",
        "1970-01-01T00:00:00Z",
        "1970-01-01T00:00:00Z"
      ]
    },
    {
      "values": [
        1002,
        0.038169607520103455,
        0.48534533381462097,
        332,
        2.0381696075201035,
        20.038169607520103
      ],
      "statuses": [
        "PRESENT",
        "PRESENT",
        "PRESENT",
        "PRESENT",
        "PRESENT",
        "PRESENT"
      ],
      "event_timestamps": [
        "1970-01-01T00:00:00Z",
        "2021-12-31T23:00:00Z",
        "2021-12-31T23:00:00Z",
        "2021-12-31T23:00:00Z",
        "1970-01-01T00:00:00Z",
        "1970-01-01T00:00:00Z"
      ]
    },
    {
      "values": [
        1003,
        0.9665873050689697,
        0.7793770432472229,
        779,
        3.9665873050689697,
        30.96658730506897
      ],
      "statuses": [
        "PRESENT",
        "PRESENT",
        "PRESENT",
        "PRESENT",
        "PRESENT",
        "PRESENT"
      ],
      "event_timestamps": [
        "1970-01-01T00:00:00Z",
        "2021-12-31T23:00:00Z",
        "2021-12-31T23:00:00Z",
        "2021-12-31T23:00:00Z",
        "1970-01-01T00:00:00Z",
        "1970-01-01T00:00:00Z"
      ]
    },
    {
      "values": [
        0,
        null,
        null,
        null,
        null,
        null
      ],
      "statuses": [
        "PRESENT",
        "NOT_FOUND",
        "NOT_FOUND",
        "NOT_FOUND",
        "PRESENT",
        "PRESENT"
      ],
      "event_timestamps": [
        "1970-01-01T00:00:00Z",
        "1970-01-01T00:00:00Z",
        "1970-01-01T00:00:00Z",
        "1970-01-01T00:00:00Z",
        "1970-01-01T00:00:00Z",
        "1970-01-01T00:00:00Z"
      ]
    }
  ]
}

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Use a new response schema for online feature retrieval in Python

… Python Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>

feast-ci-bot · 2022-01-14T18:17:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pyalex, tsotnet

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~~ [tsotnet]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>

codecov-commenter · 2022-01-14T18:37:03Z

Codecov Report

Merging #2214 (eedac3c) into master (2a95629) will increase coverage by 0.14%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2214      +/-   ##
==========================================
+ Coverage   84.98%   85.12%   +0.14%     
==========================================
  Files         105      105              
  Lines        8450     8453       +3     
==========================================
+ Hits         7181     7196      +15     
+ Misses       1269     1257      -12

Flag	Coverage Δ
integrationtests	`73.23% <76.08%> (-0.13%)`	⬇️
unittests	`59.83% <68.47%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
...n/tests/integration/online_store/test_e2e_local.py	`100.00% <ø> (ø)`
sdk/python/feast/feature_store.py	`91.82% <100.00%> (+0.23%)`	⬆️
sdk/python/feast/infra/online_stores/dynamodb.py	`85.61% <100.00%> (ø)`
sdk/python/feast/online_response.py	`100.00% <100.00%> (+5.26%)`	⬆️
.../integration/online_store/test_universal_online.py	`98.13% <100.00%> (ø)`
sdk/python/tests/unit/test_proto_json.py	`100.00% <100.00%> (ø)`
sdk/python/feast/proto_json.py	`92.42% <0.00%> (-3.04%)`	⬇️
sdk/python/feast/infra/online_stores/datastore.py	`89.15% <0.00%> (+1.88%)`	⬆️
sdk/python/feast/type_map.py	`66.40% <0.00%> (+6.25%)`	⬆️

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 2a95629...eedac3c. Read the comment docs.

Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>

pyalex · 2022-01-14T20:16:52Z

sdk/python/feast/feature_store.py

+            for row_idx, result_row in enumerate(online_features_response.results):
+                result_row.values.append(values[row_idx])
+                result_row.statuses.append(FieldStatus.PRESENT)
+                result_row.event_timestamps.append(Timestamp())


So python feature server won't provide real event timestamp? and correct status as well

ah, this is entities. I got it.
But why we put entities in feature vectors?

Looks like this note got lost during the conflict resolution:

# Note: We temporarily store entities in the same field as features for backwards compatibility. # Java server uses the same GetOnlineFeaturesResponse but does not return entities right now. # We need to unify the logic between Python & Java on this.

Basically Python SDK used to return entities as part of the online features response and I don't want to break that with this PR. Although we should have a followup conversation about how to properly resolve this.

pyalex · 2022-01-14T20:21:15Z

sdk/python/feast/feature_store.py

    ):
        # Add more feature values to the existing result rows for the request data features
        for feature_name, feature_values in request_data_features.items():
            proto_values = python_values_to_proto_values(
                feature_values, ValueType.UNKNOWN
            )

+            online_features_response.metadata.feature_names.val.append(feature_name)


Why do we put values from request back into response? It seems inconsistent with the fact that we don't put entities in response

Same reason as for entities

pyalex · 2022-01-14T21:51:50Z

/lgtm

replace GetOnlineFeaturesResponse with GetOnlineFeaturesResponseV2 in…

4f374ff

… Python Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>

tsotnet requested a review from pyalex January 14, 2022 17:37

tsotnet requested a review from a team as a code owner January 14, 2022 17:37

feast-ci-bot added release-note approved needs-kind size/XL labels Jan 14, 2022

github-actions bot assigned adchia Jan 14, 2022

pyalex approved these changes Jan 14, 2022

View reviewed changes

Merge branch 'master' into python_new_response_proto

878cf4b

tsotnet added the kind/housekeeping label Jan 14, 2022

feast-ci-bot removed the needs-kind label Jan 14, 2022

Fix unit tests

f0a0aae

Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>

adchia assigned pyalex Jan 14, 2022

Tsotne Tabidze added 2 commits January 14, 2022 11:19

Fix java compilation & python integration tests

6a67d76

Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>

Fix integration tests

eedac3c

Signed-off-by: Tsotne Tabidze <tsotne@tecton.ai>

pyalex reviewed Jan 14, 2022

View reviewed changes

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

feast-ci-bot merged commit 1b98ec9 into feast-dev:master Jan 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

replace GetOnlineFeaturesResponse with GetOnlineFeaturesResponseV2 in… #2214

replace GetOnlineFeaturesResponse with GetOnlineFeaturesResponseV2 in… #2214

tsotnet commented Jan 14, 2022

feast-ci-bot commented Jan 14, 2022

codecov-commenter commented Jan 14, 2022 •

edited

Loading

pyalex Jan 14, 2022

pyalex Jan 14, 2022

tsotnet Jan 14, 2022 •

edited

Loading

pyalex Jan 14, 2022

tsotnet Jan 14, 2022

pyalex commented Jan 14, 2022

replace GetOnlineFeaturesResponse with GetOnlineFeaturesResponseV2 in… #2214

replace GetOnlineFeaturesResponse with GetOnlineFeaturesResponseV2 in… #2214

Conversation

tsotnet commented Jan 14, 2022

feast-ci-bot commented Jan 14, 2022

codecov-commenter commented Jan 14, 2022 • edited Loading

Codecov Report

pyalex Jan 14, 2022

Choose a reason for hiding this comment

pyalex Jan 14, 2022

Choose a reason for hiding this comment

tsotnet Jan 14, 2022 • edited Loading

Choose a reason for hiding this comment

pyalex Jan 14, 2022

Choose a reason for hiding this comment

tsotnet Jan 14, 2022

Choose a reason for hiding this comment

pyalex commented Jan 14, 2022

codecov-commenter commented Jan 14, 2022 •

edited

Loading

tsotnet Jan 14, 2022 •

edited

Loading