Make sure FeatureViews with same name can not be applied at the same … #1651

tedhtchang · 2021-06-17T00:51:00Z

What this PR does / why we need it:
Make sure FeatureViews with same name can not be applied at the same time.

Which issue(s) this PR fixes:
Fix #1625

Does this PR introduce a user-facing change?:

Users will get an error when trying to apply feature views with the same name.

feast-ci-bot · 2021-06-17T00:51:10Z

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

woop · 2021-06-17T01:58:26Z

sdk/python/feast/feature_store.py

@@ -220,7 +220,17 @@ def apply(
            objects = [objects]
        assert isinstance(objects, list)

-        views_to_update = [ob for ob in objects if isinstance(ob, FeatureView)]
+        views_to_update = []
+        view_name_list = []


Perhaps simpler to use a dict?

codecov-commenter · 2021-06-17T07:40:51Z

Codecov Report

Merging #1651 (1baa0ee) into master (489a0f8) will increase coverage by 0.12%.
The diff coverage is 95.23%.

@@            Coverage Diff             @@
##           master    #1651      +/-   ##
==========================================
+ Coverage   79.08%   79.21%   +0.12%     
==========================================
  Files          80       81       +1     
  Lines        6855     6994     +139     
==========================================
+ Hits         5421     5540     +119     
- Misses       1434     1454      +20

Flag	Coverage Δ
integrationtests	`79.13% <95.23%> (+0.13%)`	⬆️
unittests	`62.07% <95.23%> (+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	`31.19% <50.00%> (-0.15%)`	⬇️
...sts/integration/registration/test_feature_store.py	`99.31% <90.00%> (-0.69%)`	⬇️
sdk/python/feast/feature_store.py	`93.66% <100.00%> (+0.16%)`	⬆️
...ion/test_cli_apply_duplicated_featureview_names.py	`100.00% <100.00%> (ø)`
sdk/python/feast/entity.py	`73.98% <0.00%> (-2.60%)`	⬇️
sdk/python/feast/feature_view.py	`86.95% <0.00%> (-2.28%)`	⬇️
sdk/python/feast/infra/offline_stores/redshift.py	`89.80% <0.00%> (-1.11%)`	⬇️
sdk/python/feast/feature.py	`75.00% <0.00%> (ø)`
sdk/python/feast/feature_service.py	`71.21% <0.00%> (ø)`
... and 3 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 489a0f8...1baa0ee. Read the comment docs.

sdk/python/feast/feature_store.py

woop · 2021-06-25T20:10:23Z

@tedhtchang from a readability standpoint, would you be able to pull out this validation into standalone function? Perhaps something like

validate_repo(repo: ParsedRepo)? or alternatively validate_feature_views(feature_views: List[FeatureView])?

tedhtchang · 2021-07-15T19:15:13Z

@woop @achals Do we need more action for this PR?

achals · 2021-07-15T19:30:00Z

@tedhtchang can we add a test? Other than that and DCO, the change looks good to me.

achals

waiting on tests

sdk/python/feast/repo_operations.py

sdk/python/feast/feature_store.py

sdk/python/feast/repo_operations.py

achals

/lgtm

sdk/python/tests/test_feature_store.py

sdk/python/feast/feature_store.py

feast-ci-bot · 2021-07-23T14:16:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: achals, tedhtchang, woop

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,woop]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

woop · 2021-07-23T14:17:07Z

/lgtm

…time Signed-off-by: ted chang <[email protected]>

Signed-off-by: ted chang <[email protected]>

Co-authored-by: Willem Pienaar <[email protected]> Signed-off-by: ted chang <[email protected]>

Signed-off-by: ted chang <[email protected]>

tedhtchang · 2021-07-28T00:32:00Z

/cc @achals @woop
I just resolved some conflict from the master branch.

achals · 2021-07-28T20:30:19Z

/lgtm

tedhtchang requested review from achals, tsotnet, woop and a team as code owners June 17, 2021 00:51

feast-ci-bot added release-note needs-kind labels Jun 17, 2021

feast-ci-bot added needs-ok-to-test size/S labels Jun 17, 2021

woop reviewed Jun 17, 2021

View reviewed changes

woop reviewed Jun 25, 2021

View reviewed changes

sdk/python/feast/feature_store.py Outdated Show resolved Hide resolved

woop reviewed Jun 25, 2021

View reviewed changes

sdk/python/feast/feature_store.py Outdated Show resolved Hide resolved

tedhtchang force-pushed the validate_duplicate_fv_name branch from ecb2657 to 2cdf83c Compare June 25, 2021 18:16

tedhtchang force-pushed the validate_duplicate_fv_name branch from 2cdf83c to c3c700b Compare June 28, 2021 11:10

feast-ci-bot added size/M and removed size/S labels Jun 28, 2021

achals requested changes Jul 16, 2021

View reviewed changes

woop reviewed Jul 16, 2021

View reviewed changes

sdk/python/feast/repo_operations.py Outdated Show resolved Hide resolved

woop reviewed Jul 16, 2021

View reviewed changes

sdk/python/feast/feature_store.py Outdated Show resolved Hide resolved

woop reviewed Jul 16, 2021

View reviewed changes

sdk/python/feast/repo_operations.py Outdated Show resolved Hide resolved

tedhtchang force-pushed the validate_duplicate_fv_name branch from c3c700b to f231d8a Compare July 17, 2021 09:28

feast-ci-bot added size/L and removed size/M labels Jul 17, 2021

tedhtchang force-pushed the validate_duplicate_fv_name branch 2 times, most recently from 5f89216 to e338738 Compare July 17, 2021 10:12

achals approved these changes Jul 20, 2021

View reviewed changes

feast-ci-bot added lgtm approved labels Jul 20, 2021

woop reviewed Jul 20, 2021

View reviewed changes

sdk/python/tests/test_feature_store.py Outdated Show resolved Hide resolved

tedhtchang force-pushed the validate_duplicate_fv_name branch from e338738 to 5a1f0e4 Compare July 20, 2021 23:07

feast-ci-bot removed the lgtm label Jul 20, 2021

tedhtchang force-pushed the validate_duplicate_fv_name branch from 5a1f0e4 to d6a1fd5 Compare July 22, 2021 01:22

woop reviewed Jul 23, 2021

View reviewed changes

sdk/python/feast/feature_store.py Outdated Show resolved Hide resolved

tedhtchang force-pushed the validate_duplicate_fv_name branch 2 times, most recently from 2bbbc44 to 083447a Compare July 23, 2021 08:29

woop approved these changes Jul 23, 2021

View reviewed changes

feast-ci-bot assigned woop Jul 23, 2021

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

tedhtchang and others added 5 commits July 27, 2021 15:30

Make sure FeatureViews with same name can not be applied at the same …

b1800d2

…time Signed-off-by: ted chang <[email protected]>

Add tests for applying feature views with duplicated names

e5c0974

Signed-off-by: ted chang <[email protected]>

Resolve refactoring conflict

ecb32b6

Signed-off-by: ted chang <[email protected]>

Update sdk/python/feast/feature_store.py

0953695

Co-authored-by: Willem Pienaar <[email protected]> Signed-off-by: ted chang <[email protected]>

Add another test case

1baa0ee

Signed-off-by: ted chang <[email protected]>

tedhtchang force-pushed the validate_duplicate_fv_name branch from 837a23a to 1baa0ee Compare July 28, 2021 00:23

feast-ci-bot removed the lgtm label Jul 28, 2021

feast-ci-bot requested review from achals and woop July 28, 2021 00:32

achals added kind/bug and removed needs-kind needs-ok-to-test labels Jul 28, 2021

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

feast-ci-bot merged commit 7972992 into feast-dev:master Jul 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make sure FeatureViews with same name can not be applied at the same … #1651

Make sure FeatureViews with same name can not be applied at the same … #1651

tedhtchang commented Jun 17, 2021 •

edited

Loading

feast-ci-bot commented Jun 17, 2021

woop Jun 17, 2021

achals Jun 17, 2021

codecov-commenter commented Jun 17, 2021 •

edited

Loading

woop commented Jun 25, 2021 •

edited

Loading

tedhtchang commented Jul 15, 2021

achals commented Jul 15, 2021

achals left a comment

achals left a comment

feast-ci-bot commented Jul 23, 2021

woop commented Jul 23, 2021

tedhtchang commented Jul 28, 2021

achals commented Jul 28, 2021

Make sure FeatureViews with same name can not be applied at the same … #1651

Make sure FeatureViews with same name can not be applied at the same … #1651

Conversation

tedhtchang commented Jun 17, 2021 • edited Loading

feast-ci-bot commented Jun 17, 2021

woop Jun 17, 2021

Choose a reason for hiding this comment

achals Jun 17, 2021

Choose a reason for hiding this comment

codecov-commenter commented Jun 17, 2021 • edited Loading

Codecov Report

woop commented Jun 25, 2021 • edited Loading

tedhtchang commented Jul 15, 2021

achals commented Jul 15, 2021

achals left a comment

Choose a reason for hiding this comment

achals left a comment

Choose a reason for hiding this comment

feast-ci-bot commented Jul 23, 2021

woop commented Jul 23, 2021

tedhtchang commented Jul 28, 2021

achals commented Jul 28, 2021

tedhtchang commented Jun 17, 2021 •

edited

Loading

codecov-commenter commented Jun 17, 2021 •

edited

Loading

woop commented Jun 25, 2021 •

edited

Loading