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

feat: Switch from Feature to Field #2514

Merged
merged 18 commits into from
Apr 12, 2022
Merged

feat: Switch from Feature to Field #2514

merged 18 commits into from
Apr 12, 2022

Conversation

felixwang9817
Copy link
Collaborator

What this PR does / why we need it: This PR switches FeatureViews to declare features using the Field class and deprecates Feature. It also deprecates positional arguments for on demand feature views, and deprecates the features parameter for feature views and on demand feature views in favor of the schema parameter.

Which issue(s) this PR fixes:

Fixes #

@felixwang9817
Add type annotation
93953d5 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field for FeatureViewProjection
7057016 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field for BaseFeatureView
8de4c6b 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field for RequestFeatureView
b32bea6 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817 felixwang9817 requested a review from a team as a code owner April 9, 2022 02:12
@felixwang9817 felixwang9817 requested review from adchia and removed request for a team April 9, 2022 02:12
@felixwang9817
Enforce kwargs for ODFVs and switch from Feature to Field and from `f…
05f2514 
…eatures` to `schema`

Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field and from features to schema for Feat…
58d02c1 
…ureView

Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Fix references to features and Feature
ba0f12e 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field for ODFV in tests
13c874b 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field in templates
1d1e603 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field in example test repos
1dc0942 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817 felixwang9817 force-pushed the feature_to_field branch 3 times, most recently from 8e51802 to e378145 Compare April 9, 2022 03:05
@codecov-commenter
Copy link

codecov-commenter commented Apr 9, 2022 •
edited
Loading

Codecov Report

Merging #2514 (9bd3761) into master (46b9044) will increase coverage by 0.80%.
The diff coverage is 94.89%.

@@            Coverage Diff             @@
##           master    #2514      +/-   ##
==========================================
+ Coverage   84.43%   85.23%   +0.80%     
==========================================
  Files         134      133       -1     
  Lines       11249    11232      -17     
==========================================
+ Hits         9498     9574      +76     
+ Misses       1751     1658      -93
Flag Coverage Δ
integrationtests 74.09% <91.83%> (-0.11%) ⬇️
unittests 59.68% <62.24%> (+0.49%) ⬆️

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

Impacted Files Coverage Δ
...n/tests/integration/registration/test_inference.py 100.00% <ø> (ø)
...tegration/feature_repos/universal/feature_views.py 83.07% <80.00%> (-0.80%) ⬇️
sdk/python/feast/feature_view_projection.py 96.42% <85.71%> (-3.58%) ⬇️
sdk/python/feast/on_demand_feature_view.py 95.28% <92.10%> (+0.16%) ⬆️
sdk/python/feast/__init__.py 91.66% <100.00%> (+0.75%) ⬆️
sdk/python/feast/base_feature_view.py 80.00% <100.00%> (+0.68%) ⬆️
sdk/python/feast/feature_view.py 85.89% <100.00%> (+0.76%) ⬆️
sdk/python/feast/inference.py 85.91% <100.00%> (+0.62%) ⬆️
sdk/python/feast/infra/provider.py 89.51% <100.00%> (ø)
sdk/python/feast/request_feature_view.py 67.56% <100.00%> (ø)
... and 21 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 46b9044...9bd3761. Read the comment docs.

achals
achals reviewed Apr 9, 2022
View reviewed changes
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.

looks good overall

docs/getting-started/concepts/feature-view.md Show resolved Hide resolved
docs/getting-started/quickstart.md

from feast import Entity, Feature, FeatureView, FileSource, ValueType
from feast import Entity, FeatureView, Field, FileSource, Float32, Int64, ValueType
Copy link
Member

Choose a reason for hiding this comment

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

should we import the types from a different namespace? To keep things better organized? I'm okay with things the way they are for now but just floating the idea.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ah that's a good (non-blocking) idea - I think from feast.typing import Float32, Int64 etc. would be much cleaner

sdk/python/feast/base_feature_view.py
@@ -64,7 +64,7 @@ def __init__(

Args:
name: The unique name of the base feature view.
features: The list of features defined as part of this base feature view.
features (optional): The list of features defined as part of this base feature view.
Copy link
Member

Choose a reason for hiding this comment

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

nit, list of fields? fine with list of features

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

prefer list of features since features here actually refers to features; e.g. I would consider schema a list of fields since it contains not just features but also join keys, but features I consider a list of features

sdk/python/feast/base_feature_view.py
Comment on lines -76 to -77
if not name:
raise ValueError("Name needs to be provided")
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason this was removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's a bit unnecessary since this initialization happens only internally; the check should still happen so I'm going to switch to an assert (since we expect it to always pass)

sdk/python/feast/feature_view.py
Comment on lines +197 to +200
# TODO(felixwang9817): Infer which fields in the schema are features, timestamps,
# and entities. For right now we assume that all fields are features, since the
# current `features` parameter only accepts feature columns.
_features = _schema
Copy link
Member

Choose a reason for hiding this comment

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

Is the intention that schema should always include the timestamps and entities? Or that they can be optionally included, and we'll try to infer and split them if they are in the schema?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the latter - we should continue to support inference

specifically, our current inference logic should be fine - timestamp columns are inferred based on the underlying data sources, entity columns are inferred based on entity join keys, and feature columns are the remaining

@felixwang9817
Switch from Feature to Field in registration integration tests
02cf8af 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field in non-registration integration tests
d5e391b 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field in random tests
57ce182 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field in ui
7f5526f 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field in some universal feature views
901a639 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Switch from Feature to Field in docs
bb2fe9d 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Add assertion that BaseFeatureView is always initialized with a name
f93c844 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
@felixwang9817
Fix imports in docs
2267064 
Signed-off-by: Felix Wang <wangfelix98@gmail.com>
achals
achals approved these changes Apr 12, 2022
View reviewed changes
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

@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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:
  • OWNERS [achals,felixwang9817]

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 feast-ci-bot merged commit 6a03bed into feast-dev:master Apr 12, 2022
@felixwang9817 felixwang9817 mentioned this pull request Apr 12, 2022
Merged
achals pushed a commit that referenced this pull request Apr 14, 2022
@feast-ci-bot
chore(release): release 0.20.0
b2d2cfd 
# [0.20.0](v0.19.0...v0.20.0) (2022-04-14)

### Bug Fixes

* Add inlined data sources to the top level registry ([#2456](#2456)) ([356788a](356788a))
* Add new value types to types.ts for web ui ([#2463](#2463)) ([ad5694e](ad5694e))
* Add PushSource proto and Python class ([#2428](#2428)) ([9a4bd63](9a4bd63))
* Add spark to lambda dockerfile ([#2480](#2480)) ([514666f](514666f))
* Added private_key auth for Snowflake ([#2508](#2508)) ([c42c9b0](c42c9b0))
* Added Redshift and Spark typecheck to data_source event_timestamp_col inference ([#2389](#2389)) ([04dea73](04dea73))
* Building of go extension fails ([#2448](#2448)) ([7d1efd5](7d1efd5))
* Bump the number of versions bumps expected to 27 ([#2549](#2549)) ([ecc9938](ecc9938))
* Create __init__ files for the proto-generated python dirs ([#2410](#2410)) ([e17028d](e17028d))
* Don't prevent apply from running given duplicate empty names in data sources. Also fix repeated apply of Spark data source. ([#2415](#2415)) ([b95f441](b95f441))
* Dynamodb deduplicate batch write request by partition keys ([#2515](#2515)) ([70d4a13](70d4a13))
* Ensure that __init__ files exist in proto dirs ([#2433](#2433)) ([9b94f7b](9b94f7b))
* Fix DataSource constructor to unbreak custom data sources ([#2492](#2492)) ([712653e](712653e))
* Fix default feast apply path without any extras ([#2373](#2373)) ([6ba7fc7](6ba7fc7))
* Fix definitions.py with new definition ([#2541](#2541)) ([eefc34a](eefc34a))
* Fix entity row to use join key instead of name ([#2521](#2521)) ([c22fa2c](c22fa2c))
* Fix Java Master ([#2499](#2499)) ([e083458](e083458))
* Fix registry proto ([#2435](#2435)) ([ea6a9b2](ea6a9b2))
* Fix some inconsistencies in the docs and comments in the code ([#2444](#2444)) ([ad008bf](ad008bf))
* Fix spark docs ([#2382](#2382)) ([d4a606a](d4a606a))
* Fix Spark template to work correctly on feast init -t spark ([#2393](#2393)) ([ae133fd](ae133fd))
* Fix the feature repo fixture used by java tests  ([#2469](#2469)) ([32e925e](32e925e))
* Fix unhashable Snowflake and Redshift sources ([cd8f1c9](cd8f1c9))
* Fixed bug in passing config file params to snowflake python connector ([#2503](#2503)) ([34f2b59](34f2b59))
* Fixing Spark template to include source name ([#2381](#2381)) ([a985f1d](a985f1d))
* Make name a keyword arg for the Entity class ([#2467](#2467)) ([43847de](43847de))
* Making a name for data sources not a breaking change ([#2379](#2379)) ([71d7ae2](71d7ae2))
* Minor link fix in `CONTRIBUTING.md` ([#2481](#2481)) ([2917e27](2917e27))
* Preserve ordering of features in _get_column_names ([#2457](#2457)) ([495b435](495b435))
* Relax click python requirement to >=7 ([#2450](#2450)) ([f202f92](f202f92))
* Remove date partition column field from datasources that don't s… ([#2478](#2478)) ([ce35835](ce35835))
* Remove docker step from unit test workflow ([#2535](#2535)) ([6f22f22](6f22f22))
* Remove spark from the AWS Lambda dockerfile ([#2498](#2498)) ([6abae16](6abae16))
* Request data api update ([#2488](#2488)) ([0c9e5b7](0c9e5b7))
* Schema update ([#2509](#2509)) ([cf7bbc2](cf7bbc2))
* Simplify DataSource.from_proto logic ([#2424](#2424)) ([6bda4d2](6bda4d2))
* Snowflake api update ([#2487](#2487)) ([1181a9e](1181a9e))
* Support passing batch source to streaming sources for backfills ([#2523](#2523)) ([90db1d1](90db1d1))
* Timestamp update ([#2486](#2486)) ([bf23111](bf23111))
* Typos in Feast UI error message ([#2432](#2432)) ([e14369d](e14369d))
* Update feature view APIs to prefer keyword args ([#2472](#2472)) ([7c19cf7](7c19cf7))
* Update file api ([#2470](#2470)) ([83a11c6](83a11c6))
* Update Makefile to cd into python dir before running commands ([#2437](#2437)) ([ca32155](ca32155))
* Update redshift api ([#2479](#2479)) ([4fa73a9](4fa73a9))
* Update some fields optional in UI parser ([#2380](#2380)) ([cff7ac3](cff7ac3))
* Use a single version of jackson libraries and upgrade to 2.12.6.1 ([#2473](#2473)) ([5be1cc6](5be1cc6))
* Use dateutil parser to parse materialization times ([#2464](#2464)) ([6c55e49](6c55e49))
* Use the correct dockerhub image tag when building feature servers ([#2372](#2372)) ([0d62c1d](0d62c1d))

### Features

* Add `/write-to-online-store` method to the python feature server ([#2423](#2423)) ([d2fb048](d2fb048))
* Add description, tags, owner fields to all feature view classes ([#2440](#2440)) ([ed5e928](ed5e928))
* Add DQM Logging on GRPC Server with FileLogStorage for Testing ([#2403](#2403)) ([57a97d8](57a97d8))
* Add Feast types in preparation for changing type system ([#2475](#2475)) ([4864252](4864252))
* Add Field class ([#2500](#2500)) ([1279612](1279612))
* Add support for DynamoDB online_read in batches ([#2371](#2371)) ([702ec49](702ec49))
* Add Support for DynamodbOnlineStoreConfig endpoint_url parameter ([#2485](#2485)) ([7b863d1](7b863d1))
* Add templating for dynamodb table name ([#2394](#2394)) ([f591088](f591088))
* Allow local feature server to use Go feature server if enabled ([#2538](#2538)) ([a2ef375](a2ef375))
* Allow using entity's join_key in get_online_features ([#2420](#2420)) ([068c765](068c765))
* Data Source Api Update ([#2468](#2468)) ([6b96b21](6b96b21))
* Go server ([#2339](#2339)) ([d12e7ef](d12e7ef)), closes [#2354](#2354) [#2361](#2361) [#2332](#2332) [#2356](#2356) [#2363](#2363) [#2349](#2349) [#2355](#2355) [#2336](#2336) [#2361](#2361) [#2363](#2363) [#2344](#2344) [#2354](#2354) [#2347](#2347) [#2350](#2350) [#2356](#2356) [#2355](#2355) [#2349](#2349) [#2352](#2352) [#2341](#2341) [#2336](#2336) [#2373](#2373) [#2315](#2315) [#2372](#2372) [#2332](#2332) [#2349](#2349) [#2336](#2336) [#2361](#2361) [#2363](#2363) [#2344](#2344) [#2354](#2354) [#2347](#2347) [#2350](#2350) [#2356](#2356) [#2355](#2355) [#2349](#2349) [#2352](#2352) [#2341](#2341) [#2336](#2336) [#2373](#2373) [#2379](#2379) [#2380](#2380) [#2382](#2382) [#2364](#2364) [#2366](#2366) [#2386](#2386)
* Graduate write_to_online_store out of experimental status ([#2426](#2426)) ([e7dd4b7](e7dd4b7))
* Make feast PEP 561 compliant ([#2405](#2405)) ([3c41f94](3c41f94)), closes [#2420](#2420) [#2418](#2418) [#2425](#2425) [#2426](#2426) [#2427](#2427) [#2431](#2431) [#2433](#2433) [#2420](#2420) [#2418](#2418) [#2425](#2425) [#2426](#2426) [#2427](#2427) [#2431](#2431) [#2433](#2433)
* Makefile for contrib for Issue [#2364](#2364) ([#2366](#2366)) ([a02325b](a02325b))
* Support on demand feature views in go feature server ([#2494](#2494)) ([6edd274](6edd274))
* Switch from Feature to Field ([#2514](#2514)) ([6a03bed](6a03bed))
* Use a daemon thread to monitor the go feature server exclusively ([#2391](#2391)) ([0bb5e8c](0bb5e8c))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@achals achals achals approved these changes

@adchia adchia Awaiting requested review from adchia adchia was automatically assigned from feast-dev/maintainers

Assignees

@achals achals

Labels
approved lgtm size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@felixwang9817 @codecov-commenter @feast-ci-bot @achals