-
Notifications
You must be signed in to change notification settings - Fork 968
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Extend get_feature_view
to return stream and on demand feature views
#4328
Conversation
Signed-off-by: tokoko <togurg14@freeuni.edu.ge>
@dmartinol Ended up having to make changes all over. Maybe you can find time to look through. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
feature_view = self._registry.get_feature_view( | ||
name, self.project, allow_cache=allow_registry_cache | ||
) | ||
if hide_dummy_entity and feature_view.entities[0] == DUMMY_ENTITY_NAME: | ||
if ( | ||
isinstance(feature_view, FeatureView) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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: