| def GetAnyFeatureView( | ||
| self, request: RegistryServer_pb2.GetAnyFeatureViewRequest, context | ||
| ): | ||
| feature_view = assert_permissions( |
There was a problem hiding this comment.
👍
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm!
Sorry, something went wrong.
There was a problem hiding this comment.
Blocking PR
Sorry, something went wrong.
| bool commit = 3; | ||
| } | ||
|
|
||
| message AnyFeatureView { |
There was a problem hiding this comment.
why AnyFeatureView and not AllFeatureViews?
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm, this is a single message though so I see.
Sorry, something went wrong.
There was a problem hiding this comment.
Why wouldn't we call it FeatureViewType?
Sorry, something went wrong.
There was a problem hiding this comment.
I feel like type is something else, it would refer to one the types, not one of the objects. AnyFeatureView basically stands for AnyTypeOfFeatureView.
Sorry, something went wrong.
There was a problem hiding this comment.
Oh I see what you're doing this for.
Man this is ugly.
The get_any_feature_view should probably be an intermediate solution and instead we should find a way to make get_feature_view include ODFV and Stream FVs.
Sorry, something went wrong.
There was a problem hiding this comment.
I get why we can't jump right to that, but maybe we should start a doc on what should be reconciled for release to Feast 1.0?
Sorry, something went wrong.
| self.commit() | ||
| return | ||
|
|
||
| raise FeatureViewNotFoundException(feature_view.name, project) |
There was a problem hiding this comment.
Did you mean to drop this from apply_materialization?
Sorry, something went wrong.
|
|
||
| all_feature_views = test_registry.list_all_feature_views(project, tags=sfv.tags) | ||
|
|
||
| print(stream_feature_views) |
There was a problem hiding this comment.
Can remove these print statements now
Sorry, something went wrong.
There was a problem hiding this comment.
Nice, this is great, thanks Tornike!
As mentioned in a comment, I think we should start jotting down some major clean ups we want to do to release 1.0. I'm experiencing this as I'm working through the writes to the ODFV and I think there's a way for us to clean up a lot more.
There's a lot of code in between class instantiation and apply that is very opaque and makes it hard to reason about what's happening. It's for reasonable choices but I think it warrants a broad review of the online store.
Also, it could be a good time to establish DuckDB as the offline store default.
Sorry, something went wrong.
|
As mentioned in a comment, I think we should start jotting down some major clean ups we want to do to release 1.0. I'm experiencing this as I'm working through the writes to the ODFV and I think there's a way for us to clean up a lot more. Yeah, there's probably a lot of discussion to be had until we finalize these APIs for 1.0 release. We should definitely continue the debate in #4235. The problem with get_feature_view is that it might be interpreted as referring to all feature views and also to plain FeatureView objects specifically. Also, it could be a good time to establish DuckDB as the offline store default. Yeah, that's probably a pretty good idea. I recently had to make a simple demo and saw that on small datasets duckdb was a lot faster than the current default, dask. I'll open a ticket for this and let's continue there, namely I think we should agree whether duckdb being the default means that it should be a required dependency or not (I think not...) |
Sorry, something went wrong.
|
As mentioned in a comment, I think we should start jotting down some major clean ups we want to do to release 1.0. I'm experiencing this as I'm working through the writes to the ODFV and I think there's a way for us to clean up a lot more. Yeah, there's probably a lot of discussion to be had until we finalize these APIs for 1.0 release. We should definitely continue the debate in #4235. The problem with get_feature_view is that it might be interpreted as referring to all feature views and also to plain FeatureView objects specifically. Also, it could be a good time to establish DuckDB as the offline store default. Yeah, that's probably a pretty good idea. I recently had to make a simple demo and saw that on small datasets duckdb was a lot faster than the current default, dask. I'll open a ticket for this and let's continue there, namely I think we should agree whether duckdb being the default means that it should be a required dependency or not (I think not...) I think a default offline store should be required, so in that case we shouldn't make it the default. What are your thoughts about us drafting a 1.0 Roadmap? It's September now, so a rational question would be: could we get it to 1.0 by end of the year? I'm not sure (probably not). |
Sorry, something went wrong.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #4235