-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
wintonzheng
commented
Oct 24, 2023
- Does this PR have impact on local development experience? If yes, make sure you have a plan and add the documentations to address issues that come with the change
- bump version
- make a release
- publish to pypi service
5a8d175
to
62c951b
Compare
@@ -456,6 +460,140 @@ async def get_historical_features( | |||
results=final_df.to_dict(orient="records"), | |||
) | |||
|
|||
@app.post(f"{settings.WYVERN_HISTORICAL_FEATURES_PATH}_v2") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the version here follow the standard api/v2/get_historical_features
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes we can do f"v2/{settings.WYVERN_HISTORICAL_FEATURES_PATH}"
Also:
this API path is just a placeholder. eventually it should replace the original API.
And this service is the feature store service, not the pipeline service. so it doesn't follow the api/vX/
pattern
# ) | ||
|
||
# Generate a 10-digit hex for the request | ||
hex_id = secrets.token_hex(5) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's a hex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's just a random hexadecimal number, which is part of the temporary/transient table name we're generating
|
||
# TODO: valid entity_name_1 and entity_name_2 are in the table columns | ||
# if entity_name_1 not in data.entities: | ||
# logger.warning( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these should be 400 Bad Request responses
18fa387
to
daa1e21
Compare
daa1e21
to
f879950
Compare
a255155
to
8aa6779
Compare
FROM {data.table} | ||
""" | ||
snowflake_ctx = generate_snowflake_ctx() | ||
snowflake_ctx.cursor().execute(select_sql) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we handle some errors and add retry logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure when we should retry tho. Let's start with busting the API if there's any error. most likely error comes from bad data in the input table (entity table), which probably wont' be fixed by retry.
let's launch this version without error handling and start catching issues with matias' data.
@@ -117,6 +118,100 @@ def build_historical_real_time_feature_requests( | |||
return result_dict | |||
|
|||
|
|||
def build_and_merge_realtime_pivot_tables( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should create a one pager that explains the logic clearly with examples. we can even get sth together using the stuff we went over to clarify the logic
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.wyvern.ai/training_data
Maybe here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point. i'll write something after i test this endpoint