Registry get_feature_view should read stream and on-demand feature views

BaseRegistry 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.

@dmartinol

cc @dmartinol

can we keep backward compatibility by adding an optional includes_subclasses that default to False?

Probably. I'm thinking of marking it as deprecated right from the get go and we can eventually make a breaking change.

I can probably take this one if you're not in a rush for this.

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.

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).

Didn't really get the need for the first one, can't you just check the type of an object?

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.

after looking into this, it's a pretty big change actually. I think it's worth doing but just want to state it explicitly.

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

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

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

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?

fix: Resolve write_to_offline_store feature view with a single regist…

chore(release): release 0.66.0

fix: Resolve write_to_offline_store feature view with a single regist…

chore(release): release 0.66.0

Navigation Menu

Repository navigation

Registry get_feature_view should read stream and on-demand feature views #4235

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Registry get_feature_view should read stream and on-demand feature views #4235

Description

Activity

tokoko commented on May 29, 2024

dmartinol commented on May 29, 2024

tokoko commented on May 29, 2024

franciscojavierarceo commented on May 29, 2024

tokoko commented on May 30, 2024

franciscojavierarceo commented on Jun 4, 2024

tokoko commented on Jun 4, 2024

franciscojavierarceo commented on Jun 4, 2024

franciscojavierarceo commented on Jun 6, 2024

franciscojavierarceo commented on Jun 6, 2024

tokoko commented on Aug 14, 2024

dmartinol commented on Aug 14, 2024

tokoko commented on Aug 14, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions