feat: Extend get_feature_view to return stream and on demand feature views#4328
feat: Extend get_feature_view to return stream and on demand feature views#4328tokoko wants to merge 1 commit intofeast-dev:masterfrom
get_feature_view to return stream and on demand feature views#4328Conversation
Signed-off-by: tokoko <[email protected]>
|
@dmartinol Ended up having to make changes all over. Maybe you can find time to look through. |
dmartinol
left a comment
There was a problem hiding this comment.
lgtm.
I just see a lot of assert isinstance(fv, FeatureView) that I hope are all correct as some of them, like the OnDemandFV, are only BaseFV instead. I cannot follow the complete flow, but I trust you 👍
|
That's mostly to accommodate the linter for now, I should probably add more feast-specific exceptions (for example if a user is asking to materialize an odfv, which doesn't make sense) On a general note, I found that we have a following trade-off here:
Despite the trade-off, I still think it's a worthwhile change. Still wanted to get your opinion. |
| ) | ||
| if hide_dummy_entity and feature_view.entities[0] == DUMMY_ENTITY_NAME: | ||
| if ( | ||
| isinstance(feature_view, FeatureView) |
There was a problem hiding this comment.
utils._list_feature_views has a similar logic but misses the isinstance check, don't we need it there too?
There was a problem hiding this comment.
Not yet, it's using list_feature_views to get the objects which is still FeatureView specific, this PR only makes changes to get_feature_view. I'll do that in a follow-up.
| on_demand_feature_view.spec.project == project | ||
| and on_demand_feature_view.spec.name == name | ||
| ): | ||
| return OnDemandFeatureView.from_proto(on_demand_feature_view) |
There was a problem hiding this comment.
the method is declared to return a FeatureView, isn't the linter complaining about that?
I think the reason is that OnDemandFeatureView.from_proto does not declare the retuned type 😉
It we really have this need, can't we apply an optional filter to return only instances of the requested type with no need to further check? fv : StreamFeatureView = fs.get_feature_view(name=..., accept_type=StreamFeatureView)
...
fvs: list[StreamFeatureView] = fs.list_feature_views(..., accept_type=StreamFeatureView)
for fv in fvs:
... |
Let me look into this. This is actually probably a better idea since we'll definitely need something like this for P.S. I'm afraid linter issues won't go away even in this case. type annotation will have to be |
Can't we surround it by fv : StreamFeatureView = cast(StreamFeatureView, fs.get_feature_view(name=..., accept_type=StreamFeatureView)) |
Which issue(s) this PR fixes: