| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
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. |
Sorry, something went wrong.
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 👍
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
| ) | ||
| 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?
Sorry, something went wrong.
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.
Sorry, something went wrong.
| 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 😉
Sorry, something went wrong.
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:
... |
Sorry, something went wrong.
Let me look into this. This is actually probably a better idea since we'll definitely need something like this for list_feature_views method anyway, so why not here as well? I was just trying to keep the interface simple, because of the chain of changes this causes in registry server and so on. P.S. I'm afraid linter issues won't go away even in this case. type annotation will have to be BaseFeatureView and some asserts here and there will be necessary. |
Sorry, something went wrong.
Can't we surround it by cast? (asserts of accept_type can be hidden in the methods for all the clients): fv : StreamFeatureView = cast(StreamFeatureView, fs.get_feature_view(name=..., accept_type=StreamFeatureView)) |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Which issue(s) this PR fixes: