GeezerVison is showing github.com — Registry get_feature_view should read stream and on-demand feature views · Issue #4235 · feast-dev/feast
Registry get_feature_view should read stream and on-demand feature views #4235DescriptionBaseRegistry has a strange non-symmetric behavior with CRUD operations of stream and on-demand feature views. All 3 types of feature views are applied with a apply_feature_view method that takes BaseFeatureView as an input, but for getting/listing existing feature views, there are 3 separate methods (get_feature_view, get_stream_feature_view, get_stream_feature_view and similar for listing). This is very impractical in practice as normally when a user asks for a FV by name, there's no way of knowing which type of FV the name refers to.
get_feature_view (and probably list_feature_views as well, but that will be hard to do in a backwards-compatible way) should be rewritten to return BaseFeatureView objects.
Reacted by Francisco Javier Arceo and Daniele Martinoli Activitytokoko commented on May 29, 2024 CollaboratorAuthor More actions dmartinol commented on May 29, 2024 can we keep backward compatibility by adding an optional includes_subclasses that default to False? tokoko commented on May 29, 2024 CollaboratorAuthor More actions Probably. I'm thinking of marking it as deprecated right from the get go and we can eventually make a breaking change. Reacted by Francisco Javier Arceo franciscojavierarceo commented on May 29, 2024 I can probably take this one if you're not in a rush for this. Reacted by Tornike Gurgenidze tokoko commented on May 30, 2024 CollaboratorAuthor More actions Sure, there are already workarounds for it, so we should get by in the meantime. I'll assign you, let us know if you won't be able to find time for it. Reacted by Francisco Javier Arceo franciscojavierarceo commented on Jun 4, 2024 Going to take care of this over the next couple of days.
Two steps:
- Create
FeatureViewType in the BaseFeatureView
- Add it to the 3 as metadata and use that to return the list of feature views broken down by type
I'm working on (1) right now and will update when done (it should be quick). tokoko commented on Jun 4, 2024 CollaboratorAuthor More actions Didn't really get the need for the first one, can't you just check the type of an object? franciscojavierarceo commented on Jun 4, 2024 Yeah, definitely could but I'd rather have more explicit metadata surrounding it.
That said, I really don't see any other data types outside of On Demand, Batch, and Stream. franciscojavierarceo commented on Jun 6, 2024 after looking into this, it's a pretty big change actually. I think it's worth doing but just want to state it explicitly. franciscojavierarceo commented on Jun 6, 2024 So first unblock is list_feature_views which is a clunky wrapper but at least gives some small support. I'll change the return object next but I thought I'd at least return the quick one.
#4256 tokoko commented on Aug 14, 2024 CollaboratorAuthor More actions I'm still scratching my head about what's the best way to solve this. So, I think we have two alternatives:
- parameterize
get_feature_view which will accept an additional param to filter search base based on feature view type. The default will be to search plain FeatureView types for backwards-compatibility. We will also at some point remove get_on_demand_feature_view and get_stream_feature_view methods/endpoints. The major downside with this is that get_feature_view will have to be annotated with BaseFeatureView return type and this causes all sorts of linter awkwardness even if the user is limiting the search base.
- Another way is to keep all existing methods intact and instead introduce additional methods
get_feature_view_any and list_feature_views_any (with better names maybe?). This will result in a similar behavior, but instead of concentrating the functionality in a single method, it will be spread out into several methods, which means that return types will make sense. If a user is looking for odfv only, get_on_demand_feature_view can be used which will always return OnDemandFeatureView. I'm leaning towards this approach now if we come up with a better name lol
dmartinol commented on Aug 14, 2024 Said that I'm still a bit confused about the goal (if the new API returns a BaseFeatureView, how the client code can use class-specific fields w/o doing isinstance first?), I would go for option 2 as well (VERB_any_feature_view?).
I imagine the legacy APIs would be still used to avoid the isinstance check Reacted by Tornike Gurgenidze tokoko commented on Aug 14, 2024 CollaboratorAuthor More actions Yes, in some cases (get_historical_features, get_online_features) FVs are provided as a string name and can resolve to any type of feature view, so some sort of type checking is necessary by design. The goal for the new API would be to address those scenarios. In other cases (materialization for example which makes sense only for FeatureView type) where only a specific type is applicable, "legacy" APIs can be used just as before w/o type checking.
get_any_feature_view sounds good, but can't say the same for list_any_feature_views. What about the duo of get_any_feature_view and list_all_feature_views?
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|