Fix unexpected feature view deletion when applying edited odfv #2054
Conversation
…ature-view has changed. Signed-off-by: ArrichM <maximilianjakob.arrich@student.unisg.ch>
ArrichM
requested review from
achals,
adchia,
felixwang9817,
tsotnet,
woop and
a team
as code owners
|
Hi @ArrichM. 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 Once the patch is verified, the new status will be reflected by the 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 Report
@@ Coverage Diff @@
## master #2054 +/- ##
==========================================
+ Coverage 57.54% 58.30% +0.75%
==========================================
Files 100 100
Lines 8028 8066 +38
==========================================
+ Hits 4620 4703 +83
+ Misses 3408 3363 -45
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -341,7 +341,7 @@ def apply_feature_view( | |||
| ): | |||
| return | |||
| else: | |||
| del self.cached_registry_proto.feature_views[idx] | |||
| del existing_feature_views_of_same_type[idx] | |||
There was a problem hiding this comment.
good catch - do you mind adding a test in test_registry.py for this case? I definitely feel like we should have caught this bug
There was a problem hiding this comment.
Sure, I just added a case testing the modification of an odfv.
…re not interfered with when modifying on-demand-feature-views. Signed-off-by: ArrichM <maximilianjakob.arrich@student.unisg.ch>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ArrichM, 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:
Approvers can indicate their approval by writing |
|
/kind bug |
|
/ok-to-test |
What this PR does / why we need it:
When changing existing on-demand-feature-views, the existing proto in the registry needs to be replaced by the new one. In this step, the existing proto has been deleted from the wrong container. The existing implementation always deletes the proto of the given index from the REGULAR feature-view container, not the odfv container.
This leads to:
Does this PR introduce a user-facing change?: