Properly exclude entities from feature inference #2048
Conversation
mavysavydav
commented
Nov 17, 2021
mavysavydav
commented
mavysavydav
requested review from
achals,
adchia,
felixwang9817,
tsotnet,
woop and
a team
as code owners
…d as features Signed-off-by: David Y Liu <[email protected]>
mavysavydav
force-pushed
the
mavysavydav-patch-1
branch
from
fb91715
to
44a5a4f
Compare
| fv.batch_source.event_timestamp_column, | ||
| fv.batch_source.created_timestamp_column, | ||
| } | { | ||
| entity_name_to_join_key_map[entity_name] for entity_name in fv.entities |
There was a problem hiding this comment.
this is the line that changed from the previous version of this method. Grabbing the join_keys to exclude rather than entity name. Everything else is mainly copy and paste.
There was a problem hiding this comment.
@mavysavydav is there a reason for the move to inference.py? I am not against it, but it makes reviewing the PR harder. Would it make sense to at least split these two changes into different commits to make it easier on the reviewer?
There was a problem hiding this comment.
yea i agree that review becomes more difficult. I had to move this method as part of this fix b/c it needs the list of entity objects which isn't available in feature_view.py. I can split out the odfv method move into a separate PR since that's more independent if you'd like
There was a problem hiding this comment.
have taken out the odfv code move
sdk/python/feast/inference.py
Outdated
| RegistryInferenceFailure: The set of features could not be inferred. | ||
| """ | ||
| for odfv in odfvs: | ||
| df = pd.DataFrame() |
There was a problem hiding this comment.
this method is basically the same, just moved to this file from on_demand_feature_views.py for consistency, and adjusted it's signature to be consistent with other inference methods
Codecov Report
@@ Coverage Diff @@
## master #2048 +/- ##
===========================================
+ Coverage 57.54% 82.50% +24.95%
===========================================
Files 100 100
Lines 8028 8465 +437
===========================================
+ Hits 4620 6984 +2364
+ Misses 3408 1481 -1927
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: codyjlin, mavysavydav 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 |
|
/ok-to-test |
Signed-off-by: David Y Liu <[email protected]>
mavysavydav
force-pushed
the
mavysavydav-patch-1
branch
from
a0dea5d
to
886d788
Compare
Signed-off-by: David Y Liu <[email protected]>
mavysavydav
force-pushed
the
mavysavydav-patch-1
branch
from
886d788
to
40df252
Compare
|
/lgtm |
* Proper fixes for consistency and to make sure entities aren't inferred as features Signed-off-by: David Y Liu <[email protected]> * Leaving out ODFV inference logic move for a different PR Signed-off-by: David Y Liu <[email protected]> * updated tests Signed-off-by: David Y Liu <[email protected]> (cherry picked from commit 63652e0)
