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

Enable entityless featureviews #1804

Merged
merged 34 commits into from
Sep 9, 2021

Conversation

codyjlin
Copy link
Contributor

@codyjlin codyjlin commented Aug 26, 2021

Signed-off-by: Cody Lin codyl@twitter.com

What this PR does / why we need it:
This PR executes on the Entityless FeatureViews RFC, with minor adjustments to Implementation 1:

  1. Registry
    1. Create __entityless entity on every apply
    2. Attach it to any entityless FVs when they are initialized
    3. Hide it from registry_dump (namely, list_entities and list_featureviews)
  2. Offline
    1. For each of the offline store providers, update the joins to be able to join with no entities (only the timestamp col)
    2. (I think this implementation detail differs from the RFC - I think it is much simpler than having to create the extra __entityless_id dummy column to join on a constant value, and then having to hide it later when selecting columns)
  3. Materialization/Online
    1. Add __entityless_id column as empty string upon materialize (namely, pull_latest_from_table_or_query in each offline store provider)
    2. If get_online_features requests any features from an entityless FV, {"__entityless_id": ""} will be a join_key_row to be converted to a entity_row_proto before look up in the online store.

Which issue(s) this PR fixes:
Fixes #1737

Does this PR introduce a user-facing change?:

The user can now specify FeatureViews with an empty `entities` list.

@feast-ci-bot
Copy link
Collaborator

Hi @codyjlin. 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.

@codecov-commenter
Copy link

codecov-commenter commented Aug 26, 2021

Codecov Report

Merging #1804 (e4bde7c) into master (b0f38ad) will increase coverage by 0.38%.
The diff coverage is 94.79%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1804      +/-   ##
==========================================
+ Coverage   84.13%   84.51%   +0.38%     
==========================================
  Files          90       90              
  Lines        6770     6827      +57     
==========================================
+ Hits         5696     5770      +74     
+ Misses       1074     1057      -17     
Flag Coverage Δ
integrationtests 84.44% <94.79%> (+0.38%) ⬆️
unittests 63.82% <65.62%> (+0.33%) ⬆️

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

Impacted Files Coverage Δ
sdk/python/feast/repo_operations.py 44.58% <0.00%> (+0.13%) ⬆️
...tegration/feature_repos/universal/feature_views.py 95.83% <80.00%> (-4.17%) ⬇️
sdk/python/feast/feature_store.py 93.14% <97.22%> (+0.17%) ⬆️
sdk/python/feast/driver_test_data.py 100.00% <100.00%> (ø)
sdk/python/feast/feature_view.py 86.57% <100.00%> (+0.27%) ⬆️
sdk/python/feast/infra/offline_stores/bigquery.py 82.06% <100.00%> (ø)
sdk/python/feast/infra/offline_stores/file.py 97.43% <100.00%> (+0.09%) ⬆️
sdk/python/feast/infra/offline_stores/redshift.py 90.99% <100.00%> (ø)
sdk/python/feast/infra/provider.py 88.98% <100.00%> (ø)
sdk/python/feast/online_response.py 95.00% <100.00%> (+0.12%) ⬆️
... and 14 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 b0f38ad...e4bde7c. Read the comment docs.

@codyjlin codyjlin changed the title Iniitial commit for entityless featureviews Enable entityless featureviews Aug 27, 2021
@codyjlin
Copy link
Contributor Author

codyjlin commented Sep 3, 2021

/kind feature

@feast-ci-bot feast-ci-bot added kind/feature New feature or request and removed needs-kind labels Sep 3, 2021
Signed-off-by: Cody Lin <codyl@twitter.com>
Cody Lin and others added 2 commits September 3, 2021 14:45
Signed-off-by: Cody Lin <codyl@twitter.com>
sdk/python/feast/driver_test_data.py Show resolved Hide resolved
sdk/python/feast/feature_store.py Outdated Show resolved Hide resolved
sdk/python/feast/feature_view.py Outdated Show resolved Hide resolved
sdk/python/tests/conftest.py Outdated Show resolved Hide resolved
sdk/python/tests/integration/e2e/test_universal_e2e.py Outdated Show resolved Hide resolved
sdk/python/tests/conftest.py Outdated Show resolved Hide resolved
Signed-off-by: Cody Lin <codyl@twitter.com>
Signed-off-by: Cody Lin <codyl@twitter.com>
Copy link
Member

@achals achals left a comment

Choose a reason for hiding this comment

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

/lgtm

sdk/python/feast/feature_store.py Show resolved Hide resolved
sdk/python/usage_tests/test_usage.py Show resolved Hide resolved
@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, codyjlin

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

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.

Support entityless features
7 participants