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.

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

Jump to bottom

Add historical retrieval for BigQuery and Parquet #1389

Merged
merged 29 commits into from
Mar 16, 2021
Merged

Add historical retrieval for BigQuery and Parquet #1389

merged 29 commits into from
Mar 16, 2021

Conversation

woop
Copy link
Member

@woop woop commented Mar 14, 2021 •
edited
Loading

What this PR does / why we need it:

  • Adds support for file (Parquet) based historical retrieval
  • Adds support for BigQuery based historical retrieval

Limitations

  • The original SQL query to generate a training dataset was executed as a multi-job query that produced tables. This query does not produce tables, but it will probably run out of memory at some point. It needs to be optimized eventually.
  • We are not applying time range filters prior to doing joins.
  • Async (RetrievalJob) behaviour isn't consistent across stores (Parquet, BigQuery). Sometimes work only starts when a user runs (to_df()), other times it starts when get_historical_features() is run.
  • apply() has been added but doesnt yet provision infrastructure. Considering that functionality out of scope for this PR.

Does this PR introduce a user-facing change?:

Added historical retrieval for file and BigQuery sources

@woop woop requested a review from jklegar as a code owner March 14, 2021 05:25
@woop woop added kind/feature New feature or request and removed size/XXL labels Mar 14, 2021
@woop woop changed the title Add historical retrieval for BigQuery and Parquet [WIP] Add historical retrieval for BigQuery and Parquet Mar 14, 2021
@woop woop force-pushed the add-offline-historical-retrieval branch 4 times, most recently from c2c3735 to d1e9fa6 Compare March 15, 2021 03:43
@woop woop changed the title [WIP] Add historical retrieval for BigQuery and Parquet Add historical retrieval for BigQuery and Parquet Mar 15, 2021
@woop woop requested a review from oavdeev March 15, 2021 22:52
oavdeev
oavdeev approved these changes Mar 16, 2021
View reviewed changes
sdk/python/feast/offline_store.py Outdated Show resolved Hide resolved
sdk/python/feast/offline_store.py Outdated


class FileRetrievalJob(RetrievalJob):
def __init__(self, evaluation_function):
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd add a type signature for evaluation_function here, and a comment explaining what it is for.

sdk/python/feast/offline_store.py Outdated
"""RetrievalJob is used to manage the execution of a historical feature retrieval"""

def __init__(self):
pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

does it need a constructor?

sdk/python/feast/offline_store.py Outdated Show resolved Hide resolved
@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oavdeev, woop

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

@woop woop force-pushed the add-offline-historical-retrieval branch from 9c00d3b to 83f173c Compare March 16, 2021 02:29
@woop
Add ParquetSource
dc4906b 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Add Parquet source, TTL validation, and fix BQ source conditional
2c465fe 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Add partial Parquet Historical retrieval
5b1c584 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Reformat and lint
16b716d 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Remove feature_views_to_query from get_historical_features
b93cba5 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Clean up get_hist_features test
20576b5 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Added point-in-time query for BigQueryOfflineStore
9a5bc12 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Fix ParquetSource tests
785af1f 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Clean up and reactor offline_store.py functions
940c2ae 
Signed-off-by: Willem Pienaar <[email protected]>
woop and others added 20 commits March 15, 2021 19:29
@woop
Relax Python dependencies and fix broken TTL
fe3e9c0 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Fix time zones in point-in-time join
b2f30e5 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Reformat using black
8d5c650 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Move get_historical_features into feature_store and get mypy to pass
c6da3cc 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Replace ParquetSource with FileSource
9f8d0d9 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Fix feature request building
cad7cb5 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Clean up context building code
00a48da 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Remove RegistryState Class
c64e99f 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Fix tests writing to disk
b9c1e90 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Skip BigQuery test during unit tests
93ad2d2 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Revert documentation changes
dfc811b 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Add documentation for historical retrieval and apply
513bcff 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Add support for loading entity dataframes
3aae713 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Fix typo in query context
aba08e1 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Remove hardcoding of created timestamp
354c17e 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Set created timestamp column in BigQuery test
0240ef1 
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Revert broken documentation and formatting
9cd335d 
Signed-off-by: Willem Pienaar <[email protected]>
@woop @oavdeev
Set context query to be immutable
82595f8 
Co-authored-by: Oleg Avdeev <[email protected]>
Signed-off-by: Willem Pienaar <[email protected]>
@woop
Add comments based on PR review
2468261 
Signed-off-by: Willem Pienaar <[email protected]>
@woop @oavdeev
Clarify TODO comment for entity dataframe time ranges
83f173c 
Co-authored-by: Oleg Avdeev <[email protected]>
Signed-off-by: Willem Pienaar <[email protected]>
@oavdeev
Copy link
Collaborator

oavdeev commented Mar 16, 2021

/lgtm

@feast-ci-bot feast-ci-bot merged commit a31545b into feast-dev:master Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oavdeev oavdeev oavdeev approved these changes

@jklegar jklegar Awaiting requested review from jklegar

Assignees

@oavdeev oavdeev

Labels
approved kind/feature New feature or request lgtm release-note size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@woop @feast-ci-bot @oavdeev