Skip to content

feat: Add api contract to fastapi docs #4721

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
HaoXuAI merged 3 commits into from
Nov 5, 2024
Merged

feat: Add api contract to fastapi docs #4721

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ad98954
use pydantic models to populate fastapi docs
robhowley Oct 30, 2024
1981a87
fix feature service feature assignment
robhowley Oct 30, 2024
720b6bf
add response types, mostly none
robhowley Oct 30, 2024
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
64 changes: 32 additions & 32 deletions sdk/python/feast/feature_server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import json
import sys
import threading
import time
import traceback
from contextlib import asynccontextmanager
from typing import List, Optional
from typing import Any, Dict, List, Optional

import pandas as pd
import psutil
Expand Down Expand Up @@ -69,6 +68,13 @@ class MaterializeIncrementalRequest(BaseModel):
feature_views: Optional[List[str]] = None


class GetOnlineFeaturesRequest(BaseModel):
entities: Dict[str, List[Any]]
feature_service: Optional[str] = None
features: Optional[List[str]] = None
Comment on lines +73 to +74
Copy link
Contributor Author

@robhowley robhowley Oct 30, 2024

Choose a reason for hiding this comment

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

could (should) add validation on the model that throws if both are set. didnt do it here, bc it would be technically breaking. the code works just picks the feature_service value and ignore features. this is an unintuitive (successful) result, seems like it would make more sense to return 422 and say, "gotta pick one". maybe flag it for 1.0? @franciscojavierarceo

full_feature_names: bool = False


def get_app(
store: "feast.FeatureStore",
registry_ttl_sec: int = DEFAULT_FEATURE_SERVER_REGISTRY_TTL,
Expand Down Expand Up @@ -108,33 +114,26 @@ async def lifespan(app: FastAPI):

app = FastAPI(lifespan=lifespan)

async def get_body(request: Request):
return await request.body()

@app.post(
"/get-online-features",
dependencies=[Depends(inject_user_details)],
)
async def get_online_features(body=Depends(get_body)):
body = json.loads(body)
full_feature_names = body.get("full_feature_names", False)
entity_rows = body["entities"]
async def get_online_features(request: GetOnlineFeaturesRequest) -> Dict[str, Any]:
Comment on lines -119 to +121
Copy link
Contributor Author

@robhowley robhowley Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

fastapi is smart about types. surfaces pydantic model in the autogenerated api docs. this also gives us schema validation on requests for free. for example

image

# Initialize parameters for FeatureStore.get_online_features(...) call
if "feature_service" in body:
if request.feature_service:
feature_service = store.get_feature_service(
body["feature_service"], allow_cache=True
request.feature_service, allow_cache=True
)
assert_permissions(
resource=feature_service, actions=[AuthzedAction.READ_ONLINE]
)
features = feature_service
features = feature_service # type: ignore
else:
features = body["features"]
all_feature_views, all_on_demand_feature_views = (
utils._get_feature_views_to_use(
store.registry,
store.project,
features,
request.features,
allow_cache=True,
hide_dummy_entity=False,
)
Expand All @@ -147,18 +146,19 @@ async def get_online_features(body=Depends(get_body)):
assert_permissions(
resource=od_feature_view, actions=[AuthzedAction.READ_ONLINE]
)
features = request.features # type: ignore

read_params = dict(
features=features,
entity_rows=entity_rows,
full_feature_names=full_feature_names,
entity_rows=request.entities,
full_feature_names=request.full_feature_names,
)

if store._get_provider().async_supported.online.read:
response = await store.get_online_features_async(**read_params)
response = await store.get_online_features_async(**read_params) # type: ignore
else:
response = await run_in_threadpool(
lambda: store.get_online_features(**read_params)
lambda: store.get_online_features(**read_params) # type: ignore
)

# Convert the Protobuf object to JSON and return it
Expand All @@ -167,8 +167,7 @@ async def get_online_features(body=Depends(get_body)):
)

@app.post("/push", dependencies=[Depends(inject_user_details)])
async def push(body=Depends(get_body)):
request = PushFeaturesRequest(**json.loads(body))
async def push(request: PushFeaturesRequest) -> None:
df = pd.DataFrame(request.df)
actions = []
if request.to == "offline":
Expand Down Expand Up @@ -220,17 +219,16 @@ async def push(body=Depends(get_body)):
store.push(**push_params)

@app.post("/write-to-online-store", dependencies=[Depends(inject_user_details)])
def write_to_online_store(body=Depends(get_body)):
request = WriteToFeatureStoreRequest(**json.loads(body))
def write_to_online_store(request: WriteToFeatureStoreRequest) -> None:
df = pd.DataFrame(request.df)
feature_view_name = request.feature_view_name
allow_registry_cache = request.allow_registry_cache
try:
feature_view = store.get_stream_feature_view(
feature_view = store.get_stream_feature_view( # type: ignore
feature_view_name, allow_registry_cache=allow_registry_cache
)
except FeatureViewNotFoundException:
feature_view = store.get_feature_view(
feature_view = store.get_feature_view( # type: ignore
feature_view_name, allow_registry_cache=allow_registry_cache
)

Expand All @@ -250,11 +248,12 @@ async def health():
)

@app.post("/materialize", dependencies=[Depends(inject_user_details)])
def materialize(body=Depends(get_body)):
request = MaterializeRequest(**json.loads(body))
for feature_view in request.feature_views:
def materialize(request: MaterializeRequest) -> None:
for feature_view in request.feature_views or []:
# TODO: receives a str for resource but isn't in the Union. is str actually allowed?
Copy link
Contributor

@dmartinol dmartinol Oct 31, 2024

Choose a reason for hiding this comment

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

it's not allowed, so we should fetch the feature_view instance as we do in the write-to-online-store endpoint.
let's open a GH issue for that, unless you want to fix it here

Copy link
Contributor

@dmartinol dmartinol Oct 31, 2024

Choose a reason for hiding this comment

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

BTW: thanks for catching it!

Copy link
Contributor Author

@robhowley robhowley Oct 31, 2024

Choose a reason for hiding this comment

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

nah, not here. that should be a sep issue and fix: pr

Copy link
Contributor

@dmartinol dmartinol Oct 31, 2024

Choose a reason for hiding this comment

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

#4726

Copy link
Contributor Author

@robhowley robhowley Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i'll take care of #4726 once this pr is merged. see draft here; will rebase and mark as ready for review.

assert_permissions(
resource=feature_view, actions=[AuthzedAction.WRITE_ONLINE]
resource=feature_view, # type: ignore
actions=[AuthzedAction.WRITE_ONLINE],
)
store.materialize(
utils.make_tzaware(parser.parse(request.start_ts)),
Expand All @@ -263,11 +262,12 @@ def materialize(body=Depends(get_body)):
)

@app.post("/materialize-incremental", dependencies=[Depends(inject_user_details)])
def materialize_incremental(body=Depends(get_body)):
request = MaterializeIncrementalRequest(**json.loads(body))
for feature_view in request.feature_views:
def materialize_incremental(request: MaterializeIncrementalRequest) -> None:
for feature_view in request.feature_views or []:
# TODO: receives a str for resource but isn't in the Union. is str actually allowed?
assert_permissions(
resource=feature_view, actions=[AuthzedAction.WRITE_ONLINE]
resource=feature_view, # type: ignore
actions=[AuthzedAction.WRITE_ONLINE],
)
store.materialize_incremental(
utils.make_tzaware(parser.parse(request.end_ts)), request.feature_views
Expand Down