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

Set default feature naming to not include feature view name. Add option to include feature view name in feature naming. #1641

Merged
merged 44 commits into from
Jul 8, 2021

Conversation

Mwad22
Copy link
Contributor

@Mwad22 Mwad22 commented Jun 11, 2021

What this PR does / why we need it:
Provides a way to strip the feature_view__ prefix from feature names during retrieval and reuse feature names directly from sources. This is done by adding a feature_names_only arguments to the functions get_historical_features(), and get_online_features(). By default, we set this parameter to True, as that is what most users seem to prefer (per guidance given by @woop).

Which issue(s) this PR fixes:

Fixes #1618

Does this PR introduce a user-facing change?:

Yes, the default behaviour of calling get_historical_features() and get_online_features() now return feature names that are stripped of the feature_view__ prefix. This introduces the possibility of feature name collisions (in which case a descriptive error is thrown, informing users they can switch to the original behaviour by setting the full_feature_names=True in their retrieval function).

Added option to include feature view name as prefix in feature naming convention.
Default naming convention is now of format "feature_name", with option to name
 features in the format "featureview__feature_name" by setting the 
**full_feature_names=True** argument in get_historical_features() and get_online_features()

@Mwad22 Mwad22 requested review from achals, tsotnet, woop and a team as code owners June 11, 2021 15:36
@Mwad22 Mwad22 changed the title Mwad22 1618 PR option to strip feature view names from feature names Jun 11, 2021
@Mwad22 Mwad22 changed the title option to strip feature view names from feature names Add option to strip feature view names from feature names Jun 11, 2021
.gitignore Outdated Show resolved Hide resolved
@woop
Copy link
Member

woop commented Jun 11, 2021

Thanks for this @Mwad22!

@Mwad22 Mwad22 changed the title Add option to strip feature view names from feature names Set default feature naming to not include feature view name. Add option to include feature view name in feature naming. Jun 11, 2021
pyvenv.cfg Outdated Show resolved Hide resolved
Mwad22 and others added 5 commits July 7, 2021 17:40
Co-authored-by: Willem Pienaar <6728866+woop@users.noreply.github.com>
Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.com>
Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.com>
Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.com>
Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.com>
Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.com>
Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.com>
@Mwad22
Copy link
Contributor Author

Mwad22 commented Jul 7, 2021

@Mwad22 mind fixing the DCO problem?

All fixed, and I believe everything else is passing. If there's nothing else you or @tsotnet would like changed, can we ship this?I only ask because the changes touch many files so the longer we wait, the greater number of merge conflicts we'll have to resolve.

@@ -182,6 +182,8 @@ def to_proto(self) -> FeatureViewProto:
ttl_duration = Duration()
ttl_duration.FromTimedelta(self.ttl)

print(f"Stream soruce: {self.stream_source}, {type(self.stream_source)}")
Copy link
Member

Choose a reason for hiding this comment

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

Should this be removed?

Copy link
Member

Choose a reason for hiding this comment

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

This has been removed in #1678

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove and push, must have gotten added when I rebased.

@woop
Copy link
Member

woop commented Jul 7, 2021

@Mwad22 mind fixing the DCO problem?

All fixed, and I believe everything else is passing. If there's nothing else you or @tsotnet would like changed, can we ship this?I only ask because the changes touch many files so the longer we wait, the greater number of merge conflicts we'll have to resolve.

Unfortunately @tsotnet's power is offline today. I will give the PR one final scan.

sdk/python/feast/infra/aws.py Outdated Show resolved Hide resolved
…fault now set only in feature_store.py

Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.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.

Thanks @Mwad22, this looks great to me. I had some super nitty things but happy to lgtm after they are addressed.

sdk/python/feast/errors.py Outdated Show resolved Hide resolved
sdk/python/feast/infra/offline_stores/bigquery.py Outdated Show resolved Hide resolved
sdk/python/tests/test_historical_retrieval.py Outdated Show resolved Hide resolved
sdk/python/tests/test_historical_retrieval.py Outdated Show resolved Hide resolved
@feast-ci-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

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

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

…nitpick changes

Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.com>
@Mwad22
Copy link
Contributor Author

Mwad22 commented Jul 8, 2021

Note, the unit tests that fail above are failing because of the following error:

An error occurred (LimitExceededException) when calling the CreateTable operation: Subscriber limit exceeded: There is a limit of 256 tables per subscriber

They are not caused by the changes in this PR.

@achals
Copy link
Member

achals commented Jul 8, 2021

/lgtm

@achals achals dismissed tsotnet’s stale review July 8, 2021 04:18

@woop and I took a look, @tsotnet's power was out today.

@feast-ci-bot feast-ci-bot merged commit 2e0113e into feast-dev:master Jul 8, 2021
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.

Provide option to strip feature view names from feature names