Historical retrieval without an entity dataframe (Local Only) #1690

HeardACat · 2021-07-05T21:55:28Z

What this PR does / why we need it:

Adds Historical retrieval without an entity dataframe as per #1611

This is experimental

Implements #1611 for Local Provider only.

Which issue(s) this PR fixes:

Partially adds #1611

Does this PR introduce a user-facing change?:

Add historical retrieval without an entity dataframe for file-based offline store

feast-ci-bot · 2021-07-05T21:55:29Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: charliec443
To complete the pull request process, please assign jklegar after the PR has been reviewed.
You can assign the PR to them by writing /assign @jklegar in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

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 · 2021-07-05T21:55:36Z

Hi @charliec443. Thanks for your PR.

I'm waiting for a feast-dev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

HeardACat · 2021-07-05T21:59:53Z

sdk/python/feast/infra/offline_stores/file.py

+                raise FeatureViewNotFoundException(entity_df)
+
+        entity_df_event_timestamp_col = DEFAULT_ENTITY_DF_EVENT_TIMESTAMP_COL  # local modifiable copy of global variable
+        if entity_df_event_timestamp_col not in entity_df.columns:


The only time we "visit" this code is if the entity_df provided is a pd.DataFrame - should we exclude this from happening? Or should the method be renamed.

woop · 2021-07-06T01:09:54Z

sdk/python/feast/feature_store.py

+                end_date,
+            )
+        except FeastProviderLoginError as e:
+            sys.exit(e)


I think we should avoid using sys.exit(e) in general. The current implementation of get_historical_features() is wrong and will be changed. The exit command will also exit the wrapping process.

I'm guessing this is related to #1666

codecov-commenter · 2021-07-06T01:13:20Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 85.88235% with 12 lines in your changes missing coverage. Please review.

Project coverage is 69.69%. Comparing base (8b3a97a) to head (d10c66a).
Report is 1676 commits behind head on master.

Files with missing lines	Patch %	Lines
sdk/python/feast/infra/offline_stores/file.py	75.00%	8 Missing ⚠️
sdk/python/feast/infra/offline_stores/bigquery.py	66.66%	1 Missing ⚠️
...python/feast/infra/offline_stores/offline_store.py	75.00%	1 Missing ⚠️
sdk/python/feast/infra/offline_stores/redshift.py	66.66%	1 Missing ⚠️
sdk/python/feast/infra/provider.py	66.66%	1 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

❗ There is a different number of reports uploaded between BASE (8b3a97a) and HEAD (d10c66a). Click for more details.

HEAD has 3 uploads less than BASE

Flag BASE (8b3a97a) HEAD (d10c66a)

integrationtests 3 0

Additional details and impacted files

@@             Coverage Diff             @@
##           master    #1690       +/-   ##
===========================================
- Coverage   84.46%   69.69%   -14.78%     
===========================================
  Files          82       80        -2     
  Lines        7093     7087        -6     
===========================================
- Hits         5991     4939     -1052     
- Misses       1102     2148     +1046

Flag	Coverage Δ
integrationtests	`?`
unittests	`69.69% <85.88%> (+0.19%)`	⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

woop · 2021-07-11T17:50:11Z

Thanks @charliec443, this is great. We might only get to a review a bit later this week, but intending to have a closer look.

sdk/python/feast/infra/offline_stores/file.py

woop · 2021-07-16T22:44:32Z

sdk/python/feast/infra/offline_stores/file.py

+                f"Please provide an entity_df of type {type(pd.DataFrame)} or {type(str)} instead of type {type(entity_df)}"
+            )
+
+        if isinstance(entity_df, str):


Are you assuming that a str is a reference to a feature view? What if the user provides a SQL query?

Is a SQL query relevant for file-based offline stores?

Is a SQL query relevant for file-based offline stores?

It isn't, but what happens when we generalize this code to other feature stores? Are you suggesting we'd have different APIs for different offline stores?

It could be a query. For the other offline stores the parameter could be "entity_df_or_query"? I'm rather indifferent towards this.

Happy to go with any approach you think is sensible.

I think what is confusing to me is why get_historical_features_by_view even supports Pandas/Query if it's by view. What if we just supported a string reference (or even an object reference) to a feature view, and nothing else?

Yes, let's do that. It removes the ambiguity.

HeardACat · 2021-07-17T00:14:36Z

rebase is driving me nuts
trying to fix the DCO error caused cascading rebase conflicts - just going to do a git push -f with only the changes.
If viewing history is important then try here: https://github.com/charliec443/feast/tree/history_str_view2

woop · 2021-07-20T01:22:37Z

rebase is driving me nuts
trying to fix the DCO error caused cascading rebase conflicts - just going to do a git push -f with only the changes.
If viewing history is important then try here: https://github.com/charliec443/feast/tree/history_str_view2

Sorry about that. We are busy with a lot of refactoring right now. Feel free to blow away your history and just provide one commit.

Signed-off-by: CS <2498638+charliec443@users.noreply.github.com>

achals · 2022-04-12T20:27:58Z

Hi @charliec443 , are you still planning on working on this PR? Is there anything we can do to help?

HeardACat requested review from achals, tsotnet, woop and a team as code owners July 5, 2021 21:55

feast-ci-bot added do-not-merge/release-note-label-needed needs-kind labels Jul 5, 2021

feast-ci-bot added needs-ok-to-test size/L labels Jul 5, 2021

HeardACat commented Jul 5, 2021

View reviewed changes

HeardACat force-pushed the history_str_view branch 2 times, most recently from 5c8f2db to 0b53477 Compare July 5, 2021 22:34

woop reviewed Jul 6, 2021

View reviewed changes

HeardACat force-pushed the history_str_view branch from 780a602 to 7688a71 Compare July 7, 2021 01:26

feast-ci-bot added release-note and removed do-not-merge/release-note-label-needed labels Jul 7, 2021

HeardACat force-pushed the history_str_view branch from 06415f9 to 0c99eeb Compare July 9, 2021 01:22

HeardACat marked this pull request as draft July 9, 2021 01:28

feast-ci-bot added the do-not-merge/work-in-progress label Jul 9, 2021

HeardACat force-pushed the history_str_view branch from 48e02ff to 8dfbce9 Compare July 9, 2021 01:35

HeardACat marked this pull request as ready for review July 9, 2021 01:47

feast-ci-bot removed the do-not-merge/work-in-progress label Jul 9, 2021

woop removed request for achals and tsotnet July 16, 2021 21:38

woop reviewed Jul 16, 2021

View reviewed changes

sdk/python/feast/infra/offline_stores/file.py Outdated Show resolved Hide resolved

woop reviewed Jul 16, 2021

View reviewed changes

HeardACat force-pushed the history_str_view branch from 8562ee2 to 6eee18f Compare July 16, 2021 23:35

feast-ci-bot added size/XXL size/L and removed size/L size/XXL labels Jul 16, 2021

HeardACat force-pushed the history_str_view branch 2 times, most recently from 3243480 to abe5aba Compare July 16, 2021 23:45

HeardACat force-pushed the history_str_view branch from abe5aba to c7c8bff Compare July 17, 2021 00:15

initial implementation

d10c66a

Signed-off-by: CS <2498638+charliec443@users.noreply.github.com>

HeardACat force-pushed the history_str_view branch from 5e8053e to d10c66a Compare July 20, 2021 18:02

felixwang9817 self-assigned this Sep 20, 2021

pyalex force-pushed the master branch from 0b4bd12 to c65865e Compare March 26, 2022 02:13

adchia unassigned felixwang9817 Apr 15, 2022

adchia linked an issue Apr 21, 2022 that may be closed by this pull request

Need to get objects from FeatureStore without prior knowledge of their IDs and timestamps... #1989

Closed

adchia removed a link to an issue Apr 21, 2022

Need to get objects from FeatureStore without prior knowledge of their IDs and timestamps... #1989

Closed

adchia closed this Jul 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Historical retrieval without an entity dataframe (Local Only) #1690

Historical retrieval without an entity dataframe (Local Only) #1690

HeardACat commented Jul 5, 2021 •

edited

Loading

feast-ci-bot commented Jul 5, 2021

feast-ci-bot commented Jul 5, 2021

HeardACat Jul 5, 2021

woop Jul 6, 2021

HeardACat Jul 6, 2021

codecov-commenter commented Jul 6, 2021 •

edited

Loading

woop commented Jul 11, 2021

woop Jul 16, 2021

HeardACat Jul 16, 2021

woop Jul 19, 2021

HeardACat Jul 19, 2021

woop Jul 20, 2021

HeardACat Jul 20, 2021

HeardACat commented Jul 17, 2021 •

edited

Loading

woop commented Jul 20, 2021

achals commented Apr 12, 2022

Historical retrieval without an entity dataframe (Local Only) #1690

Historical retrieval without an entity dataframe (Local Only) #1690

Conversation

HeardACat commented Jul 5, 2021 • edited Loading

feast-ci-bot commented Jul 5, 2021

feast-ci-bot commented Jul 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 6, 2021 • edited Loading

Codecov Report

woop commented Jul 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HeardACat commented Jul 17, 2021 • edited Loading

woop commented Jul 20, 2021

achals commented Apr 12, 2022

HeardACat commented Jul 5, 2021 •

edited

Loading

codecov-commenter commented Jul 6, 2021 •

edited

Loading

HeardACat commented Jul 17, 2021 •

edited

Loading