Set default feature naming to not include feature view name. Add option to include feature view name in feature naming. by Mwad22 · Pull Request #1641 · feast-dev/feast

Mwad22 · 2021-06-11T15:36:40Z

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()

woop · 2021-06-11T16:30:21Z

Thanks for this @Mwad22!

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>

Mwad22 · 2021-07-07T23:25:20Z

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

woop · 2021-07-07T23:40:59Z

            ttl_duration = Duration()
            ttl_duration.FromTimedelta(self.ttl)

+        print(f"Stream soruce: {self.stream_source}, {type(self.stream_source)}")


Should this be removed?

This has been removed in #1678

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

woop · 2021-07-07T23:41:37Z

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

…fault now set only in feature_store.py Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.com>

achals

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

feast-ci-bot · 2021-07-08T00:12:56Z

[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

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [achals]

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 · 2021-07-08T01:19:22Z

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 · 2021-07-08T04:14:08Z

/lgtm

@woop

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

Mwad22 requested review from a team, achals, tsotnet and woop as code owners June 11, 2021 15:36

feast-ci-bot added do-not-merge/release-note-label-needed needs-kind size/L labels Jun 11, 2021

Mwad22 changed the title ~~Mwad22 1618 PR~~ option to strip feature view names from feature names Jun 11, 2021

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

woop reviewed Jun 11, 2021

View reviewed changes

Comment thread .gitignore Outdated

woop reviewed Jun 11, 2021

View reviewed changes

Comment thread sdk/python/feast/feature_store.py Outdated

woop reviewed Jun 11, 2021

View reviewed changes

Comment thread sdk/python/feast/feature_store.py Outdated

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

mavysavydav reviewed Jun 11, 2021

View reviewed changes

Comment thread sdk/python/feast/feature_store.py Outdated

mavysavydav reviewed Jun 11, 2021

View reviewed changes

Comment thread pyvenv.cfg Outdated

mavysavydav reviewed Jun 11, 2021

View reviewed changes

Comment thread sdk/python/feast/feature_store.py Outdated

mavysavydav reviewed Jun 11, 2021

View reviewed changes

Comment thread sdk/python/feast/infra/offline_stores/bigquery.py Outdated

codyjlin reviewed Jun 11, 2021

View reviewed changes

Comment thread sdk/python/feast/infra/provider.py Outdated

Mwad22 force-pushed the mwad22-1618-PR branch from aff5016 to c611730 Compare June 12, 2021 00:35

feast-ci-bot added size/XXL and removed size/L labels Jun 12, 2021

Mwad22 force-pushed the mwad22-1618-PR branch from c611730 to 3c08212 Compare June 12, 2021 01:36

feast-ci-bot added size/L and removed size/XXL labels Jun 12, 2021

mavysavydav reviewed Jun 12, 2021

View reviewed changes

Comment thread sdk/python/feast/feature_store.py Outdated

mavysavydav reviewed Jun 12, 2021

View reviewed changes

Comment thread sdk/python/feast/feature_store.py Outdated

mavysavydav reviewed Jun 12, 2021

View reviewed changes

Comment thread sdk/python/feast/infra/provider.py Outdated

mavysavydav reviewed Jun 12, 2021

View reviewed changes

Comment thread sdk/python/tests/test_historical_retrieval.py Outdated

mavysavydav reviewed Jun 12, 2021

View reviewed changes

Comment thread sdk/python/tests/test_historical_retrieval.py Outdated

Mwad22 and others added 5 commits July 7, 2021 17:40

Update sdk/python/feast/feature_store.py

f400d65

Co-authored-by: Willem Pienaar <6728866+woop@users.noreply.github.com> Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.com>

made error logic/messages more descriptive

082fca7

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

made error logic/messages more descriptive.

3aca976

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

Simplified error messages

79aa736

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

ran formatter, issue in errors.py

d7d08ef

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

Mwad22 force-pushed the mwad22-1618-PR branch from 96e3455 to d7d08ef Compare July 7, 2021 21:48

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

Merge branch 'master' into mwad22-1618-PR

2ab8eea

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

python linter issues resolved

650340d

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

Mwad22 force-pushed the mwad22-1618-PR branch from 15e2a61 to 650340d Compare July 7, 2021 23:11

woop reviewed Jul 7, 2021

View reviewed changes

Comment thread sdk/python/feast/infra/aws.py Outdated

removed unnecessary default assignment in get_historical_features. de…

5d582a6

…fault now set only in feature_store.py Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.com>

achals approved these changes Jul 8, 2021

View reviewed changes

Comment thread sdk/python/feast/errors.py Outdated

Comment thread sdk/python/feast/infra/offline_stores/bigquery.py Outdated

Comment thread sdk/python/tests/test_historical_retrieval.py Outdated

Comment thread sdk/python/tests/test_historical_retrieval.py Outdated

feast-ci-bot added the approved label Jul 8, 2021

added error message assertion for feature name collisions, and other …

8724e0b

…nitpick changes Signed-off-by: Mwad22 <51929507+Mwad22@users.noreply.github.com>

feast-ci-bot assigned achals Jul 8, 2021

feast-ci-bot added the lgtm label Jul 8, 2021

feast-ci-bot merged commit 2e0113e into feast-dev:master Jul 8, 2021

achals mentioned this pull request Jul 8, 2021

Remove sys.exit from exception handling #1666

Closed

Conversation

Mwad22 commented Jun 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

woop commented Jun 11, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Mwad22 commented Jul 7, 2021

Uh oh!

woop Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

achals Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

Mwad22 Jul 7, 2021

Choose a reason for hiding this comment

Uh oh!

woop commented Jul 7, 2021

Uh oh!

Uh oh!

achals left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

feast-ci-bot commented Jul 8, 2021

Uh oh!

Mwad22 commented Jul 8, 2021

Uh oh!

achals commented Jul 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

Mwad22 commented Jun 11, 2021 •

edited

Loading