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

Local feature server implementation (HTTP endpoint) #1780

Merged
merged 3 commits into from
Aug 21, 2021

Conversation

tsotnet
Copy link
Collaborator

@tsotnet tsotnet commented Aug 13, 2021

Signed-off-by: Tsotne Tabidze [email protected]

What this PR does / why we need it: Implements the local HTTP feature consumption server.

RFCs for context:

From one terminal tab we can run feast serve:

$ feast serve
INFO:     Started server process [13615]
08/12/2021 09:29:36 PM INFO:Started server process [13615]
INFO:     Waiting for application startup.
08/12/2021 09:29:36 PM INFO:Waiting for application startup.
INFO:     Application startup complete.
08/12/2021 09:29:36 PM INFO:Application startup complete.
INFO:     Uvicorn running on http://127.0.0.1:6566 (Press CTRL+C to quit)
08/12/2021 09:29:36 PM INFO:Uvicorn running on http://127.0.0.1:6566 (Press CTRL+C to quit)
INFO:     127.0.0.1:52654 - "GET /get-online-features/ HTTP/1.1" 200 OK
INFO:     127.0.0.1:52660 - "GET /get-online-features/ HTTP/1.1" 200 OK

From another tab we can run cURL requests:

$ curl -X GET \
    "http://127.0.0.1:6566/get-online-features/" \
    -H "Content-type: application/json" \
    -H "Accept: application/json" \
    -d '{
        "features": [
            "driver_hourly_stats:conv_rate",
            "driver_hourly_stats:acc_rate",
            "driver_hourly_stats:avg_daily_trips"
        ],
        "entities": {
            "driver_id": [1001, 1002, 1003]
        },
        "full_feature_names": true
    }' | jq
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1345  100  1054  100   291   3685   1017 --:--:-- --:--:-- --:--:--  4702
{
  "field_values": [
    {
      "fields": {
        "driver_hourly_stats__conv_rate": 0.5315734148025513,
        "driver_id": 1001,
        "driver_hourly_stats__acc_rate": 0.5842991471290588,
        "driver_hourly_stats__avg_daily_trips": 714
      },
      "statuses": {
        "driver_hourly_stats__conv_rate": "PRESENT",
        "driver_hourly_stats__acc_rate": "PRESENT",
        "driver_hourly_stats__avg_daily_trips": "PRESENT",
        "driver_id": "PRESENT"
      }
    },
    {
      "fields": {
        "driver_id": 1002,
        "driver_hourly_stats__avg_daily_trips": 133,
        "driver_hourly_stats__conv_rate": 0.23269200325012207,
        "driver_hourly_stats__acc_rate": 0.5605814456939697
      },
      "statuses": {
        "driver_hourly_stats__avg_daily_trips": "PRESENT",
        "driver_hourly_stats__acc_rate": "PRESENT",
        "driver_id": "PRESENT",
        "driver_hourly_stats__conv_rate": "PRESENT"
      }
    },
    {
      "fields": {
        "driver_hourly_stats__conv_rate": 0.7948746085166931,
        "driver_hourly_stats__avg_daily_trips": 506,
        "driver_id": 1003,
        "driver_hourly_stats__acc_rate": 0.9278625845909119
      },
      "statuses": {
        "driver_hourly_stats__acc_rate": "PRESENT",
        "driver_hourly_stats__avg_daily_trips": "PRESENT",
        "driver_hourly_stats__conv_rate": "PRESENT",
        "driver_id": "PRESENT"
      }
    }
  ]

Which issue(s) this PR fixes:

Fixes #

Does this PR introduce a user-facing change?:

Implement `feast serve` CLI command that starts a local HTTP feature server

@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2021

Codecov Report

Merging #1780 (11acb49) into master (8ef2053) will increase coverage by 0.51%.
The diff coverage is 80.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1780      +/-   ##
==========================================
+ Coverage   84.71%   85.23%   +0.51%     
==========================================
  Files          86       91       +5     
  Lines        6308     6752     +444     
==========================================
+ Hits         5344     5755     +411     
- Misses        964      997      +33     
Flag Coverage Δ
integrationtests 85.15% <80.95%> (+0.49%) ⬆️
unittests 63.96% <80.95%> (-0.35%) ⬇️

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

Impacted Files Coverage Δ
sdk/python/feast/feature_server.py 26.47% <26.47%> (ø)
sdk/python/feast/cli.py 43.95% <50.00%> (+0.27%) ⬆️
sdk/python/feast/proto_json.py 95.45% <95.45%> (ø)
sdk/python/feast/feature_store.py 92.64% <100.00%> (-0.94%) ⬇️
sdk/python/feast/type_map.py 50.83% <100.00%> (ø)
sdk/python/tests/unit/test_proto_json.py 100.00% <100.00%> (ø)
...ion/feature_repos/universal/data_source_creator.py 77.77% <0.00%> (-0.80%) ⬇️
sdk/python/feast/feature_view.py 87.05% <0.00%> (-0.72%) ⬇️
sdk/python/feast/usage.py 60.95% <0.00%> (-0.59%) ⬇️
... and 19 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 8ef2053...11acb49. Read the comment docs.

…id extra structure (e.g. "int64_val") in json

Signed-off-by: Tsotne Tabidze <[email protected]>
@tsotnet
Copy link
Collaborator Author

tsotnet commented Aug 17, 2021

The request API is now much more compact. Here's an example JSON request:

{
        "features": [
            "driver_hourly_stats:conv_rate",
            "driver_hourly_stats:acc_rate",
            "driver_hourly_stats:avg_daily_trips"
        ],
        "entities": {
            "driver_id": [1001, 1002, 1003]
        },
        "full_feature_names": true
    }

I haven't modified the response yet. I'm thinking about making it columnar as well, and making the statuses an optional field (will need to be set manually by user get_statuses=true. @woop @felixwang9817 what do you guys think?

@woop
Copy link
Member

woop commented Aug 17, 2021

The request API is now much more compact. Here's an example JSON request:

{
        "features": [
            "driver_hourly_stats:conv_rate",
            "driver_hourly_stats:acc_rate",
            "driver_hourly_stats:avg_daily_trips"
        ],
        "entities": {
            "driver_id": [1001, 1002, 1003]
        },
        "full_feature_names": true
    }

I haven't modified the response yet. I'm thinking about making it columnar as well, and making the statuses an optional field (will need to be set manually by user get_statuses=true. @woop @felixwang9817 what do you guys think?

Agreed on the above. @tsotnet do we have an idea of the performance impact of using a columnar representation? Ultimately we'll be converting the representation into row level keys in order to do lookups. Can we do it in a way that has little overhead?

Signed-off-by: Tsotne Tabidze <[email protected]>
@tsotnet
Copy link
Collaborator Author

tsotnet commented Aug 19, 2021

Agreed on the above. @tsotnet do we have an idea of the performance impact of using a columnar representation? Ultimately we'll be converting the representation into row level keys in order to do lookups. Can we do it in a way that has little overhead?

I haven't done benchmarks, but I suspect that the savings from the network overhead (due to the smaller request size) will be more significant than columnar to row level in-memory transformation.

Copy link
Collaborator

@felixwang9817 felixwang9817 left a comment

Choose a reason for hiding this comment

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

/lgtm

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: felixwang9817, 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:

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

@feast-ci-bot feast-ci-bot merged commit 745a1b4 into feast-dev:master Aug 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants